877115 - Moz2Dify GLContext and GLTextureImage

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nicolas Silva [:nical]]

It would be handy that TextureImage and GLContext work with the Moz2D classes. Right now they use thebes for sizes, texture uploads, etc. which makes it very tempting to only use thebes in code that uses TextureImage (like layers), rather than using the new shiny Moz2D.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Moz2Dify GLContext and GLTextureImage

I am thinking of something like that in the shot term: methods that just wrap the thebes related stuff in GLContext/textureImage's, with a gfx2D signature instead and calls in the thebes implem internally.

At some point we'll have to take the time to actually purge the thebes stuff out of GLContext, but it'll take time to fix all the code that depends on it.

Comment 2

[Nicolas Silva [:nical]]

Comment on attachment 755306 [details] [diff] [review]
Moz2Dify GLContext and GLTextureImage

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

::: gfx/gl/GLContext.h
@@ +885,5 @@
>                                               const nsIntPoint& aSrcPoint = nsIntPoint(0, 0),
>                                               bool aPixelBuffer = false,
>                                               GLenum aTextureUnit = LOCAL_GL_TEXTURE0);
> +    ShaderProgramType UploadSurfaceToTexture(gfx::DataSourceSurface* aSurface,
> +                                             const gfx::IntRect& aDstRegion,

err, actually I meant nsIntRegion here

::: gfx/gl/GLTextureImage.h
@@ +171,5 @@
>       */
>      virtual bool DirectUpdate(gfxASurface *aSurf, const nsIntRegion& aRegion, const nsIntPoint& aFrom = nsIntPoint(0,0)) = 0;
> +    // Moz2D equivalent
> +    bool UpdateFromDataSource(gfx::DataSourceSurface *aSurf,
> +                              const gfx::IntRect& aRect,

nsIntRegion here instead of IntRect

Comment 3

[Kelsey Gilbert [:jgilbert]]

(In reply to Nicolas Silva [:nical] from comment #1)
> Created attachment 755306 [details] [diff] [review]
> Moz2Dify GLContext and GLTextureImage
> 
> I am thinking of something like that in the shot term: methods that just
> wrap the thebes related stuff in GLContext/textureImage's, with a gfx2D
> signature instead and calls in the thebes implem internally.
> 
> At some point we'll have to take the time to actually purge the thebes stuff
> out of GLContext, but it'll take time to fix all the code that depends on it.

Why not do what we can now? I don't see the reason for keeping the functions you touch here in GLContext.

Comment 4

[Nicolas Silva [:nical]]

It's mostly a matter of how much I am willing to be side-tracked from the TextureClient/Host stuff. I really want to get the Texture work done asap so that the people pending on it don't wait for too long, but at the same time it needs at least an equivalent to TextureImage::DirectUpdate that uses a DataSourceSurface rather than a gfxASurface.

I can commit to remove the thebes stuff from TextureImage entirely after the TextureClient/Host work.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 755306 [details] [diff] [review]
Moz2Dify GLContext and GLTextureImage

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

I will hold you to this. :)

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 755306 [details] [diff] [review]
Moz2Dify GLContext and GLTextureImage

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

::: gfx/gl/GLContext.cpp
@@ +1932,5 @@
>    data += aPoint.x * gfxASurface::BytePerPixelFromFormat(aSurf->Format());
>    return data;
>  }
>  
> +ShaderProgramType 

Trailing whitespace

::: gfx/gl/GLTextureImage.h
@@ +20,5 @@
> +class DataSourceSurface;
> +}
> +}
> +
> +namespace mozilla {

Merge the two mozilla blocks

Comment 7

[Adrian May [:adrianmay]]

I'd like to provisionaly grab this and see what I can do. I'd be aiming for a proper conversion, rather than more wrappers (which just piles up work for the future,) so I'd appreciate any hints as to what order to approach that in, i.e., how can I break it up into smaller jobs starting with those where the client code will give the least aggro?

Comment 8

[Nicolas Silva [:nical]]

(In reply to Adrian May [:adrianmay] from comment #7)
> I'd like to provisionaly grab this and see what I can do. I'd be aiming for
> a proper conversion, rather than more wrappers (which just piles up work for
> the future,) so I'd appreciate any hints as to what order to approach that
> in, i.e., how can I break it up into smaller jobs starting with those where
> the client code will give the least aggro?

Great!

So basically we want to take all the APIs in GLContext and TextureImage that use Thebes classes (such as gfxImageSurface, gfxIntSize, gfxImageFormat...) and convert them into using Moz2D classes (gfx::DataSourceSurface, gfx::IntSize, gfx::SurfaceFormat...).

Thebes stuff lives here: http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/
Moz2D stuff here: http://mxr.mozilla.org/mozilla-central/source/gfx/2d/

In any case, we will need the Thebes-style APIs to stick around for a while in GLContext and TextureImage, because I suppose they have a lot of users a bit of everywhere and you don't want to convert users of GLContext into using Moz2D all at once.

You could take one of the Thebes-based methods, convert it into using Moz2D, and then wrap this method into an adapter for thebes so that e keep the thebes interface but then it is easier to get rid of it.

like, say, TextureImage::UploadSurfaceToTexture:
Right now it is implemented in term of Thebes and wrapped into a Moz2D adapter method, you could invert this by implementing it in term of Moz2D and wrap it with a Thebes.

Maybe doing this will require that you convert other methods of TextureImage or GLContext. You will find out along the way. If so, try to do them in separate patches to ease reviews and make it not not painful for you to rebase if other people touch surrounding code before you get to land your work.

Don't hesitate to ask a lot of questions on this bug, by email, or on irc.mozilla.org on the #gfx channel.

Comment 9

[Adrian May [:adrianmay]]

Attachment: Moz2DWrappers_1.patch

OK, here's a start. But there are problems. 

GetUpdateRegion is blocked because I can't find a Moz2D equivalent of nsIntRegion, i.e. something with lots of rectangles in. Maybe I'm just blind.

BeginUpdate and GetBackingSurface have bigger problems. They return gfxASurface. That has a whole zoo of derived classes which can all be returned in these positions, but don't all seem to have equivalents in Moz2D. Even if they did, there'd be a lot of work to clone the old types into the new ones.

Advice?

Comment 10

[Nicolas Silva [:nical]]

Comment on attachment 787408 [details] [diff] [review]
Moz2DWrappers_1.patch

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

Awesome, I am glad to see you are already getting started!

::: gfx/gl/GLTextureImage.cpp
@@ +644,5 @@
>  
> +
> +// Moz2D equivalents...
> +const gfx::IntSize& TextureImage::GetSizeMoz2D() const { 
> +  return ToIntSize(mSize); 

nit: please remove the trailing white spaces.

::: gfx/gl/GLTextureImage.h
@@ +144,5 @@
>      virtual nsIntRect GetTileRect() {
>          return nsIntRect(nsIntPoint(0,0), mSize);
>      }
> +    // Moz2D equivalent...
> +    gfx::IntRect GetTileRectMoz2D();

See my comment about GetSizeMoz2D

@@ +242,5 @@
>      { return nullptr; }
>  
>      const nsIntSize& GetSize() const { return mSize; }
> +    // Moz2D equivalent...
> +    const gfx::IntSize& GetSizeMoz2D() const;

I hadn't really thought about the case of return types (sorry), but actually I am not sure it is worth adding a new method name for the cases where we are converting the method name. It makes the user go from

gfx::IntSize size = gfx::ToIntSize(image->GetSize());

to

gfx::IntSize size = image->GetSizeMoz2D();

While ultimately we want to just call

gfx::IntSize size = image->GetSize();

I am not sure it is worth adding temporary method names in this case since the callers will have to be rewritten as we advance the Moz2Dification.
What you did for method parameters is definitely good, though.

It would be interesting to see how much foreign code you would need to modify to convert GetSize() to just return a gfx::IntSize (the modification would just be wrapping the caller into gfx::NsIntSize(). That could be worth doing in a separate patch.

I realize I may be contradicting myself (sorry about that) because I said earlier that you don't want to convert all the users of TextureImage to Moz2D at once, but in the case of a simple method like GetSize it is much easier than in the case of stuff that manipulates gfxImageSurfaces for instance.

@@ +283,5 @@
>      virtual nsIntRect GetSrcTileRect() {
>          return nsIntRect(nsIntPoint(0,0), mSize);
>      }
> +    // Moz2D equivalent...
> +    gfx::IntRect GetSrcTileRectMoz2D(); 

nit: please remove the trailing white spaces.
And same comment as for GetSizeMoz2D.

Comment 11

[Nicolas Silva [:nical]]

Comment on attachment 787408 [details] [diff] [review]
Moz2DWrappers_1.patch

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

If you can reupload a patch without the XxxMoz2D() methods, i'll r+ it.
Cancelling the review of this patch in the mean time (usually some people use r- in this case but it tends to scare new contributors because of the "negative" notion, there is stuff in your patch that we want to keep).

Comment 12

[Nicolas Silva [:nical]]

Moz2D doesn't have an equivalent of nsIntRegion yet, so we will keep nsIntRegion arounf for a while.

Comment 13

[Adrian May [:adrianmay]]

OK, I'll zap the XxxMoz2D methods and take a look at converting the client code for IntRect and IntSize, It should be a no-brainer, but perhaps big. I guess we won't touch the gfxASurface stuff?

Comment 14

[Adrian May [:adrianmay]]

Attachment: Moz2DWrappers_2.patch

Here it is without the XxxMoz2D calls or trailing spaces.

Comment 15

[Nicolas Silva [:nical]]

Comment on attachment 787445 [details] [diff] [review]
Moz2DWrappers_2.patch

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

cool, I'll land this today

Comment 16

[Nicolas Silva [:nical]]

(In reply to Adrian May [:adrianmay] from comment #13)
> OK, I'll zap the XxxMoz2D methods and take a look at converting the client
> code for IntRect and IntSize, It should be a no-brainer, but perhaps big. I
> guess we won't touch the gfxASurface stuff?

Yes, my advice is try to do one patch just for GetSize() first, then another one for, say, GetTileRect(), etc. When we are done with the easier stuff we can look at how we can tackle gfxASurface which will most likely be a bit more involved.

As you convert call sites of some of the methods you will probably see that some users of the methods are already using gfx::IntSize and converting to thebes so in some cases you will just remove the adapter glue code, which is nice.

Comment 17

[Adrian May [:adrianmay]]

So are nsIntSize and gfxIntSize the same thing? I got an error for overloading a function between those two.

Comment 18

[Nicolas Silva [:nical]]

(In reply to Adrian May [:adrianmay] from comment #17)
> So are nsIntSize and gfxIntSize the same thing? I got an error for
> overloading a function between those two.

Apparently so. http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPoint.h#17

Comment 19

[Adrian May [:adrianmay]]

Attachment: Moz2DWrappersGetSize.patch

Changed TextureImage::GetSize to return gfx::IntSize and fixed the clients by including the glue and wrapping ThebesIntSize around the returned gfx::IntSize.

This sits on top of Moz2DWrappers_2.patch. 

I'll plough through the similar cases now and send individual patches.

Comment 20

[Adrian May [:adrianmay]]

Attachment: Moz2DWrappersTileRect.patch

Here's another chunk for the TileRect stuff. Sits on top of Moz2DWrappersGetSize.patch and tidies up some stuff I messed earlier.

Comment 21

[Adrian May [:adrianmay]]

Attachment: Moz2DWrappersMisc.patch

I think this just about wraps it up for GLTextureImage. Note the removal of the & in GetSize. Sits on top of Moz2DWrappersTileRect.patch.

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 787954 [details] [diff] [review]
Moz2DWrappersGetSize.patch

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

::: gfx/gl/GLTextureImage.cpp
@@ +648,5 @@
>    Resize(NsIntSize(aSize));
>  }
>  
> +const gfx::IntSize& TextureImage::GetSize() const { 
> +  return ToIntSize(mSize); 

Please fix trailing whitespace

Comment 23

[Adrian May [:adrianmay]]

Attachment: Moz2DWrappersMisc.patch

Whitespace fixed.

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 788024 [details] [diff] [review]
Moz2DWrappersTileRect.patch

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

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +182,1 @@
>        return gfx::IntSize(rect.width, rect.height);

Could be "return mTexImage->GetTileRect().Size();"

@@ +182,3 @@
>        return gfx::IntSize(rect.width, rect.height);
>      }
>      return gfx::IntSize(mTexImage->GetSize().width, mTexImage->GetSize().height);

And you could have made this "return mTexImage->GetSize();" in the previous patch.

@@ +367,3 @@
>        return gfx::IntSize(rect.width, rect.height);
>      }
>      return gfx::IntSize(mTexture->GetSize().width, mTexture->GetSize().height);

Ditto

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +235,5 @@
>  
>      bool usingTiles = (mTexImage->GetTileCount() > 1);
>      do {
>        if (mTexImageOnWhite) {
> +        NS_ASSERTION(ThebesIntRect(mTexImageOnWhite->GetTileRect()) == ThebesIntRect(mTexImage->GetTileRect()), "component alpha textures should be the same size.");

Is this necessary?

Comment 25

[Adrian May [:adrianmay]]

Attachment: Moz2DWrappersMisc.patch

Fixed. I see what you meant about existing wrappers now.

The assert did indeed need massaging. Otherwise it complained about == being private in something, I forget what. Try it and see. It was interesting.

Comment 26

[Nicolas Silva [:nical]]

Comment on attachment 787954 [details] [diff] [review]
Moz2DWrappersGetSize.patch

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

r=me with white spaces fixed

Comment 27

[Adrian May [:adrianmay]]

Hi Nicolas,

> r=me with white spaces fixed

Sorry, I'm a bit confused. Are you asking me to change something or satisfied with the current patch?

Comment 28

[Nicolas Silva [:nical]]

Comment on attachment 788024 [details] [diff] [review]
Moz2DWrappersTileRect.patch

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

I agree with Ms2ger's comments, r=me with his comments addressed (except for the assert change since it is needed).

Comment 29

[Nicolas Silva [:nical]]

Comment on attachment 787954 [details] [diff] [review]
Moz2DWrappersGetSize.patch

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

And please add the two changes Ms2ger mentioned in the other patch here, about returning mTexImage->GetSize() directly in TextureImageTextureSourceOGL::GetSize()
and TextureImageDeprecatedTextureHostOGL::GetSize()

Comment 30

[Nicolas Silva [:nical]]

(In reply to Adrian May [:adrianmay] from comment #27)
> Hi Nicolas,
> 
> > r=me with white spaces fixed
> 
> Sorry, I'm a bit confused. Are you asking me to change something or
> satisfied with the current patch?

The patch doesn't have any outstanding logic problem so I "r+ed" it but I would like you to upload a new version with white spaces fixed and that makes GetSize look nice in TextureImageTextureSourceOGL. (since those are just small changes I wouldn't r- the patch. so r+ in advance if you make small changes).

Since you  will be uploading new versions of the patches bugzilla will put them out of order, please add "Part X -" (or anything with a number of a letter to indicate the order) in the title of the new patches so that we can easily tell the order.

And thanks again for submitting patches so quickly

Comment 31

[Adrian May [:adrianmay]]

OK, I'll note the "Part X" thing for the future. In this instance, the 4 patches all follow on from each other in the order that they're posted above. I obsoleted anything else.

I believe I fixed everything Ms2ger asked for by modifying the final patch. I realise that's a bit naff cos you'd have to land dodgy patches and then fix them with the final one, so you can only do them all at once, which is bad. 

But right now I'm not sure how to fix that in my hg q. I could qpop to the original problems and fix them, but then the subsequent patches would be baseless, or would they?

So is it OK right now or do you need more changes from me?

Comment 32

[Nicolas Silva [:nical]]

Comment on attachment 788107 [details] [diff] [review]
Moz2DWrappersMisc.patch

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

Oh okay I hadn't seen you'd addressed the previous comments in a followup patch.
Usually it's better to do it in the patches themselves but in this case since all the patch do roughly the same stuff and are small it's okay to do it as a follow up.
If you have already fixed your previous patch we'll take it, otherwise I'll fold them. Sorry for the contradictory information.
For the next patches let's modify the patches themselves when addressing comments rather than doing followups. If you are not using it, mercurial queues is an hg extension that helps a lot with this kind of workflows.

Same here, r=me with comments below addressed.

::: gfx/gl/GLTextureImage.h
@@ +236,5 @@
>       */
>      virtual already_AddRefed<gfxASurface> GetBackingSurface()
>      { return nullptr; }
>  
> +    const gfx::IntSize GetSize() const;

Since you return by value you don't need to make the returned value const.

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +619,5 @@
>      SurfaceStream* surfStream = SurfaceStream::FromHandle(streamDesc.handle());
>      if (surfStream) {
>        mStreamGL = dont_AddRef(surfStream->GLContext());
>      }
> +  }

Let's not fix the white spaces outside of the scope of the patch. It's a bit frustrating but it makes the hg history easier to navigate.

Comment 33

[Nicolas Silva [:nical]]

Also, when uploading a modified version of a patch that has been r+ed already, just add "(carries r=nical)" in the patch title, non need to ask for another review if the changes aren't big.

Comment 34

[Adrian May [:adrianmay]]

Attachment: Moz2DWrappersMisc.patch

Here then, with the other trailing whitespace back in, the const removed, and the endorsement in the patch title.

Comment 35

[Adrian May [:adrianmay]]

BTW, I am using hg queues, but I still don't know how to modify an earlier patch without the later ones losing their base, i.e. trying to patch files that aren't the same anymore. Or is it clever enough to rebase the later patches automatically?

Comment 36

[Adrian May [:adrianmay]]

Attachment: Moz2DWrappersMisc.patch

Took the const out of the cpp as well. Duh!

Comment 37

[Nicolas Silva [:nical]]

Attachment: Moz2DWrappers rebased and folded (carries r=nical)

Rebased and folded the patch queue into one patch.
pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=32813127cb6b

I'll land as soon as try is green.

Comment 38

[Nicolas Silva [:nical]]

(In reply to Adrian May [:adrianmay] from comment #35)
> BTW, I am using hg queues, but I still don't know how to modify an earlier
> patch without the later ones losing their base, i.e. trying to patch files
> that aren't the same anymore. Or is it clever enough to rebase the later
> patches automatically?

To modify an earlier patch you need to come back to that patch ("hg qpop") and once you are done modifying and you have "hg qrefresh"ed it, "hg qpush" back to the later ones and solve the eventual conflicts.

You have to rebase your patch queue every now and then because mozilla-central changes while you are writing your code, so you at least need to rebase before landing.

The more often you do it the easiest it is to rebase, but it always depend on how complex the patch is. For instance it took me just a few minutes to rebase your patch queue.

I would not say mq is very clever about rebasing, (basically it will not try anything fancy if the code has changed within a few lines of the code you modified) but it does the job and if the patch is small enough it is usally trivial to fix the conflicts.

Comment 39

[Adrian May [:adrianmay]]

OK thanks for the tute. I'll experiment with that in some sandbox. In the meantime I'm busy with something for the Android crew but I'll be back over here soon enough, probably finishing off 898436.

Comment 40

[Nicolas Silva [:nical]]

fixed the build and landed
https://hg.mozilla.org/integration/mozilla-inbound/rev/32c7fa71f624

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/32c7fa71f624

Comment 42

[Nicolas Silva [:nical]]

It'd be great to move the methods in GLContext that we are converting to Moz2D into a separate file and make them not methods of GLContext at all.

Say, GLUtils.h/.cpp

for example:

already_AddRefed<gfxImageSurface> mozilla::gl::GLContext::GetTexImage(GLuint aTexture, bool aYInvert, SurfaceFormat aFormat);

Would become

TemporaryRef<mozilla::gfx::DataSourceSurface> mozilla::gl::ReadBack(GLuint aTexture, bool aYInvert, SurfaceFormat aFormat)

Doing that would also help slim down GLContext that's become bloated.

Comment 43

[Andreas Gal :gal]

Since we are already hacking around on GLContext, would it make sense to put the current context into TLS, since the driver portion of it already is anyway? Ref-counting the GLContext and keeping a million references in destructors to it seems to cause a trail of tears and shutdown issues.

Comment 44

[Nicolas Silva [:nical]]

(In reply to Andreas Gal :gal from comment #43)
> Since we are already hacking around on GLContext, would it make sense to put
> the current context into TLS, since the driver portion of it already is
> anyway? Ref-counting the GLContext and keeping a million references in
> destructors to it seems to cause a trail of tears and shutdown issues.

Do you mean keeping track of the last context that was made current to avoid the cost of MakeCurrent()? If so my understanding is that we can't do this because of something like plugins that are not out of process may be doing opengl stuff with their own contexts and we can't track that.

Comment 45

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

Regarding putting the current context in TLS, see bug 749678, where this was proposed in an attempt to optimize away redundant MakeCurrent calls and make GetCurrentContext cheap. This didn't land because on most platforms we couldn't easily ensure that we were the only ones talking to OpenGL on the main thread; but this could definitely land on Firefox OS if it's useful there; and recently in a IRC conversation Vlad suggested that we could actually make this safe everywhere by patching {egl,wgl,etc}MakeCurrent in memory directly with our TLS instrumentation.

What issues are caused by GLContext being reference-counted?

Notice that, like many things around gfx, a GLContext can have to be referenced by various things belonging to different 'world' each with their own lifetimes. For example, a GLContext may have to be referenced by a Layer, whose lifetime is tied to the IPC system, and it could also have to be referenced by a canvas rendering context, whose lifetime is tied to garbage collection. Given that, it is difficult to imagine an alternative to reference-counting GLContexts.

Comment 46

[Andreas Gal :gal]

That doesn't make sense to me. A GLContext has a specific lifetime. If I make a canvas, the GLContext should live as long as that canvas lives. Nobody should ever get access to that context, except to render to it. Nobody should render to it once it was destroyed. Similarly for layers.

Comment 47

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

Well, that was a bad example, sorry. I was thinking of surfaces, not GL contexts. Let me try again.

Suppose that we start multiplexing GLContext's. Multiplexing OpenGL contexts is something that we want to start playing with soon for Skia/GL; doing that multiplexing at the level of GLContext is a reasonable thing to want to do eventually so that more things can share a GLContext (e.g. WebGL contexts). When we get there, we'll have multiple things holding on to a GLContext, and they'll have unrelated lifetimes (e.g. canvas rendering contexts exposed to javascript).

Speaking of Skia/GL, that was another good use case for refcounted GLContext, as we had many pieces of code trying to reference that GLContext (the CanvasRenderingContext2D, the DrawTargetSkia, Skia internals) and being able to rely on refcounting GLContext allowed for simpler, easier-to-review code that doesn't crash.

More generically, GLContexts are just tools to talk to the GPU; I don't see a strong reason to want them to have single owners. They should be accessed from only one thread, but it should be OK for them to be used by many things, as long as these things are on the same thread.

Comment 48

[Andreas Gal :gal]

Thats an even stronger reason to not ref-count GLContext. Instead we should have 1 GLContext per thread. All access can be scoped to ensure exclusive access and we can lazily spin up the GLContext when a thread accesses it and spin it down on a timeout or shutdown (again, access is always scoped, so doing so from the event loop is safe).

Comment 49

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

Maybe, that solution (absolute multiplexing, only 1 GLContext per thread) will be possible; that would actually be pretty cool; I had actually not thought of going that far; I can't tell out of hand whether it's going to be actually possible or not, especially if we are talking about doing it everywhere on all platforms (otherwise, it's not removing the need for refcounting). Things to watch out for will include:

 1. Context loss. We'd now lose all GLContext's as soon as we lose one, e.g. when running a 'nasty' WebGL script. That would be a regression on OSes/drivers where per-context loss works reliably, which is the case of e.g. recent NVIDIA desktop drivers.

 2. Possible performance implications. Part of why I don't know yet if we'll want to multiplex WebGL's GLContexts everywhere (on mobile, we'll probably want to, for the memory savings, but that is less critical on desktop) is that I don't know yet what the performance implications will be. In theory it should not be bad, but I don't know what driver-side performance caveats this not-quite-usual usage pattern of OpenGL will run into. For the more special patterns used by Skia/GL, we already know that Google is using multiplexing there, so it must be working well enough for that, at least on mobile GPUs.

 3. Security implications, which we may or may not care about. In typical recent desktop GPUs (I don't know about mobile), there is an actual MMU, and at least one major GPU vendor said that each OpenGL context gets a separate address space. That means that we get some isolation from using actually separate OpenGL contexts. That might be considered worthwhile e.g. for WebGL on desktop (while security matters less outside of WebGL, and on mobile the memory savings from multiplexing might outweigh these somewhat abstract security considerations).

Comment 50

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

Also, notice that experimenting with GLContext multiplexing is blocked on bug 901498 which will make it possible to implement GLContext multiplexing without having to write special code paths for all the extraneous code that is currently built into GLContext.

Comment 51

[Andreas Gal :gal]

(In reply to Benoit Jacob [:bjacob] from comment #49)
> Maybe, that solution (absolute multiplexing, only 1 GLContext per thread)
> will be possible; that would actually be pretty cool; I had actually not
> thought of going that far; I can't tell out of hand whether it's going to be
> actually possible or not, especially if we are talking about doing it
> everywhere on all platforms (otherwise, it's not removing the need for
> refcounting). Things to watch out for will include:
> 
>  1. Context loss. We'd now lose all GLContext's as soon as we lose one, e.g.
> when running a 'nasty' WebGL script. That would be a regression on
> OSes/drivers where per-context loss works reliably, which is the case of
> e.g. recent NVIDIA desktop drivers.

This is probably tolerable, especially since we are moving to process separation anyway. In case of a context loss we recreate it and layers shouldn't be affected.

> 
>  2. Possible performance implications. Part of why I don't know yet if we'll
> want to multiplex WebGL's GLContexts everywhere (on mobile, we'll probably
> want to, for the memory savings, but that is less critical on desktop) is
> that I don't know yet what the performance implications will be. In theory
> it should not be bad, but I don't know what driver-side performance caveats
> this not-quite-usual usage pattern of OpenGL will run into. For the more
> special patterns used by Skia/GL, we already know that Google is using
> multiplexing there, so it must be working well enough for that, at least on
> mobile GPUs.

I would worry about this once we run into it. The simplification of ripping out GLContext reference is significant. If that ever shows to be bad for perf we can use 1 per type (1 per thread per WebGL use, one per content use, etc). That should fix any performance issues, even if we get them, and still be much easier to reason about.

> 
>  3. Security implications, which we may or may not care about. In typical
> recent desktop GPUs (I don't know about mobile), there is an actual MMU, and
> at least one major GPU vendor said that each OpenGL context gets a separate
> address space. That means that we get some isolation from using actually
> separate OpenGL contexts. That might be considered worthwhile e.g. for WebGL
> on desktop (while security matters less outside of WebGL, and on mobile the
> memory savings from multiplexing might outweigh these somewhat abstract
> security considerations).

Process separation will yield this benefit. As long you have one CPU address space, OpenGL addresses spaces don't matter IMO.

I am currently debugging a GLContext shutdown issue, and thats **** me off enough that I will volunteer for this work once my shader cleanup patch actually has landed.

Comment 52

[Andreas Gal :gal]

(Agree on the dependency, this will need a long list of dependencies before we can move on the actually 1-GLContext switch.)

Comment 53

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

(In reply to Andreas Gal :gal from comment #51)
> >  3. Security implications, which we may or may not care about. In typical
> > recent desktop GPUs (I don't know about mobile), there is an actual MMU, and
> > at least one major GPU vendor said that each OpenGL context gets a separate
> > address space. That means that we get some isolation from using actually
> > separate OpenGL contexts. That might be considered worthwhile e.g. for WebGL
> > on desktop (while security matters less outside of WebGL, and on mobile the
> > memory savings from multiplexing might outweigh these somewhat abstract
> > security considerations).
> 
> Process separation will yield this benefit.

Process separation will only fully yield this benefit if we have iframes of different origins run in different processes. I haven't been following electrolysis closely, so I don't know whether that is part of the current plan.

> As long you have one CPU address
> space, OpenGL addresses spaces don't matter IMO.

On way that GPU-side separate address spaces are potentially interesting even in a single-process browser, is that we arguably trust opaque vendor GPU-side code less than we trust our own CPU-side code. (Of course, there also is the separate problem of bugs in CPU-side driver code).

Comment 54

[Andreas Gal :gal]

As long the CPU address space is shared, the GPU address space simply doesn't matter. Just about 0 exploits have been shown attacking via a GPU address space vector. Thousands have been shown attacking via CPU address space. I am comfortable going with the historic numbers here.

Comment 55

[Andreas Pehrson [:pehrsons]]

I have been looking at Moz2Difying the GLContext a bit now. I'll submit some patches this week.

adrianmay: if you are still on top of this, let me know :)

Comment 56

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 1. Remove ImageFormat typedef from GLContext.

Comment 57

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 2. Moz2Dify GLContext::ResizeOffscreen signature.

Comment 58

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

Comment 59

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 4. Change gfxIntSize to gfx::IntSize in GLScreenBuffer and friends.

Comment 60

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 5. Minimize gfxIntSize usage in GLContext.

Comment 61

[Andreas Pehrson [:pehrsons]]

bzexport didn't set the reviewer properly from the summary apparently..

Changes are mostly to gfxIntSize. Should be straight forward.

Comment 62

[Andreas Pehrson [:pehrsons]]

Will need a run through try as well. I have only tried compiling for mac and B2G.

Comment 63

[Nicolas Silva [:nical]]

Comment on attachment 8345317 [details] [diff] [review]
[Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

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

r=me with the comment below added

::: gfx/layers/CopyableCanvasLayer.cpp
@@ +130,5 @@
> +      RefPtr<DataSourceSurface> readDSurf = sharedSurf_Basic->GetData();
> +      readSurf = new gfxImageSurface(readDSurf->GetData(),
> +                                     ThebesIntSize(readDSurf->GetSize()),
> +                                     readDSurf->Stride(),
> +                                     SurfaceFormatToImageFormat(readDSurf->GetFormat()));

Here we need to make sure that readSurf never outlives sharedSurf_Basic->mData. Please add a comment here stressing that (in particular we should be careful not to return it), and as a security, please also declare RefPtr<DataSourceSurface> readDSurf next to readSurf's declaration, so that the lifetime constraint is visible and explicit in this code.

Comment 64

[Nicolas Silva [:nical]]

Ouch! It looks like this stuff collided badly with the GLContext modularization work :( could you rebase again, please? A lot of stuff has moved away from GLContext.h into other files but the logic should not have changed much, so if the rebase goes smoothly and it builds no need for another round of reviews, I'll push to try right away.

Comment 65

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 1. Remove ImageFormat typedef from GLContext. (carries r=nical)

Comment 66

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 2. Moz2Dify GLContext::ResizeOffscreen signature. (carries r=nical)

Comment 67

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes. (carries r=nical)

Fixed according to your comments.

Comment 68

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 4. Change gfxIntSize to gfx::IntSize in GLScreenBuffer and friends. (carries r=nical)

Comment 69

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 5. Minimize gfxIntSize usage in GLContext. (carries r=nical)

Comment 70

[Andreas Pehrson [:pehrsons]]

nical, there you go, freshly rebased and fixed.

I tried the carries r= magic both through bzexport and manually but no go there. Any hints?
bzexport also frequently complains about not being able to set the assignee. Do you mind? =)

Comment 71

[Nicolas Silva [:nical]]

(In reply to Andreas Pehrson [:pehrsons] from comment #70)
> nical, there you go, freshly rebased and fixed.
> 
> I tried the carries r= magic both through bzexport and manually but no go
> there. Any hints?

I don't use bzexport so I don't know what problem you are referring to. What's wrong with your patches? carries r= is in the title so it looks good to me.

(In reply to Nicolas Silva [:nical] from comment #64)
> Ouch! It looks like this stuff collided badly with the GLContext
> modularization work :( could you rebase again, please?

Err sorry, I got mixed up and thought I had to apply the first patch too (the one written by Adrian) which has already landed so it collided massively with itself. I could have probably applied your patches without asking you to rebase.

Comment 72

[Nicolas Silva [:nical]]

try push https://tbpl.mozilla.org/?tree=Try&rev=4e34e0d2e1c1

Comment 73

[Andreas Pehrson [:pehrsons]]

I thought carries would make bugzilla keep the r+ but maybe that's wrong. Can't see it documented anywhere.

Comment 74

[Nicolas Silva [:nical]]

(In reply to Andreas Pehrson [:pehrsons] from comment #73)
> I thought carries would make bugzilla keep the r+ but maybe that's wrong.
> Can't see it documented anywhere.

I don't think we have that magic, so we put "carries r=" in the patch title when uploading to bugzilla. It's better if you just put "r=" at the end of the hg commit message though.
Looks like you have a few build targets to fix :)

Comment 75

[Andreas Pehrson [:pehrsons]]

Ah. Ohwell.

I set up a windows environment to make my future life easier, so fixing this has taken some time. Should be in by tomorrow if most things compile over night!

Comment 76

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

I'll apply your patches locally and work on top of them, so that I don't bitrot you as I continue with GLContext cleanup.

Comment 77

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

Comment 78

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

Comment 79

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

Comment 80

[Andreas Pehrson [:pehrsons]]

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

Comment 81

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8347182 [details] [diff] [review]
[Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

Fixed the windows errors now. Could you please double check that I am using moz2d correctly in CanvasLayerD3D*::UpdateSurface?

Cheers!

Comment 82

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8345916 [details] [diff] [review]
[Moz2Dify] Part 5. Minimize gfxIntSize usage in GLContext. (carries r=nical)

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

::: gfx/gl/GLContext.cpp
@@ +2507,5 @@
>      return true;
>  }
>  
>  bool
> +GLContext::ResizeScreenBuffer(const IntSize& size)

ResizeOffscreen calls ResizeScreenBuffer(ThebesIntSize(aNewSize)), which creates a new gfxIntSize, only to implicitly create a new IntSize to call this function. Please remove the ThebesIntSize call.

Comment 83

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 5. Minimize gfxIntSize usage in GLContext. (carries r=nical)

Comment 84

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

Comment 85

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8348133 [details] [diff] [review]
[Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

Rebase needed some new code.

Comment 86

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 6. Moz2Dify SharedSurface_EGLImage::GetPixels.

Comment 87

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/111cc426fa9e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/45f8859c6fd6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4488ec28910e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/290ad5863615
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bf8095c168fb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/65ad9d8860d6

Comment 88

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2dc6642eba5d

for failures in mochitest-1 debug tests like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=32051948&tree=Mozilla-Inbound
and reftest failures like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=32052338&tree=Mozilla-Inbound

None of the try runs seem to have run any tests...

Comment 89

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

I reproduced, in a debug build on linux with GL layers enabled ( layers.acceleration.force-enabled , default outside of linux), while running online WebGL 1.0.1 conformance tests ( https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/webgl-conformance-tests.html ).

Assertion failure: dest->Stride() == dest->Width() * destPixelSize, at /hack/mozilla-central/gfx/gl/GLContext.cpp:2145

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff074da37 in mozilla::gl::GLContext::ReadPixelsIntoImageSurface (this=0x7fffbc47f800, dest=0x7fffbec2fa60) at /hack/mozilla-central/gfx/gl/GLContext.cpp:2145
2145        MOZ_ASSERT(dest->Stride() == dest->Width() * destPixelSize);
(gdb) bt
#0  0x00007ffff074da37 in mozilla::gl::GLContext::ReadPixelsIntoImageSurface (this=0x7fffbc47f800, dest=0x7fffbec2fa60) at /hack/mozilla-central/gfx/gl/GLContext.cpp:2145
#1  0x00007ffff074d72f in mozilla::gl::GLContext::ReadScreenIntoImageSurface (this=0x7fffbc47f800, dest=0x7fffbec2fa60) at /hack/mozilla-central/gfx/gl/GLContext.cpp:2086
#2  0x00007ffff076c00b in mozilla::gl::SharedSurface_Basic::Fence (this=0x7fffbabfe880) at /hack/mozilla-central/gfx/gl/SharedSurfaceGL.cpp:310
#3  0x00007ffff076dc01 in mozilla::gfx::SurfaceStream_TripleBuffer::SwapProducer (this=0x7fffaf5148a0, factory=0x7fffc5cd5980, size=...) at /hack/mozilla-central/gfx/gl/SurfaceStream.cpp:425
#4  0x00007ffff0754faf in mozilla::gl::GLScreenBuffer::Swap (this=0x7fffb6f51ca0, size=...) at /hack/mozilla-central/gfx/gl/GLScreenBuffer.cpp:424
#5  0x00007ffff0755115 in mozilla::gl::GLScreenBuffer::PublishFrame (this=0x7fffb6f51ca0, size=...) at /hack/mozilla-central/gfx/gl/GLScreenBuffer.cpp:445
#6  0x00007ffff074be59 in mozilla::gl::GLContext::PublishFrame (this=0x7fffbc47f800) at /hack/mozilla-central/gfx/gl/GLContext.cpp:1498
#7  0x00007ffff176f08f in mozilla::WebGLContext::PresentScreenBuffer (this=0x7fffb5887800) at /hack/mozilla-central/content/canvas/src/WebGLContext.cpp:1186
#8  0x00007ffff1778b08 in mozilla::WebGLContextUserData::PreTransactionCallback (data=0x7fffb933ced0) at /hack/mozilla-central/content/canvas/src/WebGLContext.cpp:857
#9  0x00007ffff081d4ab in mozilla::layers::CanvasLayer::FirePreTransactionCallback (this=0x7fffc46cf800) at /hack/mozilla-central/gfx/layers/Layers.h:1834
#10 0x00007ffff082a805 in mozilla::layers::ClientCanvasLayer::RenderLayer (this=0x7fffc46cf800) at /hack/mozilla-central/gfx/layers/client/ClientCanvasLayer.cpp:120

Comment 90

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

(gdb) p dest->Stride()
$2 = 208
(gdb) p dest->Width()
$3 = 50
(gdb) p destPixelSize
$4 = 4

So the bug is that the existing code assumed a fully packed surface (no stride at the end of each row of pixels) and that assumption broken by the switch to moz2d surfaces, which apparently are not packed (above we have a 8 pixel stride at the end of each row (208-50*4))

Comment 91

[Andreas Pehrson [:pehrsons]]

Attachment: Add factory method for DataSourceSurfaces with custom stride.

Comment 92

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

Comment 93

[Andreas Pehrson [:pehrsons]]

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

Comment 94

[Bas Schouten (:bas.schouten)]

Comment on attachment 8350089 [details] [diff] [review]
Add factory method for DataSourceSurfaces with custom stride.

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

::: gfx/2d/Factory.cpp
@@ +655,5 @@
> +Factory::CreateDataSourceSurfaceWithStride(const IntSize &aSize,
> +                                           SurfaceFormat aFormat,
> +                                           int32_t aStride)
> +{
> +  if (aStride < aSize.width * BytesPerPixel(aFormat)) {

nit: Add an assertion here for debug builds.

Comment 95

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

new try with a enum conversion fixed: https://tbpl.mozilla.org/?tree=Try&rev=b9a56a114a47

Comment 96

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

https://tbpl.mozilla.org/?tree=Try&rev=288c58ae1815
https://tbpl.mozilla.org/?tree=Try&rev=26c13a74ec89
https://tbpl.mozilla.org/?tree=Try&rev=7c75199a909d
https://tbpl.mozilla.org/?tree=Try&rev=91f85b897e1b
https://tbpl.mozilla.org/?tree=Try&rev=b3957297e97a
https://tbpl.mozilla.org/?tree=Try&rev=de8cbd0f3633
https://tbpl.mozilla.org/?tree=Try&rev=5fd98cf07392

Comment 97

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

Now we know that it is part 3 that is causing the reftest failures.

Comment 98

[Andreas Pehrson [:pehrsons]]

Yep. I finally have a working environment so I hope to have a fix pretty soon. Currently building firefox at AMS =)

Comment 99

[Andreas Pehrson [:pehrsons]]

I just reproduced it locally. Work can start..

Comment 100

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8350091 [details] [diff] [review]
[Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

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

::: gfx/layers/d3d10/CanvasLayerD3D10.cpp
@@ +168,5 @@
> +                                                                   map.RowPitch,
> +                                                                   FORMAT_B8G8R8A8);
> +          Rect drawRect(0, 0, shareSurf->Size().width, shareSurf->Size().height);
> +          dt->DrawSurface(frameData, drawRect, drawRect);
> +          dt->Flush();

Here is the bad guy causing the failures. I have no idea why yet, but I tested drawing with gfxContext and two gfxImageSurfaces (like before, essentially) and that worked just fine.

Comment 101

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

Comment 102

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 6. Moz2Dify SharedSurface_EGLImage::GetPixels. (carries r=nical)

Comment 103

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 2. Moz2Dify GLContext::ResizeOffscreen signature. (carries r=nical)

Comment 104

[Andreas Pehrson [:pehrsons]]

Attachment: [Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

Made a fix before. This time a rebase.

Comment 105

[Andreas Pehrson [:pehrsons]]

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

Let's see some magic! Consider it my christmas gift for you :p

Comment 106

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

Comment on attachment 8351417 [details] [diff] [review]
[Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

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

Awesome Xmas present!

Note that there remains the bad enum conversion that we discussed in part 1. I'll fix that too, and land the whole thing.

Comment 107

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

Apparently due to some more bitrotting, a few more fixes had to be made to get things to build on either desktop Linux or Android. Enough to require another try push:

https://tbpl.mozilla.org/?tree=Try&rev=7c99e2d2750a

Comment 108

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

Another push for Mac:

https://tbpl.mozilla.org/?tree=Try&rev=7ed6dafe3727

Comment 109

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

Remaining platforms (Win and B2G) also needed similar build fixes:
https://tbpl.mozilla.org/?tree=Try&rev=b6388a1b41fd

Comment 110

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

Happy new year!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a2b3c7ba51ce
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6ad6f699e1ce
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4312ade99015
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4757b9992474
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6ab615e8e4b2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/705c8c405dbb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a1d6463e391b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/370591efe014

Comment 111

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a2b3c7ba51ce
https://hg.mozilla.org/mozilla-central/rev/6ad6f699e1ce
https://hg.mozilla.org/mozilla-central/rev/4312ade99015
https://hg.mozilla.org/mozilla-central/rev/4757b9992474
https://hg.mozilla.org/mozilla-central/rev/6ab615e8e4b2
https://hg.mozilla.org/mozilla-central/rev/705c8c405dbb
https://hg.mozilla.org/mozilla-central/rev/a1d6463e391b
https://hg.mozilla.org/mozilla-central/rev/370591efe014

Comment 112

[Andreas Pehrson [:pehrsons]]

Excellent work Benoit! And happy new year =)

Comment 113

[Andreas Pehrson [:pehrsons]]

Attachment: Remove GetTexImage from GLContext helper;

Comment 114

[Andreas Pehrson [:pehrsons]]

Attachment: Moz2Dify layer snapshot dumping further;

Comment 115

[Andreas Pehrson [:pehrsons]]

try push: https://tbpl.mozilla.org/?tree=Try&rev=7595cc692606

Comment 116

[Andreas Pehrson [:pehrsons]]

Note on the order: keep it chronological

Comment 117

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59f331fd7867
https://hg.mozilla.org/integration/mozilla-inbound/rev/0208bf574011

Comment 118

[Andreas Pehrson [:pehrsons]]

Attachment: Moz2Dify CanvasLayerD3D9 drawing shared surface into D3DLOCKED_RECT.

Comment 119

[Andreas Pehrson [:pehrsons]]

Attachment: Moz2Dify CanvasLayerD3D10 drawing shared surface into D3D10_MAPPED_TEXTURE2D.

Comment 120

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8358486 [details] [diff] [review]
Moz2Dify CanvasLayerD3D9 drawing shared surface into D3DLOCKED_RECT.

This is a continuation of a previous patch on this bug: https://hg.mozilla.org/mozilla-central/rev/4757b9992474

Comment 121

[Andreas Pehrson [:pehrsons]]

try push: https://tbpl.mozilla.org/?tree=Try&rev=92bb082bf204

Comment 122

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59f331fd7867
https://hg.mozilla.org/mozilla-central/rev/0208bf574011

Comment 123

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8357028 [details] [diff] [review]
Remove GetTexImage from GLContext helper;

They're not obsolete just because they landed :)

Comment 124

[Ryan VanderMeulen [:RyanVM]]

This has some conflicts that I don't want to try to rebase through, sorry :(

Also, should this bug be resolved when these two patches land?

Comment 125

[Andreas Pehrson [:pehrsons]]

Oh, I just saw you obsolete the older ones and assumed that was how it's done.

No, not resolved yet.

Comment 126

[Andreas Pehrson [:pehrsons]]

Attachment: Moz2Dify CanvasLayerD3D9 drawing shared surface into D3DLOCKED_RECT. (carries r=nical)

Comment 127

[Andreas Pehrson [:pehrsons]]

Attachment: Moz2Dify CanvasLayerD3D10 drawing shared surface into D3D10_MAPPED_TEXTURE2D. (carries r=nical)

Comment 128

[Ryan VanderMeulen [:RyanVM]]

I did it mainly to better keep track of what had landed and what hadn't :). Was hard to make sense of as someone who hasn't been watching this bug all along :). Thanks for the rebase!

https://hg.mozilla.org/integration/mozilla-inbound/rev/dba042cbc381
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfd0864f189d

Comment 129

[Ryan VanderMeulen [:RyanVM]]

I asked you to do the rebase because of what were obvious context differences that I didn't want to make assumptions about. Looks like those differences were ignored in your rebase. Consequently, these broke Windows builds. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/456104e27a9a

https://tbpl.mozilla.org/php/getParsedLog.php?id=32922046&tree=Mozilla-Inbound

Comment 130

[Andreas Pehrson [:pehrsons]]

Attachment: Moz2Dify CanvasLayerD3D9 drawing shared surface into D3DLOCKED_RECT. (carries r=nical)

Comment 131

[Andreas Pehrson [:pehrsons]]

Attachment: Moz2Dify CanvasLayerD3D10 drawing shared surface into D3D10_MAPPED_TEXTURE2D. (carries r=nical)

Comment 132

[Andreas Pehrson [:pehrsons]]

Sorry Ryan, apparently not obvious enough for me >_<
Well, lesson learned!

Here goes a try push: https://tbpl.mozilla.org/?tree=Try&rev=4cd01e1836f1

Comment 133

[Andreas Pehrson [:pehrsons]]

Ah, win64 is not a release target. Then we should be good to go.

Sorry again for the inconvenience.

Comment 134

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6da040d1f81
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d816dd5e310

Comment 135

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e6da040d1f81
https://hg.mozilla.org/mozilla-central/rev/0d816dd5e310

Comment 136

[Luis de Bethencourt [:luisbg]]

I see these two patches merged in mozilla-central. Should the bug be closed?

Comment 137

[Andreas Pehrson [:pehrsons]]

No, there is still some work to be done.

I have been occupied by other things lately but hope to get back to it soon.

Comment 138

[Luis de Bethencourt [:luisbg]]

Cool! Great work Andreas.

Comment 139

[Kartikaya Gupta (email:[e-mail])]

Would you mind if we closed this bug and filed a follow-up bug for the remaining work? This stuff landed a while ago and it gets confusing if patches from a single bug are split across releases. Also, this bug shows up as a mentored bug in the bugsahoy dashboard when it's not really appropriate for mentoring it seems like.

Comment 140

[Andreas Pehrson [:pehrsons]]

Please do. I had hoped to get around to finish this one but that never happened, unfortunately. It should not be too much left though.