Closed Bug 1264764 Opened 8 years ago Closed 8 years ago

Move PTexture under PCompositorBridge

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: gw280, Assigned: gw280)

References

Details

Attachments

(1 file, 2 obsolete files)

This should allow us to move TextureClientPool under CompositorBridgeChild which will help our tab switch times.
This is a real mess right now. I could do with some guidance.
Attachment #8741477 - Flags: feedback?(nical.bugzilla)
Assignee: nobody → gwright
Comment on attachment 8741477 [details] [diff] [review]
0001-attempt-at-moving-PTexture-under-CompositorBridge.patch

Review of attachment 8741477 [details] [diff] [review]:
-----------------------------------------------------------------

You are going in the right direction and there's a lot of stuff left to adjust. have fun!

::: gfx/layers/ipc/CompositorBridgeChild.cpp
@@ +123,5 @@
> +      //gfxDevCrash(gfx::LogReason::TextureAliveAfterShutdown)
> +      //  << "A texture is held alive after shutdown (PCompositorBridge)";
> +      texture->Destroy();
> +    }
> +  }

You need to move this up a bit, just before SendWillClose()

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ +2762,5 @@
> +    // The compositor was recreated, and we're receiving layers updates for a
> +    // a layer manager that will soon be discarded or invalidated. We can't
> +    // return null because this will mess up deserialization later and we'll
> +    // kill the content process. Instead, we signal that the underlying
> +    // TextureHost should not attempt to access the compositor.

This one is a tad tricky. The solution (short term maybe), could be to pass the PLayerTransaction as a parameter of the PTextureParent constructor. Not sure how this plays out when PTexture is not restricted to a given PLayerTransction. You should ask dvander.

@@ +2768,5 @@
> +  } else if (aLayersBackend != mLayerManager->GetCompositor()->GetBackendType()) {
> +    gfxDevCrash(gfx::LogReason::PAllocTextureBackendMismatch) << "Texture backend is wrong";
> +  }
> +
> +  return nullptr; // FIXME: TextureHost::CreateIPDLActor(this, aSharedData, aLayersBackend, flags);

Ah I see, this is annoying because CompositorBridgeParent is not an ISurfaceAllocator. I think you'll need to make CompositorBridgeParent inherit ISurfaceAllocator (and PLayerTransactionParent will probably not need to implement it then). It also needs to implement ShmemAllocator.
Implementing these two interfaces is rather easy, mostly forwarding calls to some IPDL generated stuff like AllocShmem, etc.

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +1060,4 @@
>        !mShadowManager->IPCOpen()) {
>      return nullptr;
>    }
> +  // FIXME: return mShadowManager->SendPTextureConstructor(aSharedData, aLayersBackend, aFlags);

mShadowManager is a PLayerTransactionChild managed by a PCompositorBridgeChild, there's an ipdl generated method to get access to the manager.
Attachment #8741477 - Flags: feedback?(nical.bugzilla) → feedback+
Updated patch that actually works now.

David, can you comment on what needs to be done to handle texture clients across device resets? You pondered that maybe the texture clients could store a generation id; have you had a chance to think more about it and how it would work in practice?
Attachment #8750594 - Flags: feedback?(dvander)
Blocks: 1176011
Comment on attachment 8750594 [details] [diff] [review]
0001-attempt-at-moving-PTexture-under-CompositorBridge.patch

Review of attachment 8750594 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ +2279,5 @@
> +                                            const TextureFlags& aFlags)
> +{
> +  TextureFlags flags = aFlags;
> +
> +  if (/*FIXME: mPendingCompositorUpdates*/false) {

I think we can move this check into the CrossProcessCompositorBridgeParent version of this function, and remove the mPendingCompositorUpdates business from LayerTransactionParent.

The reason is we currently don't care about stale layers updates from the parent process. Maybe we should, but we don't yet, so just moving the check should preserve all the existing behavior.
Attachment #8750594 - Flags: feedback?(dvander) → feedback+
try push here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=50cefbb00ccc

The shutdown crash I had turned out to be due to getting refcounting through both NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_MAIN_THREAD_DESTRUCTION in (CrossProcess)CompositorBridgeParent and ISurfaceAllocator, which caused inconsistencies. I fixed this by removing NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_MAIN_THREAD_DESTRUCTION and instructing AtomicAddRefWithFinalize (via ISurfaceAllocator) to destruct on the main thread, in the same way that ImageBridge(Child,Parent) operate.
Attachment #8741477 - Attachment is obsolete: true
Attachment #8750594 - Attachment is obsolete: true
Attachment #8751562 - Flags: review?(nical.bugzilla)
Attachment #8751562 - Flags: review?(dvander)
Attachment #8751562 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8751562 [details] [diff] [review]
0001-attempt-at-moving-PTexture-under-CompositorBridge.patch

Review of attachment 8751562 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. LayersId usage looks okay - r=me with the state thing below fixed.

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ +2867,5 @@
> +    // return null because this will mess up deserialization later and we'll
> +    // kill the content process. Instead, we signal that the underlying
> +    // TextureHost should not attempt to access the compositor.
> +    flags |= TextureFlags::INVALID_COMPOSITOR;
> +  } else if (aLayersBackend != state->mLayerManager->GetCompositor()->GetBackendType()) {

if |state| is null, this will crash. I'd change "state && state->mPendingCompositorUpdates" to "!state || state->mPendingCompositorUpdates".
Attachment #8751562 - Flags: review?(dvander) → review+
https://hg.mozilla.org/mozilla-central/rev/dc0186974f99
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: