Closed Bug 1396474 Opened 2 years ago Closed 2 years ago

Rotated buffer contents can be drawn incorrectly when using OMTP

Categories

(Core :: Graphics: Layers, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

This leads to some of reftest failures on unaccelerated systems. This is much more likely when having to draw all 4 quadrants.

This is caused by RotatedContentBuffer::BorrowDrawTargetForQuadrant applying the current transform of the DrawTarget to its final output transform. With OMTP that transform may have already been adjusted on the painting thread if painting has commenced by the time we attempt to acquire the draw target for one of the other quadrants.

As far as I can tell when aSetTransform is false mLoanedTransform, nor applying the current transform doesn't seem to serve any purpose. I'd like to understand why we do this in the first place.
Matt, what do you think? Can we get away with this or should I add something more complicated like pulling the transform when iterator count is 0 or maybe something simpler like never applying the current transform at all. (I'm unsure when this would ever matter for -rotated- content buffers, i.e. when we're not using tiled layers or anything like that)
Flags: needinfo?(matt.woodrow)
Comment on attachment 8904108 [details]
Bug 1396474: When aSetTransform is false, do not rely on the current DT transform when returning the output transform.

https://reviewboard.mozilla.org/r/175886/#review181154

I think this fine, we shouldn't ever be setting persistent transforms on the buffer.

It's a bit scary though, it seems like multiple threads are accessing mLoanedDrawTarget?

It would be nice if we could avoid that somehow, it's pretty easy to have bugs like this.
Attachment #8904108 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Comment on attachment 8904108 [details]
> Bug 1396474: When aSetTransform is false, do not rely on the current DT
> transform when returning the output transform.
> 
> https://reviewboard.mozilla.org/r/175886/#review181154
> 
> I think this fine, we shouldn't ever be setting persistent transforms on the
> buffer.
> 
> It's a bit scary though, it seems like multiple threads are accessing
> mLoanedDrawTarget?
> 
> It would be nice if we could avoid that somehow, it's pretty easy to have
> bugs like this.

Not really multiple threads.. there's only a single paintthread right now. mLoanedDrawTarget is basically the final destination draw target, so it's always going to be accessed on the paint thread. If what you mean is multiple DrawTargetCapture's are drawing to the same mLoanedDrawTarget, then yeah.
Flags: needinfo?(matt.woodrow)
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e933a2178e1
When aSetTransform is false, do not rely on the current DT transform when returning the output transform. r=mattwoodrow
(In reply to Bas Schouten (:bas.schouten) from comment #0)
> This leads to some of reftest failures on unaccelerated systems. This is
> much more likely when having to draw all 4 quadrants.
> 
> This is caused by RotatedContentBuffer::BorrowDrawTargetForQuadrant applying
> the current transform of the DrawTarget to its final output transform. With
> OMTP that transform may have already been adjusted on the painting thread if
> painting has commenced by the time we attempt to acquire the draw target for
> one of the other quadrants.
> 
> As far as I can tell when aSetTransform is false mLoanedTransform, nor
> applying the current transform doesn't seem to serve any purpose. I'd like
> to understand why we do this in the first place.

Yeah we don't really use it. It's really just there so we can reuse the code that determines the transform to set it on the capture drawtarget, which we'll then use to set it on the actual destination draw target on the paint thread.
(In reply to Bas Schouten (:bas.schouten) from comment #4)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> > Comment on attachment 8904108 [details]
> > Bug 1396474: When aSetTransform is false, do not rely on the current DT
> > transform when returning the output transform.
> > 
> > https://reviewboard.mozilla.org/r/175886/#review181154
> > 
> > I think this fine, we shouldn't ever be setting persistent transforms on the
> > buffer.
> > 
> > It's a bit scary though, it seems like multiple threads are accessing
> > mLoanedDrawTarget?
> > 
> > It would be nice if we could avoid that somehow, it's pretty easy to have
> > bugs like this.
> 
> Not really multiple threads.. there's only a single paintthread right now.
> mLoanedDrawTarget is basically the final destination draw target, so it's
> always going to be accessed on the paint thread. If what you mean is
> multiple DrawTargetCapture's are drawing to the same mLoanedDrawTarget, then
> yeah.

Yes but it should be serialized in the same order as what we do with the main thread. E.g., the same mLoanedDrawTarget can be drawn to multiple times on the main thread without OMTP.
(In reply to Mason Chang PTO Until 9/5 [:mchang] from comment #7)
> (In reply to Bas Schouten (:bas.schouten) from comment #4)
> > (In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> > > Comment on attachment 8904108 [details]
> > > Bug 1396474: When aSetTransform is false, do not rely on the current DT
> > > transform when returning the output transform.
> > > 
> > > https://reviewboard.mozilla.org/r/175886/#review181154
> > > 
> > > I think this fine, we shouldn't ever be setting persistent transforms on the
> > > buffer.
> > > 
> > > It's a bit scary though, it seems like multiple threads are accessing
> > > mLoanedDrawTarget?
> > > 
> > > It would be nice if we could avoid that somehow, it's pretty easy to have
> > > bugs like this.
> > 
> > Not really multiple threads.. there's only a single paintthread right now.
> > mLoanedDrawTarget is basically the final destination draw target, so it's
> > always going to be accessed on the paint thread. If what you mean is
> > multiple DrawTargetCapture's are drawing to the same mLoanedDrawTarget, then
> > yeah.
> 
> Yes but it should be serialized in the same order as what we do with the
> main thread. E.g., the same mLoanedDrawTarget can be drawn to multiple times
> on the main thread without OMTP.

I believe that order is guaranteed though as long as we have a single paint thread that processes events in order.
https://hg.mozilla.org/mozilla-central/rev/2e933a2178e1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.