[OMTC Windows] failing assertion in ContentClient with component alpha layers on Windows 7

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nical, Assigned: bas.schouten)

Tracking

unspecified
mozilla32
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Comment 1

5 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

5 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

5 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

5 years ago
Posted patch fix-no-bufferSplinter Review
This is my suggestion.
Attachment #8410419 - Flags: review?(nical.bugzilla)
Reporter

Comment 5

5 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 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

5 years ago
Attachment #8408179 - Attachment is obsolete: true
Attachment #8408179 - Flags: review?(bas)
Assignee

Updated

5 years ago
Status: NEW → ASSIGNED
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 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

5 years ago
Filed bug 1003383.
https://hg.mozilla.org/mozilla-central/rev/a0825a0c4bf9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.