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.

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.

Basically the same as we do with CompositorParent in nsBaseWidget.

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?

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 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 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.

(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)

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.

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 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 on attachment 810974 [details] [diff] [review] Release CompositableClients on the right thread v2 Review of attachment 810974 [details] [diff] [review]: ----------------------------------------------------------------- awesome

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

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

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

Comment on attachment 825440 [details] [diff] [review] Avoid introducing a global constructor Review of attachment 825440 [details] [diff] [review]: ----------------------------------------------------------------- First!

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