907292 - Use DataSourceSurface instead of gfxImageSurface in TextureHost::GetAsSurface

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

Here is another task in the effort toward replacing thebes by Moz2D:

Let's take the GetAsSurface methods of TextureHost and sons (gfx/layers/composite/TextureHost.h) and port it from using thebe's gfxImageSurface to using Moz2D's gfx::DataSourceSurface.

This has to be done for all the TextureHost classes. Some of them are in TextureHostOGL.h and TextureD3D11.h (be careful, the D3D ones are only compiled on windows so do not forget to convert them too if you are on linux or mac, because your compiler won't tell you if you forget them).

This is a rather mechanical task because gfxImageSurface and gfx::DataSourceSurface fulfill the same role, except they belong to two different APIs.

Comment 1

[Nicolas Silva [:nical]]

There is some info about porting code Moz2D in this blog post: http://mozillagfx.wordpress.com/2013/08/21/looking-for-a-good-first-place-to-contribute-to-gecko-gfx/

Comment 2

[Jesse Fox [:jfox]]

Hi Nicolas,

I would like to begin working on this bug. Is there anything else I should know that isn't already explained in your first comment? Thanks.

Comment 3

[Nicolas Silva [:nical]]

(In reply to Jesse Fox [:jfox] from comment #2)
> Hi Nicolas,
> 
> I would like to begin working on this bug. Is there anything else I should
> know that isn't already explained in your first comment? Thanks.

Great! I think most of the info is there but it is hard for me to guess all the questions that can arise when you are new to this code base, so you will most likely have questions along the way. Just don't hesitate to ask on the bug or email me ([e-mail]).

Actually I realize I didn't even mention what those GetAsSurface methods are for: when we build with MOZ_DUMP_PAINTING we dump the content of the elements that we composite to the screen for debugging purposes. To do that we read-back the textures that we have uploaded on the GPU and store it gfxImageSurface objects. the idea here is to store them in gfx::DataSourceSurface objects instead since it's the cool new API and we want to use it.
So it's normal that it will lead you to modify some code like this: http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/CompositableHost.cpp#l204 which uses this GetAsSurface feature.

Comment 4

[David Corvoysier]

Hi Nicolas,

Having read your blog post, the Moz2Dify bugs seemed a good starting point to get familiar with Firefox gfx code. I have started implementing this one in parallel of jesse (just as an exercise).

Converting GetAsSurface methods is indeed mechanical, but the calling code expects most of the time to be able to actually dump the resulting SourceSurface: is there some Moz2D code that could replace the Dump methods the gfxASurface has ? If not, what options do we have (convert to a gfxImage, create a dedicated DrawTarget ...) ?

Comment 5

[Nicolas Silva [:nical]]

(In reply to David Corvoysier from comment #4)
> Converting GetAsSurface methods is indeed mechanical, but the calling code
> expects most of the time to be able to actually dump the resulting
> SourceSurface: is there some Moz2D code that could replace the Dump methods
> the gfxASurface has ? If not, what options do we have (convert to a
> gfxImage, create a dedicated DrawTarget ...) ?

I think what you are looking for is in gfxUtils here:
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUtils.h#154

Comment 6

[David Corvoysier]

(In reply to Nicolas Silva [:nical] from comment #5)
> (In reply to David Corvoysier from comment #4)
> > Converting GetAsSurface methods is indeed mechanical, but the calling code
> > expects most of the time to be able to actually dump the resulting
> > SourceSurface: is there some Moz2D code that could replace the Dump methods
> > the gfxASurface has ? If not, what options do we have (convert to a
> > gfxImage, create a dedicated DrawTarget ...) ?
> 
> I think what you are looking for is in gfxUtils here:
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUtils.h#154

Thanks for the pointer. I only see however methods to dump a DrawTarget, whereas the particular data path I am looking at deals with buffers wrapped in SourceSurface objects ... I see three options: 
- add more gfxUtils methods to dump a SourceSurface (would that mean getting access to the underlying buffer, then wrapping it into a gfxSurface ?),
- Change the code calling GetAsSurface to work directly on the corresponding DrawTarget (but I don't think there is one in every case),
- Convert the SourceData to a "data" DrawTarget, and pass it to the dump methods.

Sorry if got it all wrong ...

Comment 7

[Nicolas Silva [:nical]]

There are probably other ways but one could be to create a draw target that uses the DataSourceSurface's data:
http://dxr.mozilla.org/mozilla-central/source/gfx/2d/2D.h?from=2d.h#l919

(that's probably what you meant in the third item)

Don't worry, those things aren't obvious when you are new to the code base.

Comment 8

[David Corvoysier]

Yes, that was what I meant in my third item. 

I am not sure however that I can use the method you pointed at directly, since it requires a backend, and I don't have a clue which one i am supposed to choose: one option I see would be to rely on gfxPlatform::getPlatform and use the CreateDrawTargetForData method instead (this uses the currently active backend, I believe). This last suggestion at least compiles properly ...

Comment 9

[Nicolas Silva [:nical]]

Ah, you are right. I think it is fine to use gfxPlatform::CreateDrawTargetForData here. Especially because here we will just read from content that is already rasterized. I'll make sure to add a reviewer that can confirm that.

Comment 10

[Jesse Fox [:jfox]]

Hi all,
I just wanted to check in and let you know I'm still working on this. I haven't had much time to work on it the past two weeks, but my schedule is looking much better for the rest of the month. Thanks for the patience.

Comment 11

[Andreas Pehrson [:pehrsons]]

Are you still working on this?
I have started looking at it as well to at least use it as a good learning lesson on the gfx code.

If any of you guys are close to having a patch ready, please go ahead.
But nothing commented in little over two months indicates that it might have stalled to me, in which case I'd be happy to take it over.

Comment 12

[Andreas Pehrson [:pehrsons]]

I have a patch that is about ready now. Is there an easy way to test if it compiles on Windows? The try server?

Should I also change the pointer type returned by GetAsSurface from already_AddRefed<> to TemporaryRef<> (which seem to be what Moz2D favors) ?

Comment 13

[Nicolas Silva [:nical]]

Sweet! yes it is better to use TemporaryRef<> instead of already_AddRefed<>.
Upload the patch on bugzilla and I'll push it to the try servers (don't hesitate to say "hey nical, please push this to try" when you do because I am dealing with enough emails these days that I may get mixed up or forget)

Comment 14

[Andreas Pehrson [:pehrsons]]

Attachment: Change gfxImageSurface to gfx::DataSourceSurface et al;

Comment 15

[Andreas Pehrson [:pehrsons]]

Ok, so nical, please push to try!
I'll add you as reviewer while I'm at it.

The code, bugzilla and mercurial (although considering to switch to git with the moz-git-tools) are all fairly new to me, so please advise on _anything_ I'm doing wrong =)

Comment 16

[Nicolas Silva [:nical]]

Comment on attachment 8337745 [details] [diff] [review]
Change gfxImageSurface to gfx::DataSourceSurface et al;

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

Looks good to me

Comment 17

[Nicolas Silva [:nical]]

Let's see if it builds everywhere: https://tbpl.mozilla.org/?tree=Try&rev=89359465b058

Comment 18

[Nicolas Silva [:nical]]

Ah, B2G is busted, you need to also convert the Texture classes in GrallocTextureHost.h with that it should be good.

Comment 19

[Andreas Pehrson [:pehrsons]]

Attachment: Change gfxImageSurface to gfx::DataSourceSurface in TextureHost et al

Comment 20

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8338382 [details] [diff] [review]
Change gfxImageSurface to gfx::DataSourceSurface in TextureHost et al

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

I fixed GrallocTextureHost and compiled it for B2G. Worked fine on my setup!

Comment 21

[Andreas Pehrson [:pehrsons]]

Eh, did I just post a review of my own patch? Oh well. #newtobugzilla

Comment 22

[Nicolas Silva [:nical]]

Comment on attachment 8338382 [details] [diff] [review]
Change gfxImageSurface to gfx::DataSourceSurface in TextureHost et al

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

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +324,5 @@
> +                                                : nullptr;
> +    surf = Factory::CreateWrappingDataSourceSurface(image->Data(),
> +                                                    image->Stride(),
> +                                                    gfx::ToIntSize(image->GetSize()),
> +                                                    gfx::ImageFormatToSurfaceFormat(image->Format()));

Woops! This is not correct and I didn't notice it in my previous review (sorry). What happens here is that you create a gfxImageSurface that owns the image data, and you wrap that into a DataSourceSurface, but at the end of the scope the image data is deallocated along with the gfxImageSurface and the returned DataSourceSurface is then wraping uninitialized data.

I think the easiest way to fix that is to add an equivalent of GLContext::GetTexImage that returns a DataSourceSurface instead of a gfxImageSurface (filing a bug for it).

Comment 23

[Andreas Pehrson [:pehrsons]]

Attachment: Change gfxImageSurface to gfx::DataSourceSurface in TextureHost et al

Comment 24

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8339164 [details] [diff] [review]
Change gfxImageSurface to gfx::DataSourceSurface in TextureHost et al

nical, could you push this and 943293 to try?
That should then be ready to go.

Comment 25

[Nicolas Silva [:nical]]

try push (including patches from bug 943293) https://tbpl.mozilla.org/?tree=Try&rev=6763d0d165c7

Comment 26

[Andreas Pehrson [:pehrsons]]

Nicolas, could you have another go with the latest in 943293?

Comment 27

[Nicolas Silva [:nical]]

there you go https://tbpl.mozilla.org/?tree=Try&rev=1f6655ad1703

Comment 28

[Andreas Pehrson [:pehrsons]]

Attachment: Change gfxImageSurface to gfx::DataSourceSurface in TextureHost et al

Confirmed compiling for B2G now.

Comment 29

[Andreas Pehrson [:pehrsons]]

This poor fella needs a rebase now. Looking into it.

Comment 30

[Nicolas Silva [:nical]]

(In reply to Andreas Pehrson [:pehrsons] from comment #29)
> This poor fella needs a rebase now. Looking into it.

Don't worry, i did a quick fixup and rebase and I am about to land your patch queue

Comment 31

[Nicolas Silva [:nical]]

Comment on attachment 8339833 [details] [diff] [review]
Change gfxImageSurface to gfx::DataSourceSurface in TextureHost et al

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

I locally fixed up the "GLContextUtils::" and turned an #include into a forward declaration.

Comment 32

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6ac02b04f53

Comment 33

[Andreas Pehrson [:pehrsons]]

nical, why return nullptr in BasicCompositor::GetAsSurface?

I was also looking at the cause of the rebase (942318). See the diff: https://bugzilla.mozilla.org/attachment.cgi?id=8338057&action=diff on line 107. What happens when surface goes out of scope? Doesn't it get killed, making the data in mSurface invalid?

Looks to me just like the bug I had in the first version of my patch here =)

Comment 34

[Nicolas Silva [:nical]]

(In reply to Andreas Pehrson [:pehrsons] from comment #33)
> nical, why return nullptr in BasicCompositor::GetAsSurface?

While rebasing I saw that the gfxImageSurface version of the code was returning null. I was not sure whether this was a change of behavior that caused the rebase, or if you had added the functionality (I should have checked actually) and went the lazy way. We should fix that as a followup (and there are also a lot of other places where we currently return null out of laziness, so we could fix those too).

> 
> I was also looking at the cause of the rebase (942318). See the diff:
> https://bugzilla.mozilla.org/attachment.cgi?id=8338057&action=diff on line
> 107. What happens when surface goes out of scope? Doesn't it get killed,
> making the data in mSurface invalid?
> 
> Looks to me just like the bug I had in the first version of my patch here =)

In this case it is ok because the gfxImageSurface is also created as a wrapper around the data that is in the SurfaceDescriptor, so when the gfxImageSurface goes out of scope, it doesn't deallocate the buffer (mOwnsData in gfxImageSurface is false in this case). This code is a bit confusing and to answer your question I had to go check the code in a bunch of different places to be sure about who owned the data.

Comment 35

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Nicolas Silva [:nical] from comment #34)
> (In reply to Andreas Pehrson [:pehrsons] from comment #33)
> > nical, why return nullptr in BasicCompositor::GetAsSurface?
> 
> While rebasing I saw that the gfxImageSurface version of the code was
> returning null. I was not sure whether this was a change of behavior that
> caused the rebase, or if you had added the functionality (I should have
> checked actually) and went the lazy way. We should fix that as a followup
> (and there are also a lot of other places where we currently return null out
> of laziness, so we could fix those too).

Given that we're in the last week before trains change, can we resolve this before landing, instead of as a follow up?  Or at least understand the reasoning and intent behind this.  The "I'm not sure" part is worrying.  This is the "feedback-" reason fwiw.

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c6ac02b04f53

Comment 37

[Nicolas Silva [:nical]]

(In reply to Milan Sreckovic [:milan] from comment #35)
> Given that we're in the last week before trains change, can we resolve this
> before landing, instead of as a follow up?  Or at least understand the
> reasoning and intent behind this.  The "I'm not sure" part is worrying. 
> This is the "feedback-" reason fwiw.

I was not sure so I took the safest route (the one that changes least the original code: GetAsSurface was unimplemented and I left it unimplemented instead of adding an implementation). The code in question (GetAsSurface) is only used as a debug facility so it is pretty safe safe anyways.