Matt Woodrow (:mattwoodrow) Assignee
	Description • 8 years ago

As discussed in bug 1288618, we don't want to share layers::Image directly with the compositor.

We should instead create a new protocol for sharing PTexture (VideoBridge?) and share the textures that way.

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 3 • 8 years ago

Comment on attachment 8792703 [details] [diff] [review]
Add PVideoBridge to share textures with the compositor

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

r=me w/ comments addressed

::: dom/media/ipc/VideoDecoderParent.cpp
@@ +177,5 @@
>                                         data->mDuration,
>                                         data->mFrames,
>                                         data->mKeyframe),
>                           video->mDisplay,
> +                         texture ? self->mParent->StoreImage(texture) : SurfaceDescriptorGPUVideo(0),

|texture| is used unconditionally above, so can it really be null here?

::: gfx/layers/ipc/VideoBridgeChild.cpp
@@ +14,5 @@
> +
> +/* static */ void
> +VideoBridgeChild::Startup()
> +{
> +  sVideoBridgeChildSingleton = new VideoBridgeChild();

Is there any way to assert that this happens on a specific thread?

@@ +38,5 @@
> +}
> +
> +VideoBridgeChild::~VideoBridgeChild()
> +{
> +  sVideoBridgeChildSingleton = nullptr;

sVideoBridgeChildSingleton holds a ref to this. Is this line supposed to be in Shutdown?

::: gfx/layers/ipc/VideoBridgeParent.cpp
@@ +45,5 @@
> +VideoBridgeParent::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  // Can't alloc/dealloc shmems from now on.
> +  mClosed = true;
> +  mSelfRef = nullptr;

Better to do this mSelfRef = nullptr in DeallocPVideoBridgeParent. Just for consistency with other protocols where it's necessary.

@@ +113,5 @@
> +
> +void
> +VideoBridgeParent::NotifyNotUsed(PTextureParent* aTexture, uint64_t aTransactionId)
> +{
> +  RefPtr<TextureHost> texture = TextureHost::AsTextureHost(aTexture);

Can you assert this happens on the IPDL thread? Usually these "Actor to C++ Owner" casts are inherently racy, so we want to make sure they're being used correctly.

@@ +123,5 @@
> +     !texture->NeedsFenceHandle()) {
> +    return;
> +  }
> +
> +  MOZ_ASSERT(false, "Async messages not supported");

Should this be MOZ_ASSERT_UNREACHABLE?

	Pulsebot
	Comment 4 • 8 years ago

Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4457a0428b
Add PVideoBridge to share textures with the compositor. r=dvander,nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeb3209d58fe
Remove old Compositor sharing code. r=nical

	Carsten Book [:Tomcat]
	Comment 6 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2b4457a0428b
https://hg.mozilla.org/mozilla-central/rev/aeb3209d58fe

	Carsten Book [:Tomcat]
	Comment 7 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2b4457a0428b
https://hg.mozilla.org/mozilla-central/rev/aeb3209d58fe

	Gerry Chang [:gchang]
	Comment 9 • 8 years ago

Mark 51 as fix-optional. If it's worth uplift to 51, feel free to nominate it.