Closed
Bug 991513
Opened 10 years ago
Closed 10 years ago
Gralloc locking / unlocking during empty transactions
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: mstange, Assigned: mattwoodrow)
Details
Attachments
(3 files)
3.92 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
2.94 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
8.45 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
I just took a profile of homescreen panning and noticed that we're blocking in GrallocTextureClientOGL::Unlock() during the empty transactions that should only be updating layer transforms: nsRefreshDriver::Tick(long long, mozilla::TimeStamp) nsViewManager::ProcessPendingUpdates() nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) Paint::PresShell::Paint PresShell::Paint(nsView*, nsRegion const&, unsigned int) mozilla::layers::ClientLayerManager::EndEmptyTransaction(mozilla::layers::LayerManager::EndTransactionFlags) ClientLayerManager::EndTransactionInternal mozilla::layers::ClientLayerManager::EndTransactionInternal(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) mozilla::layers::ClientContainerLayer::RenderLayer() mozilla::layers::ClientContainerLayer::RenderLayer() mozilla::layers::ClientThebesLayer::RenderLayer() mozilla::layers::ContentClientRemoteBuffer::EndPaint() mozilla::layers::GrallocTextureClientOGL::Unlock() We should not do that.
Comment 1•10 years ago
|
||
Where is the corresponding Lock coming from? RotatedBuffer -> CreateBuffer?
Assignee | ||
Comment 2•10 years ago
|
||
ContentClientDoubleBuffered::PreprareFrame I believe. We used to just set a 'buffer provider' that let the RotatedContentBuffer know that a buffer existed, and what size/format it was without actually locking it. Then RotatedContentBuffer::EnsureBuffer() would map/lock the real buffer once we got to the point where we knew we actually needed to modify it.
Assignee | ||
Comment 3•10 years ago
|
||
Adding nical to the cc list because locking time was identified as one of the issues with D3D11 OMTC. Fixing this bug should improve performance there, I'd expect tcanvasmark to show the difference.
Assignee | ||
Comment 4•10 years ago
|
||
Apart from just delaying the locking, this also has the effect that we no longer do the FinalizeFrame copying work if aRegionToDraw is empty. I think this is ok, since we don't call SwapBuffers in this case either, so it should be fine to leave the buffers (and mFrontAndBackBufferDiffer) unchanged until we need to actually paint something.
Attachment #8403055 -
Flags: review?(ncameron)
Assignee | ||
Comment 5•10 years ago
|
||
The same effect, but for ContentClientSingleBuffered. I'm not sure why this code was as complicated as it was. Calling SetDTBuffer with the current mBufferRect/Rotation shouldn't do anything interesting except set the buffer pointer itself. We already handle that in EnsureBuffer.
Attachment #8403058 -
Flags: review?(ncameron)
Assignee | ||
Comment 6•10 years ago
|
||
Makes the calling code a bit simpler.
Attachment #8403061 -
Flags: review?(ncameron)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → matt.woodrow
Updated•10 years ago
|
Attachment #8403061 -
Flags: review?(ncameron) → review+
Comment 7•10 years ago
|
||
Comment on attachment 8403055 [details] [diff] [review] Don't lock until we need the buffers Review of attachment 8403055 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/RotatedBuffer.cpp @@ +500,5 @@ > result.mRegionToDraw.Sub(neededRegion, validRegion); > > + if (result.mRegionToDraw.IsEmpty()) > + return result; > + This scares me. I think it is OK, but do we definitely not need everything that happens in FinalizeFrame? My worry is we will do a swap even though we didn't paint and we'll have an old set of flags. Also this code path triggers a flush that we will miss. Will that break D2D things? I'll believe we can do this if you say so, but I am still a bit unsure.
Comment 8•10 years ago
|
||
Comment on attachment 8403055 [details] [diff] [review] Don't lock until we need the buffers Review of attachment 8403055 [details] [diff] [review]: ----------------------------------------------------------------- Ah, I just saw the comment in the bug which mostly answers my question. r=me assuming this works on Windows with the missing flush.
Attachment #8403055 -
Flags: review?(ncameron) → review+
Comment 9•10 years ago
|
||
Comment on attachment 8403058 [details] [diff] [review] Don't lock until we need the buffers - single buffered Review of attachment 8403058 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/ContentClient.cpp @@ -545,5 @@ > - > - RefPtr<DrawTarget> oldBuffer; > - oldBuffer = SetDTBuffer(backBuffer, > - mBufferRect, > - mBufferRotation); Hmm, I think you are right and this is pointless, but it definitely used to do something - I wish I could remember what. I think we used to have different buffer rect/rotation values in the subclass vs the superclass and this updated the latter to the former. I guess that changed. I think you can get rid of SetDTBuffer and SetDTBufferOnWhite - they are both dead code now. r=me with that.
Attachment #8403058 -
Flags: review?(ncameron) → review+
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdacae6b8331 https://hg.mozilla.org/integration/mozilla-inbound/rev/6c5ff525ab62 https://hg.mozilla.org/integration/mozilla-inbound/rev/ab89b8f6b481
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cdacae6b8331 https://hg.mozilla.org/mozilla-central/rev/6c5ff525ab62 https://hg.mozilla.org/mozilla-central/rev/ab89b8f6b481
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•