886219 - Get async-video enabled on OSX

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Matt Woodrow (:mattwoodrow)]

https://tbpl.mozilla.org/?tree=Try&rev=2d193e892924

Looks like 3 different leaks, a reftest failure and an abort on M1.

I have patches for some of these, working on it.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Force an invalidation when the video ends

The reftest failure is because the video plays async, and nothing ever notifies the reftest harness that it needs to take a new snapshot.

This adds a forced invalidation when the video ends, so that we get a layers transaction (and MozAfterPaint) event.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Release the ImageBridgeParent

Basically the same as we do with CompositorParent in nsBaseWidget.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Always release the YCbCr image

Note: This is obviously wrong, and will crash when we're running ImageBridgeChild from a different process.

The problem here is that we often try to release these images from the compositor side. The compositable is double buffered, so most frames get passed back to the ImageBridgeChild to be released correctly.

The very last frame (when we kill the texture host) is still owned by the compositor, and currently we just leak it.

This patch fixes the leak for in-process ImageBridge, but we need something better for cross-process. Ideas?

Comment 4

[Matt Woodrow (:mattwoodrow)]

I haven't been able to reproduce the M1 crash, but I believe it's the same issue as the third patch.

We try release the YCbCr image on the compositor side, and since we skip over the owner check, we just release it. We then get a bad shmem error, which I assume is because it's already been released.

It's not obvious how we actually release the shmem while still holding a reference to the SharedPlanarYCbCr image though. I might be missing something.

Comment 5

[Nicolas Silva [:nical]]

Comment on attachment 766534 [details] [diff] [review]
Force an invalidation when the video ends

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

::: content/media/VideoFrameContainer.h
@@ +59,5 @@
> +  enum {
> +    INVALIDATE_DEFAULT,
> +    INVALIDATE_FORCE
> +  };
> +  void Invalidate() { InvalidateWithFlags(INVALIDATE_DEFAULT); }

nit: a little comments about what this does and how DEFAULT behaves differently than FORCE would be nice.

Comment 6

[Nicolas Silva [:nical]]

Comment on attachment 766536 [details] [diff] [review]
Always release the YCbCr image

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

If you do this you will deadlock sometimes. ReleaseOwnedSurfaceDescriptor will internally dispatch a synchronous task on the ImageBridgeChild thread so you can't call it from the parent side. In any case, releasing a shared YCbCrImage from the parent side is dangerous because there can be one or several references to the image on the child side on several different threads. This awful situation is one of the reasons for the texture client/host rewrite that I am working on.

Comment 7

[Nicolas Silva [:nical]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> It's not obvious how we actually release the shmem while still holding a
> reference to the SharedPlanarYCbCr image though. I might be missing
> something.

iirc we release it by dropping the reference to the SharedPlanarYCbCrImage, and the shmem is destroyed by something that is triggered by the SharedPlanarYCbCrImage's destructor.

In bug 858914 I am making it so when you destroy the TextureClient that owns the Shmem you have the insurance that the shmem will be destroyed, which is tricky to do right now because TextureClient doesn't own the shmem. So it will fix this problem (which greatly motivated bug 858914 in the first place)

Comment 8

[Matt Woodrow (:mattwoodrow)]

The final leak (a few gfxImageSurfaces) appear to also be the same bug :)

It's leaking the mSurface image on a SharedPlanarYCbCrImage.

Nical: I'll mark bug 858914 as blocking this one, and hopefully it resolves these issues.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Release CompositableClients on the right thread

https://tbpl.mozilla.org/?tree=Try&rev=74a73cd2da42

The only bug that shows up is the assertion crash.

From what I can tell, we can't release the ImageClient (created by the ImageBridgeChild) on the main thread.

This patch forces us to always release it from the image bridge thread, and seems to fix the issue.

Just f? for now since it needs more cleanup before I could land it.

Comment 10

[Nicolas Silva [:nical]]

Comment on attachment 810367 [details] [diff] [review]
Release CompositableClients on the right thread

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

::: gfx/layers/ImageContainer.cpp
@@ +149,5 @@
>  
>    if (mImageClient) {
>      nsRefPtr<Image> img = mImageClient->CreateImage((uint32_t*)aFormats,
> +                                                            aNumFormats,
> +                                                            true);

I am not super found of adding a boolean parameter for async stuff here.

Actually you don't need to store this information, please check for (mImageClient->GetAsyncID() != 0) instead, non-zero async id means the ImageClient is used by ImageBridge

::: gfx/layers/client/ImageClient.cpp
@@ +490,3 @@
>          return img.forget();
>        case SHARED_RGB:
>          img = new SharedRGBImage(this);

You should provide the same fix for SharedRGBImage too

::: gfx/layers/ipc/SharedPlanarYCbCrImage.cpp
@@ +37,5 @@
>  
>  SharedPlanarYCbCrImage::~SharedPlanarYCbCrImage() {
>    MOZ_COUNT_DTOR(SharedPlanarYCbCrImage);
> +
> +  if (mAsync) {

&& !InImageBridgeChildThread()

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Release CompositableClients on the right thread v2

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Enable async-video

https://tbpl.mozilla.org/?tree=Try&rev=423d6e0aa65f

Comment 13

[Nicolas Silva [:nical]]

Comment on attachment 810974 [details] [diff] [review]
Release CompositableClients on the right thread v2

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

awesome

Comment 14

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f78208b337db
https://hg.mozilla.org/integration/mozilla-inbound/rev/1af25db786e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/663ec5436747
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f7917ea7b9f

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f78208b337db
https://hg.mozilla.org/mozilla-central/rev/1af25db786e4
https://hg.mozilla.org/mozilla-central/rev/663ec5436747
https://hg.mozilla.org/mozilla-central/rev/5f7917ea7b9f

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Avoid introducing a global constructor

One review will be enough, whoever does it first clears the other review flag.

Comment 17

[Nicolas Silva [:nical]]

Comment on attachment 825440 [details] [diff] [review]
Avoid introducing a global constructor

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

First!

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

Great! Now all I have to do is wait for inbound to reopen!

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/51f057783123

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/51f057783123