Closed Bug 991513 Opened 6 years ago Closed 6 years ago

Gralloc locking / unlocking during empty transactions

Categories

(Core :: Graphics, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: mstange, Assigned: mattwoodrow)

Details

Attachments

(3 files)

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.
Where is the corresponding Lock coming from? RotatedBuffer -> CreateBuffer?
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.
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.
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)
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)
Makes the calling code a bit simpler.
Attachment #8403061 - Flags: review?(ncameron)
Assignee: nobody → matt.woodrow
Attachment #8403061 - Flags: review?(ncameron) → review+
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 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 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+
You need to log in before you can comment on or make changes to this bug.