Closed Bug 1043929 Opened 5 years ago Closed 5 years ago

CompositableHost::AddMaskEffect should unlock the texture in case of failure

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: nical, Assigned: nical)

Details

Attachments

(2 files)

If the TextureHost is locked but doesn't return a TextureSource (which is unlikely but nothing proves that it can't happen), we return without unlocking the TextureHost. It's important to always unlock locked textures to avoid getting into a weird state in the next frame.
This isn't likely to bite us but let's fix it now that I noticed it otherwise it'll be harder to debug if it happens some day.
As a second step we should go further and ensure that if lock succeeds, there will always be a valid TextureSource. It might already be implicitly the case, or very close to it so it's probably mostly about adding some assertions and documenting the API.
Attachment #8462554 - Flags: review?(bas)
There will be followups, but with that we can already catch most of the failure cases
Attachment #8462568 - Flags: review?(bas)
Comment on attachment 8462554 [details] [diff] [review]
make warnings more explicit about what's going on and unlock the host if needed.

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

I'm not sure I see a lot of value in this but it also can't hurt I suppose.
Attachment #8462554 - Flags: review?(bas) → review+
Comment on attachment 8462568 [details] [diff] [review]
Ensure that a locked TextureHost yields a valid TextureSource for some of the texture types and add some assertions

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

In general I think this a good improvement.

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +836,5 @@
>  
>  void
>  TextureHostD3D9::Updated(const nsIntRegion* aRegion)
>  {
> +  MOZ_ASSERT(mTexture);

We should still have an early return here in addition to the assertion.
Attachment #8462568 - Flags: review?(bas) → review+
One of the patches requires the TextureHost to have a Compositor in order to Lock() successfully, which is a valid assumption but TestTextures.cpp tries to test the texture class in isolation so it had to be fixed (in the test).
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a0bbba44b81
You need to log in before you can comment on or make changes to this bug.