This is a bit weird. My comment explains it partially. Previously we had a special YCbCr texture host that didn't implement the tiled iterator, so we took the path that supported picture rects. Now we just have a linked list of 3 TextureImageTextureHostOGL objects, and these do. We iterate over the tiles (usually just one) of the first one, and call DrawQuad. This will read from all three textures, since they're in the effect. Though if we have actual tiles with multiple textures, then we won't be advancing the iterator on the others, so it will be broken (bug 899583 I'm assuming). This fixes it, but you may have better ideas.

https://tbpl.mozilla.org/?tree=Try&rev=e2379f6c4675 The remaining issue (which i don't think shows up on try), is that we still have a potential race when using single buffered image clients. My preference here would be to tell new ImageClients if they are going to be used in a double buffered way. If they're *not* (and are going to be single buffered), then they either need to be immutable, or support locking correctly. The implementations could then decide which of these two options is right for them, and add the immutable flag to themselves if needed. Thoughts?

Comment on attachment 783891 [details] [diff] [review] Implement picture rect support Review of attachment 783891 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/ImageHost.cpp @@ +72,5 @@ > + mPictureRect.width, > + mPictureRect.height); > + //XXX: We might have multiple texture sources here (e.g. 3 YCbCr textures), and we're > + // only iterating over the tiles of the first one. Are we assuming that the tiling > + // will be identical? Can we ensure that somehow? You can remove this comment: with bug 899583, when we have YCbCr textures we disable the TileIterator thing (i'll land it soon).

Comment on attachment 783892 [details] [diff] [review] Recreate the TextureClient if the size changes Review of attachment 783892 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/TextureClient.h @@ +123,5 @@ > virtual bool IsAllocated() const = 0; > > virtual bool ToSurfaceDescriptor(SurfaceDescriptor& aDescriptor) = 0; > > + virtual const gfx::IntSize& GetSize() const = 0; Please return a gfx::IntSize by value rather than by const reference. That's what we do in the rest of the texture code, IntSize is 64bit in total so it's not a problem to return it by value, and it's simpler for everyone when all the GetSize functions have the same signature.

(In reply to Matt Woodrow (:mattwoodrow) from comment #4) > Created attachment 783891 [details] [diff] [review] > Implement picture rect support > > This is a bit weird. My comment explains it partially. > > Previously we had a special YCbCr texture host that didn't implement the > tiled iterator, so we took the path that supported picture rects. > > Now we just have a linked list of 3 TextureImageTextureHostOGL objects, and > these do. > > We iterate over the tiles (usually just one) of the first one, and call > DrawQuad. This will read from all three textures, since they're in the > effect. > > Though if we have actual tiles with multiple textures, then we won't be > advancing the iterator on the others, so it will be broken (bug 899583 I'm > assuming). > > This fixes it, but you may have better ideas. Now TextureSource is the tile iterator, not TextureHost, so if you want to iterate over multiple textures, you must do it on the (multiple) TextureSources. With YCbCr the Y plane is bigger than the Cb and Cr so we'd have to do some tricky stuff to get it to work properly with just disabling BigImage (the new name for in-TextureSource Tiling). Bug 899583 fixes the YCbCr issue the same way we fixed it for deprecated textures (by making sure we don't use BiImage).

(In reply to Matt Woodrow (:mattwoodrow) from comment #6) > https://tbpl.mozilla.org/?tree=Try&rev=e2379f6c4675 > > > The remaining issue (which i don't think shows up on try), is that we still > have a potential race when using single buffered image clients. > > My preference here would be to tell new ImageClients if they are going to be > used in a double buffered way. > > If they're *not* (and are going to be single buffered), then they either > need to be immutable, or support locking correctly. > > The implementations could then decide which of these two options is right > for them, and add the immutable flag to themselves if needed. > > Thoughts? That would work for me. With a flag in CreateImageClient that provides info about what kind of usage the ImageClient is for (immutable textures only, preallocated textures only, double buffering, single buffering, async...) and remove this info from the CompositableType (which would then be just IMAGE, CONTENT, etc.). The nice thing is that the Host doesn't need to know about buffering or immutability. Right now choosing the image client type is the responsibility of the caller of CreateImageClient, I am not against moving this logic into CreateImageClient.

I don't which of bug 893824, bug 900133 or bug 898129 caused it, but inbound has been closed for long enough. https://hg.mozilla.org/integration/mozilla-inbound/rev/5e1009e4b1e1

https://hg.mozilla.org/integration/mozilla-inbound/rev/c242c3498ff1 https://hg.mozilla.org/integration/mozilla-inbound/rev/60c4ed147847 https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4eb733fd77 https://hg.mozilla.org/integration/mozilla-inbound/rev/9ea3f06786ea

We can probably do better again by having proper cross-process locking, but this fixes the performance regression of new textures and avoids races.

Comment on attachment 785090 [details] [diff] [review] Add TEXTURE_IMMEDIATE_UPLOAD to avoid racing when single buffered. Review of attachment 785090 [details] [diff] [review]: ----------------------------------------------------------------- r=me with this comment fixed: ::: gfx/layers/CompositorTypes.h @@ +86,5 @@ > + // If we're not double buffered, or uploading > + // within a transaction, then we need to support > + // locking correctly. > + return !(aFlags & (TEXTURE_IMMEDIATE_UPLOAD | > + TEXTURE_DOUBLE_BUFFERED)); ... || (!aFlags & TEXTURE_IMMUTABLE) I see below that you use this to decide whether we should mark the texture immutable, but checking for TEXTURE_IMMUTABLE will not break it and the result of this funtion will correspond more to the function name.

Comment on attachment 785087 [details] [diff] [review] Call OnTransaction *after* we possibly recreate our texture host as well Review of attachment 785087 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the unneeded mCanvasClient->OnTransaction(); removed ::: gfx/layers/client/ClientCanvasLayer.cpp @@ +103,5 @@ > mCanvasClient->Connect(); > ClientManager()->Attach(mCanvasClient, this); > } > + } else { > + mCanvasClient->OnTransaction(); I think we don't need this

https://hg.mozilla.org/mozilla-central/rev/c242c3498ff1 https://hg.mozilla.org/mozilla-central/rev/60c4ed147847 https://hg.mozilla.org/mozilla-central/rev/fc4eb733fd77 https://hg.mozilla.org/mozilla-central/rev/9ea3f06786ea

https://hg.mozilla.org/integration/mozilla-inbound/rev/a857a4246dd9 https://tbpl.mozilla.org/?tree=Try&rev=f6a2f562512f https://hg.mozilla.org/integration/mozilla-inbound/rev/036780fccc89 https://hg.mozilla.org/integration/mozilla-inbound/rev/49632bc14e27

Still not quite done here.

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/5695cebb5c12 for breaking the builds like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=26106235&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/779afd60e520 https://hg.mozilla.org/integration/mozilla-inbound/rev/fa5a3c887c7b https://hg.mozilla.org/integration/mozilla-inbound/rev/64ab855db547

https://hg.mozilla.org/mozilla-central/rev/779afd60e520 https://hg.mozilla.org/mozilla-central/rev/fa5a3c887c7b https://hg.mozilla.org/mozilla-central/rev/64ab855db547

Otherwise we think they're immutable before we've drawn them at all.

I managed to break things so we weren't specifying TEXTURE_DEALLOCATE_HOST and it was leaking.

Green! https://tbpl.mozilla.org/?tree=Try&rev=21409983f814

Comment on attachment 790019 [details] [diff] [review] Enable new textures on OSX Review of attachment 790019 [details] [diff] [review]: ----------------------------------------------------------------- yay \o/

Comment on attachment 790018 [details] [diff] [review] Make sure we always deallocate textures Review of attachment 790018 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/ShadowLayers.cpp @@ +383,5 @@ > if (!aTexture->ToSurfaceDescriptor(descriptor)) { > NS_WARNING("Failed to serialize a TextureClient"); > return; > } > + MOZ_ASSERT(aTexture->GetFlags() != 0); For the B2G camera that wants to manage recycling the gralloc buffers we will need to not have TEXTURE_DEALLOCATE_CLIENT nor TEXTURE_DEALLOCATE_HOST (that's the reason this info is not just stored in one bit), so this assertion may not be correct when we get there. I am fine with the assertion staying until we completely implement new gralloc textures but it is likely to be removed/changed at some point.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f19e44e32c75 https://hg.mozilla.org/integration/mozilla-inbound/rev/b8ce320d6574 https://hg.mozilla.org/integration/mozilla-inbound/rev/252dbb41d609

https://hg.mozilla.org/mozilla-central/rev/f19e44e32c75 https://hg.mozilla.org/mozilla-central/rev/b8ce320d6574 https://hg.mozilla.org/mozilla-central/rev/252dbb41d609

Backed out the new textures pref on OSX https://hg.mozilla.org/integration/mozilla-inbound/rev/5f81cbac8d62

Unfortunately, this was causing crashes in the WebGL mochitest suite, so I had to back it out. https://hg.mozilla.org/integration/mozilla-inbound/rev/b4c88993bb3c https://tbpl.mozilla.org/php/getParsedLog.php?id=27075194&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=27074503&tree=Mozilla-Inbound