Closed
Bug 996745
Opened 10 years ago
Closed 10 years ago
[OMTC Windows] failing assertion in ContentClient with component alpha layers on Windows 7
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: nical, Assigned: bas.schouten)
Details
Attachments
(1 file, 1 obsolete file)
1.32 KB,
patch
|
mattwoodrow
:
review+
nical
:
feedback+
|
Details | Diff | Splinter Review |
See this try push: https://tbpl.mozilla.org/?tree=Try&rev=0816f3982887
Reporter | ||
Comment 1•10 years ago
|
||
We hit an assertion that checks that the ContentClient "HaveBufferOnWhite()". the latter checks that the BufferProvider on white or DrawTarget on white is set. In the stack trace they are not set which is unfortunate because the mTextureClient and mTextureClient on white are non null (and mBufferProvider[onWhite] is supposed to be mTextureClient[OnWhite]). A simple fix is to just set the buffer provider there. I am not entorely satisfied as I haven't figured out why we are in a situation where the buffer providers aren't set, but it does fix the assertion as shown in this try push: https://tbpl.mozilla.org/?tree=Try&rev=a6030dc4867e
Attachment #8408179 -
Flags: review?(bas)
Assignee | ||
Comment 2•10 years ago
|
||
This is fine with me, however I also have another patch for this that changes things a little more involved. Namely this doesn't prevent us from doing an absolutely useless copy :). The reason that the buffer providers are nullptr is that we've just called Clear() in BeginPaint because our content type changed. Once we've done that we've actually cleared mBufferRect too, which will be empty here, even though the call to FinalizeFrame explicitly has comments around it saying mBufferRect should -not- be changed before the function. My fix is to add if (HaveBuffer()) around the call to finalizeframe, although I'm not yet 100% sure that's the right fix either.
Reporter | ||
Comment 3•10 years ago
|
||
Awesome, sounds like you have a better fix and that we still have an okay-ish fix (my patch) as backup just in case. Reassigning this to you, so that you can do it the right way, then.
Assignee: nical.bugzilla → bas
Assignee | ||
Comment 4•10 years ago
|
||
This is my suggestion.
Attachment #8410419 -
Flags: review?(nical.bugzilla)
Reporter | ||
Comment 5•10 years ago
|
||
Comment on attachment 8410419 [details] [diff] [review] fix-no-buffer Review of attachment 8410419 [details] [diff] [review]: ----------------------------------------------------------------- Matt is a better reviewer for this. It looks good to me but I don't understand the surrounding code well enough.
Attachment #8410419 -
Flags: review?(nical.bugzilla)
Attachment #8410419 -
Flags: review?(matt.woodrow)
Attachment #8410419 -
Flags: feedback+
Comment 6•10 years ago
|
||
Comment on attachment 8410419 [details] [diff] [review] fix-no-buffer Review of attachment 8410419 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/RotatedBuffer.cpp @@ +520,5 @@ > + // Do not modify result.mRegionToDraw or result.mContentType after this call. > + // Do not modify mBufferRect, mBufferRotation, or mDidSelfCopy, > + // or call CreateBuffer before this call. > + FinalizeFrame(result.mRegionToDraw); > + } This seems like the right thing to do if we've just cleared our buffers for a content change, and should strictly be an improvement. What about the second paint to a Layer? The first paint will have allocated buffers and transferred them to the compositor, so we once again have no buffers. Since we're not taking this path, does that mean we'll have to repaint the entire visible region again instead of copying it back? It seems like we want to differentiate between 'we just don't have a buffer yet', and 'we explicitly rejected an existing buffer' and then make another attempt to copy back from the front buffer for the former case.
Assignee | ||
Updated•10 years ago
|
Attachment #8408179 -
Attachment is obsolete: true
Attachment #8408179 -
Flags: review?(bas)
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 7•10 years ago
|
||
We also have non-optimal behaviour if we reallocate the buffer due to a size change. In that case FinalizeFrame will copy to the old buffer, and then we'll copy again into the new buffer. Depending on the invalidations, that could be an unnecessary copy of the entire buffer. I think we want to call FinalizeFrame *after* we do the buffer allocation step so we always have a destination buffer. We can just have it not do anything if the content types don't match (but not matching size is ok). We'll also probably want the code that copies from old -> new during a reallocation to skip any pixels that will be overwritten again during FinalizeFrame.
Comment 8•10 years ago
|
||
Comment on attachment 8410419 [details] [diff] [review] fix-no-buffer Review of attachment 8410419 [details] [diff] [review]: ----------------------------------------------------------------- r=me since this is better than what we have now. Please file a followup bug for the remaining work.
Attachment #8410419 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 9•10 years ago
|
||
Filed bug 1003383.
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0825a0c4bf9
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a0825a0c4bf9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•