Closed
Bug 1043929
Opened 10 years ago
Closed 10 years ago
CompositableHost::AddMaskEffect should unlock the texture in case of failure
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: nical, Assigned: nical)
Details
Attachments
(2 files)
1.45 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
5.81 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
There will be followups, but with that we can already catch most of the failure cases
Attachment #8462568 -
Flags: review?(bas)
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
Addressed the comment and landed. https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3ea1113de3 https://hg.mozilla.org/integration/mozilla-inbound/rev/0d8a1a23f586
Assignee | ||
Comment 6•10 years ago
|
||
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
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4e3ea1113de3 https://hg.mozilla.org/mozilla-central/rev/0d8a1a23f586 https://hg.mozilla.org/mozilla-central/rev/6a0bbba44b81
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•