947194 - Moz2Dify Image::GetAsSurface

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

Image::GetAsSurface should be converted to return a gfx::SourceSurface instead of a gfxASurface.

All the Image implementations need to be converted as well so it may be worth trying to separate this into small tasks (I haven't looked at the amount of users of GetAsSurface, so I don't know for sure how much work this is).

Comment 1

[Ali Ak]

Attachment: Moz2Dify Image::GetAsSurface

WIP. Replace Image::GetAsSurface with Image::GetAsSourceSurface
and all usages of it

Comment 2

[Ali Ak]

Hey,

A few questions about this direction. It looks like this patch might explode though. 

So I started looking at only converting CairoImage to return a gfx::SourceSurface. Steps taken so far include:

1) Add an nsAutoRefTraits specialization on nsMainThreadSourceSurfaceRef; typedefed gfx::SourceSurface as the RawRef. Left the runnable implementation the same as the one foe nsMainThreadSurfaceRef (which has a gfxASurface* typedefed as RawRef)
2) Change ImageCairo to hold a nsCountedRef<nsMainThreadSourceSurfaceRef> which internally add refs and unrefs a gfx::SourceSurface.
3) Added a virtual GetAsSourceSurface to base class Image, and implemented it as returning a TemporaryRef<gfx::SourceSurface> by calling RefPtr<gfx::SourceSurface>::get() and then nsCountedRef<nsMainThreadSourceSurfaceRef>::forget() in CairoImage
4) Changed CairoImage::SetData to set a gfx::SourceSurface instead.

5) Change usages of CairoImage::GetAsSurface...

But that seems much more complicated than anticipated. Or maybe there's a better way to do it. I'm not even sure if the conversions are correct. Please provide feedback on the direction being taken, the idioms used to change from thebes to moz2d, and how to avoide the currect explosion of what needs to be changed. The patch above is at the point where might as well just change all occurances at once so the way it's going is probably a bad idea?

Comments please :)

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

I don't feel super competent on these matters. If you need info from another person than :nical, try :nrc or :mattwoodrow.

Comment 4

[Matt Woodrow (:mattwoodrow)]

For the most part this looks pretty good, but there's a lot missing before it will run without regressions.

I think you want to try split this up into smaller pieces.

My suggestion would be to add GetAsSourceSurface() and implement it using gfxPlatform::GetSourceSurfaceForSurface.

Then you can do a patch for each Image subclass, implementing it 'natively' (like you're doing for CairoImage).

And also a set of patches for each of the consumers, changing them one at a time.

That should be easier to make progress on, rather than trying to handle the entire web of dependencies in a single patch.

Comment 5

[Ali Ak]

Attachment: Fix warnings in accessible

Comment 6

[Ali Ak]

Attachment: Implement base Image::GetAsSourceSurface

Comment 7

[Ali Ak]

Attachment: Deprecate ImageContaier gfxASurface calls

Changed ImageContainer::GetCurrentAsSurface and LockCurrentAsSurface
to Deprecated, and added equivalent SourceSurface variants.

Comment 8

[Ali Ak]

Attachment: Fix some warnings in layout

Comment 9

[Ali Ak]

Attachment: Fix warnings in accessible

Comment 10

[Ali Ak]

Attachment: [PATCH] Fixed gfxSurfaceDrawable to take SourceSurface instead of DrawTarget.

It is the surface that is drawn into others, so it shouldn't take DrawTargets.
---
 gfx/thebes/gfxDrawable.cpp |   18 +++++-------------
 gfx/thebes/gfxDrawable.h   |    6 +++---
 gfx/thebes/gfxUtils.cpp    |    4 +++-
 image/src/SurfaceCache.cpp |   28 ++++++++++++++--------------
 image/src/SurfaceCache.h   |   10 +++++-----
 image/src/VectorImage.cpp  |    5 +++--
 6 files changed, 33 insertions(+), 38 deletions(-)

Comment 11

[Ali Ak]

Ok, so moving along :)

I ran in to compiler warnings so fixed those as separate patches and pushed as well in this bug. Hope that's ok?

So when I started changing AutoLockImage, that's when things get out of control. Ended up having to moz2dify a gfxSurfaceDrawable. It seems like it takes a DrawTarget, when in fact it should take a SourceSurface since it's drawn and never drawed to.

ddenis (who sits next to me at work) is helping out with this bug as well, so there's a patch from him to change the ctor on gfxSurfaceDrawable)

Is that the right direction?

Comment 12

[Ali Ak]

Ran in to a small snag. Next step I changed AutoLockImage's ctor to take a SourceSurface instead of a gfxASurface. It's used in 2 places so quite contained. Build, ran, crashed.

BasicImageLayer::GetAndPaintCurrentImage used to auto lock on a gfxASurface and create a gfxPattern out of it and pass that pattern down to BasicImageLayer::PaintContext -> gfxContext::FillWithOpacity -> gfxContext::FillAzure -> DrawTargetCG::FillRect.

FillRect then calls GetImageFromSourceSurface on SourceSurface taken out of the pattern object and calls abort because the SourceSurface::GetType is not any of SURFACE_COREGRAPHICS_IMAGE, SURFACE_COREGRAPHICS_CGCONTEXT or SURFACE_DATA.

Tried changing Image::GetAsSourceSurface to return SourceSurface->GetDataSurface() (since I assume that would be at least a SURFACE_DATA type. But still goes boom.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8347229 [details] [diff] [review]
[PATCH] Fixed gfxSurfaceDrawable to take SourceSurface instead of DrawTarget.

Review of attachment 8347229 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxDrawable.cpp
@@ +118,5 @@
>                           const gfxMatrix& aTransform)
>  {
>      nsRefPtr<gfxPattern> pattern;
> +    if (mSourceSurface) {
> +      pattern = new gfxPattern(mSourceSurface, Matrix());

If aContext->IsCairo() is true, then this won't work. That's why we passed a DrawTarget previously, so that we can fallback to GetThebesSurfaceForDrawTarget if necessary.

Ideally that would never happen, but it's possible we still have some callers relying on Thebes surfaces.

Have you checked to see if this passes try? Adding a MOZ_ASSERT(!aContext->IsCairo()) would be worthwhile too, since we'll probably just silently not-draw otherwise.

If it fails then we can fix the caller, or handle both a DrawTarget and a SourceSurface (which is what m-c has now).

Comment 14

[Matt Woodrow (:mattwoodrow)]

(In reply to Ali Ak from comment #12)
> Ran in to a small snag. Next step I changed AutoLockImage's ctor to take a
> SourceSurface instead of a gfxASurface. It's used in 2 places so quite
> contained. Build, ran, crashed.
> 
> BasicImageLayer::GetAndPaintCurrentImage used to auto lock on a gfxASurface
> and create a gfxPattern out of it and pass that pattern down to
> BasicImageLayer::PaintContext -> gfxContext::FillWithOpacity ->
> gfxContext::FillAzure -> DrawTargetCG::FillRect.
> 
> FillRect then calls GetImageFromSourceSurface on SourceSurface taken out of
> the pattern object and calls abort because the SourceSurface::GetType is not
> any of SURFACE_COREGRAPHICS_IMAGE, SURFACE_COREGRAPHICS_CGCONTEXT or
> SURFACE_DATA.
> 
> Tried changing Image::GetAsSourceSurface to return
> SourceSurface->GetDataSurface() (since I assume that would be at least a
> SURFACE_DATA type. But still goes boom.

Admittedly the GetType() is a bit confused at the moment. Some of the classes that implement DataSourceSurface return SURFACE_DATA, but others return something more specific.

You're using CreateDrawTargetForSurface (which creates a BACKEND_CAIRO/DrawTargetCairo DT), and then passing that to GetSourceSurfaceForSurface, so you get a BACKEND_CAIRO SourceSurface. We don't really interop between backend types.

Skip creating the DT, and pass nullptr as the first argument for GetSourceSurfaceForSurface. This will use the platform default backend type (which will be CG for you), and then DrawTargetCG will be much happier.

Comment 15

[Ali Ak]

(In reply to Matt Woodrow (:mattwoodrow) from comment #14)
> Admittedly the GetType() is a bit confused at the moment. Some of the
> classes that implement DataSourceSurface return SURFACE_DATA, but others
> return something more specific.
> 
> You're using CreateDrawTargetForSurface (which creates a
> BACKEND_CAIRO/DrawTargetCairo DT), and then passing that to
> GetSourceSurfaceForSurface, so you get a BACKEND_CAIRO SourceSurface. We
> don't really interop between backend types.
> 
> Skip creating the DT, and pass nullptr as the first argument for
> GetSourceSurfaceForSurface. This will use the platform default backend type
> (which will be CG for you), and then DrawTargetCG will be much happier.

Aha, I see. And yeah that seems to do the trick. Thanks! I'll push the new patches. Also realized that I forgot to deprecate Image::GetAsSurface and went straight to ImageContainer. Will Fix that too.

Comment 16

[Ali Ak]

Attachment: Patch 1: Bug 947194 - Implement base Image::GetAsSourceSurface

Added an Image::GetAsSourceSurface to return a gfx::SourceSurface
and implemented in terms of gfxPlatform::GetSourceSurfaceForSurface.

Comment 17

[Ali Ak]

Attachment: Patch 2: Bug 947194 - Deprecate Image::GetAsSurface in base and derived classes

Modified the following files replaceing ::GetAsSurface with
::DeprecatedGetAsSurface and modified calleres.

	modified:   gfx/layers/D3D9SurfaceImage.cpp
	modified:   gfx/layers/D3D9SurfaceImage.h
	modified:   gfx/layers/GrallocImages.cpp
	modified:   gfx/layers/GrallocImages.h
	modified:   gfx/layers/ImageContainer.cpp
	modified:   gfx/layers/ImageContainer.h
	modified:   gfx/layers/MacIOSurfaceImage.h
	modified:   gfx/layers/SharedTextureImage.h
	modified:   gfx/layers/basic/BasicImages.cpp
	modified:   gfx/layers/client/ImageClient.cpp
	modified:   gfx/layers/d3d10/ImageLayerD3D10.cpp
	modified:   gfx/layers/d3d10/ImageLayerD3D10.h
	modified:   gfx/layers/ipc/SharedPlanarYCbCrImage.cpp
	modified:   gfx/layers/ipc/SharedPlanarYCbCrImage.h
	modified:   gfx/layers/ipc/SharedRGBImage.cpp
	modified:   gfx/layers/ipc/SharedRGBImage.h

Comment 18

[Ali Ak]

Attachment: Patch 3: Bug 947194 - Add Moz2D SourceSurface calls to ImageContainer

Added member functions GetCurrentAsSourceSurface and
LockCurrentAsSourceSurface to return a gfx::SourceSurface instead
of a gfxASurface

Comment 19

[Ali Ak]

Attachment: Patch 4: Bug 947194 - Deprecate ImageContainer gfxASurface calls

Deprecate LockCurrentAsSurface and GetCurrentAsSurface inside
ImageContainer

Comment 20

[Ali Ak]

Attachment: Patch 5: Bug 947194 - Moz2Dify AutoLockImage contructor

Changed the AucoLockImage constructor to take a SourceSurface
instead of a gfxASurface

Comment 21

[Ali Ak]

Attachment: Patch 6: Bug 947194 - Deprecate TextureClientSurface::UpdateSurface

Added TextureClientSurface::UpdateSourceSurface to use
gfx::SourceSurface instead of gfxASurface

Comment 22

[Ali Ak]

Ok so do you think this would be ready for checking in now after the specific GetAsSourceSurface functions are implemented for each Image subclass? 

Then after the remaining Image patches, I think we should create another bug to replace ImageContainer::DeprecatedLockCurrentAsSurface and DeprecatedGetCurrentAsSurface with their equivalent SourceSurface counterparts. That would include changing SurfaceFromElementResult to have a SourceSurface as a member and changing BasicImageLayer::GetAsSurface to BasicImageLayer::GetAsSourceSurface (I guess those two can be worked on separately?)

Thoughts?

Comment 23

[Ali Ak]

Attachment: Patch 7: Bug 947194 - Implement CairoImage::GetAsSourceSurface

Deprecate CairoImage::Data::mSurface and add an mSourceSurface.
Also change all the callers of CairoImage::SetData to set the
SourceSurface as well.

Also change scope of CairoImage::mSource to private (mSurface was being
used raw, encapsulation what?)

Comment 24

[Nicolas Silva [:nical]]

(In reply to Ali Ak from comment #22)
> Ok so do you think this would be ready for checking in now after the
> specific GetAsSourceSurface functions are implemented for each Image
> subclass? 

There is a lot of code here and I haven't looked at most of it, so it's hard to answer this question right away. Once your patch queue has been reviewed and if it passes the tests then it should be landed for sure.
If some of the patches are finished you should flag them for review (plus it's a good way to get people to look at it). Also if there is a set of patches that you know can be applied without breaking functionality, you should ask someone to push them to try servers so as to make sure it doesn't break anything.

> 
> Then after the remaining Image patches, I think we should create another bug
> to replace ImageContainer::DeprecatedLockCurrentAsSurface and
> DeprecatedGetCurrentAsSurface with their equivalent SourceSurface
> counterparts.

Yes splitting bugs into smaller ones with well defined tasks is always a good idea.

Comment 25

[Ali Ak]

Attachment: Patch 8: Bug 947194 - Implement RemoteBitmapImage::GetAsSourceSurface

Just added a subclass specific implementation of GetAsSourceSurface.

Comment 26

[Ali Ak]

(In reply to Nicolas Silva [:nical] from comment #24)
> There is a lot of code here and I haven't looked at most of it, so it's hard
> to answer this question right away. Once your patch queue has been reviewed
> and if it passes the tests then it should be landed for sure.
> If some of the patches are finished you should flag them for review (plus
> it's a good way to get people to look at it). Also if there is a set of
> patches that you know can be applied without breaking functionality, you
> should ask someone to push them to try servers so as to make sure it doesn't
> break anything.

Ok cool. I will get someone with try access push to try for me, and if all is well there I'll mark them for review

Comment 27

[Ali Ak]

Attachment: Patch 8: Bug 947194 - Implement RemoteBitmapImage::GetAsSourceSurface

Just added a subclass specific implementation of GetAsSourceSurface.

Comment 28

[Andreas Pehrson [:pehrsons]]

try: https://tbpl.mozilla.org/?tree=Try&rev=eee46edf1942

Comment 29

[Ali Ak]

Attachment: Patch 7: Bug 947194 - Implement CairoImage::GetAsSourceSurface

Deprecate CairoImage::Data::mSurface and add an mSourceSurface.
Also change all the callers of CairoImage::SetData to set the
SourceSurface as well.

Also change scope of CairoImage::mSource to private (mSurface was being
used raw, encapsulation what?)

Comment 30

[Ali Ak]

Hey again!

Pushed to try, and everything except windows (missed a few things in imagelayerd3d9/10) builds and passes most tests.

I have a question about the tests though, for example in the try link in comment 28, all the reftests fail on one specific test, but that test's name has an open bug. So I assume this is ok?

Also, the Android 2.2 Opt X tests (XPCShellTest) fails on quite a number of stuff. Most have an open bug, but quite a few have closed bugs as well, but they're closed as wontfix for firefox 25. So I guess these can be ignored too?

There's one X test - https://bugzilla.mozilla.org/show_bug.cgi?id=881104 - that fails and looks like it's actually resolved (without a wontfix). But reading through the bug it also seems ignorable?

Comment 31

[Matt Woodrow (:mattwoodrow)]

(In reply to Ali Ak from comment #30)
> Hey again!
> 
> Pushed to try, and everything except windows (missed a few things in
> imagelayerd3d9/10) builds and passes most tests.
> 
> I have a question about the tests though, for example in the try link in
> comment 28, all the reftests fail on one specific test, but that test's name
> has an open bug. So I assume this is ok?

It does have an existing bug open, but this is for an intermittent failure. Your push appears to have made it fail every time. Also note that the number of differing pixels value is different.

You'll need to investigate this before these patches can land.

Everything else (apart from the mentioned windows failures) looks fine.

Comment 32

[Ali Ak]

Turning this bug into a meta bug and adding the bits and bobs that it depends on as separate bugs.

Comment 33

[Jonathan Watt [:jwatt]]

Fixed by bug 960524.