889959 - Only upload image layers of identical images once

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Matt Woodrow tells me that we have this optimization for non-OMTC but not in the OMTC case.

Comment 1

[Matt Woodrow (:mattwoodrow)]

CC'ing nical, since this also definitely touches code that he's refactoring.

When we have the same image being used in multiple ImageLayers, then they will all share the same ImageContainer/Image.

I think we want to make the ImageContainer own both the TextureClient, and the mLastPaintedSerial (both currently owned by ImageClient).

That way multiple ImageClients can reference the same TextureClient, and we avoid duplicating lots of texture memory.

Comment 2

[Nicolas Silva [:nical]]

I have been thinking about this lately and as you said, it boils down to making it possible to share textures between compositables (as opposed to the current situation of textures being owned by one compositable). We also want this for the furure "texture atlas" optimization where small image layers's content should be in one texture, and for video streams that output to several ImageContainers at the same time.
I'd like to resurect the PTexture protocol for this. this way TextureClient/Host could outlive compositables, more importantly the IPC stuff would use the PTexture rather than {PCompositable + id} to refer to textures, and that would make sharing textures between layers easy.
I think mLastPaintedSerial should rather move into TextureClient.

Comment 3

[Nick Cameron [:nrc]]

(In reply to Nicolas Silva [:nical] from comment #2)
> I'd like to resurect the PTexture protocol for this. this way

/me runs screaming

Comment 4

[Matt Woodrow (:mattwoodrow)]

I'd quite like to take this now that we have PTexture again.

I think what needs to be done is:

* Fix bug 926745 - removing ForceRemove() calls
* Rebase and land bug 912897
* Implement the ISharedImage interface for CairoImage


A couple of questions:

CreateBufferTextureClient (which is what we want for CairoImage I believe) requires a CompositableClient parameter in the constructor which is used for Shmem allocations. Would it make sense to pass this (or just the allocator) as a param to ISharedImage::GetTextureClient? This way we can lazily construct the TextureClient once we have an allocator.

It looks like the ISharedImage design is based on each Image* only ever being used with one compositor connection. I *think* this could break fairly easily on desktop if you had the same video/image in multiple windows (will try test this today). We probably need a separate TextureClient (or just TextureChild) for each compositor connection. I guess if GetTextureClient() takes a compositable/forwarder/allocator parameter than we can look in a map for an existing texture client for that connection or create a new one. Gets pretty complicated when the underlying object isn't refcounted though, having it shared with two separate IDPL objects puts us back into a crazy liftetime management mess. Thoughts?

Sorry for the ni? spam :)

Comment 5

[Matt Woodrow (:mattwoodrow)]

I have a prototype of the above, not including handling multiple compositor connections.

And I can confirm that it's a real issue, getting an active ImageLayer with the same source image in two windows crashes with an IPDL error.

Comment 6

[Nicolas Silva [:nical]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> I'd quite like to take this now that we have PTexture again.
> 
> I think what needs to be done is:
> 
> * Fix bug 926745 - removing ForceRemove() calls

yeah I testes locally some time ago and you basically just need to replace ForceRemove by something like ReleaseAfterTransaction which just holds the reference until the the end of the transaction, except for FlushAllImages where we need to get back our gralloc buffers asap.

> CreateBufferTextureClient (which is what we want for CairoImage I believe)
> requires a CompositableClient parameter in the constructor which is used for
> Shmem allocations. Would it make sense to pass this (or just the allocator)
> as a param to ISharedImage::GetTextureClient? This way we can lazily
> construct the TextureClient once we have an allocator.

Sounds like a good idea.

> 
> It looks like the ISharedImage design is based on each Image* only ever
> being used with one compositor connection. I *think* this could break fairly
> easily on desktop if you had the same video/image in multiple windows (will
> try test this today). We probably need a separate TextureClient (or just
> TextureChild) for each compositor connection.

You will need a TextureClient/Host pair for each IPC channel. You won't be able to share a shmem or an actor through several ipc channels at the same time.

> I guess if GetTextureClient()
> takes a compositable/forwarder/allocator parameter than we can look in a map
> for an existing texture client for that connection or create a new one. Gets
> pretty complicated when the underlying object isn't refcounted though,
> having it shared with two separate IDPL objects puts us back into a crazy
> liftetime management mess. Thoughts?

I would really like to avoid having a buffer used by several TextureClient/Host pairs. That would just undo the every assumption under which TextureClient was designed and implemented.

Maybe (as a first step) we should copy the texture if it is shared between several windows? most of the optimizations we really want are within a given page in one window (so in the same layer manager), unless we really want to be very good at rendering the same tumblr pages (or video) on two windows at the same time which doesn't look that useful.
I don't think the effort required to share the same texture through several channels is worth the gain.

And, in the case where we render the same video on two windows, don't we have two distinct dome trees with distinct HTMLMediaElements generating separate video frames?

> 
> Sorry for the ni? spam :)

No problem :)

Comment 7

[Matt Woodrow (:mattwoodrow)]

I think there's quite a few scary corner cases here :(

What if we close the window, but the Image (along with the TextureClient and TextureClient) outlive it?

What if an Image that was previously used for async-video (through ImageBridge) tries to get used for a normal layer transaction?

The IsSharedWithCompositor() check is probably shielding us from all of these at the moment, but we're just not rendering instead.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: WIP

This is close, definitely uses the same Texture for the same images used in multiple places, even across windows.

I think the CompositableForwarder destructor is too late to do cleanup though, we're getting a warning from the AncestorDestroyed path in TextureHost cleanup. It should at least protect us against pointer re-use bugs with the hashtable.

nical, what do you think about this apporach?

Comment 9

[Nicolas Silva [:nical]]

The warning is because of another bug that I'll fix as soon as I land the new D3D11 textures.

(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> I think there's quite a few scary corner cases here :(
> 
> What if we close the window, but the Image (along with the TextureClient and
> TextureClient) outlive it?

If you close the channel, it will force the deallocation of the texture client's data. So the TextureClient will be marked as Invalid, and future attempts to lock it will fail.

If there exists cases where the texture memory could be used by other windows that have a different channel, then we need a way to copy the texture's data before it gets destroyed.

But when would we wind up in such a situation?

> 
> What if an Image that was previously used for async-video (through
> ImageBridge) tries to get used for a normal layer transaction?

The TextureClient should be copied into another texture client that is on the right IPC channel if the existing TextureClient's forwarder is different from the one that we are trying to put the texture into.


IIUC, your patch is lazily creating a TextureClient for each CompositableForwarder that the CairoImage encounters (I suppose in the vast majority of cases there is only one). And the CairoImage also has it's own buffer for the data, which is not shared with the compositor.

It would be even better if the CairoImage was already using a TextureClient (like SharedRGBImage and SharedPlanarYCbCrImage do) to store the image, in order to save the first copy. Then it would eventually copy the texture client into another texture client if it sees a different forwarder.
This way we'd save one buffer and a copy. This could be a followup, what do you think?

Comment 10

[Nicolas Silva [:nical]]

The shutdown observers business looks very complicated. Do we really need it ?

Comment 11

[Matt Woodrow (:mattwoodrow)]

(In reply to Nicolas Silva [:nical] from comment #9)
> > What if we close the window, but the Image (along with the TextureClient and
> > TextureClient) outlive it?
> 
> If you close the channel, it will force the deallocation of the texture
> client's data. So the TextureClient will be marked as Invalid, and future
> attempts to lock it will fail.

Nice :)
> 
> If there exists cases where the texture memory could be used by other
> windows that have a different channel, then we need a way to copy the
> texture's data before it gets destroyed.
> 
> But when would we wind up in such a situation?

Well, with the current implementation of CairoImage, we can't. Other Image types that share the only copy of the data (like SharedRGB etc), we could, but we haven't implemented multiple TextureClients for those yet. I believe video will never put us in this situation, and that's the only producer for the relevant Image types. I guess it's possible we might change something in the future that means we do get into this.


> > What if an Image that was previously used for async-video (through
> > ImageBridge) tries to get used for a normal layer transaction?
> 
> The TextureClient should be copied into another texture client that is on
> the right IPC channel if the existing TextureClient's forwarder is different
> from the one that we are trying to put the texture into.

Sweet.

> 
> 
> IIUC, your patch is lazily creating a TextureClient for each
> CompositableForwarder that the CairoImage encounters (I suppose in the vast
> majority of cases there is only one). And the CairoImage also has it's own
> buffer for the data, which is not shared with the compositor.
> 
> It would be even better if the CairoImage was already using a TextureClient
> (like SharedRGBImage and SharedPlanarYCbCrImage do) to store the image, in
> order to save the first copy. Then it would eventually copy the texture
> client into another texture client if it sees a different forwarder.
> This way we'd save one buffer and a copy. This could be a followup, what do
> you think?

I absolutely agree! I don't think it'll be all that easy though. The majority of images that ImageLib decodes won't ever get wrapped in a CairoImage, it's a relatively rare optimization, unlike layerizing of video. I very much doubt we want to decode all images in a child process into shmem, so we'd need some way to know how the image might be layerized in the future. We don't have that information available at the moment, so an interesting problem :)


(In reply to Nicolas Silva [:nical] from comment #10)
> The shutdown observers business looks very complicated. Do we really need it
> ?

I think we do yes. Open the same page (that has a layerized image) in two windows, so we get a CairoIamge with two TextureClients. Then close one of the windows. Without a shutdown observer, then we leak the TextureClient* (the buffer gets cleaned up, but the object itself won't). We also have a stale pointer to a CompositableForwarder as our hash key, which puts us at risk of pointer re-use bugs.

We could put the observer on the TextureClient instead? So when the channel closes, and we cleanup the buffer, we also optionally send notifications to any owners of TextureClients to tell them that their TextureClient is now invalid.

Comment 12

[Nicolas Silva [:nical]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> I absolutely agree! I don't think it'll be all that easy though. The
> majority of images that ImageLib decodes won't ever get wrapped in a
> CairoImage, it's a relatively rare optimization, unlike layerizing of video.
> I very much doubt we want to decode all images in a child process into
> shmem, so we'd need some way to know how the image might be layerized in the
> future. We don't have that information available at the moment, so an
> interesting problem :)

Ah, makes sense. We could hijack the buffer in non-cross-process cases and use MemoryTextureClient. But I wouldn't bother if it's a rare optimization.

> 
> 
> (In reply to Nicolas Silva [:nical] from comment #10)
> > The shutdown observers business looks very complicated. Do we really need it
> > ?
> 
> I think we do yes. Open the same page (that has a layerized image) in two
> windows, so we get a CairoIamge with two TextureClients. Then close one of
> the windows. Without a shutdown observer, then we leak the TextureClient*

You have a string reference to a TextureClient that will go away with the CairoImage eventually, no?

> (the buffer gets cleaned up, but the object itself won't). We also have a
> stale pointer to a CompositableForwarder as our hash key, which puts us at
> risk of pointer re-use bugs.

That's more annoying. Maybe we should identify Forwarders with a 64bit ID instead of just their pointer if we are at risk of pointer reuse bugs.

> 
> We could put the observer on the TextureClient instead? So when the channel
> closes, and we cleanup the buffer, we also optionally send notifications to
> any owners of TextureClients to tell them that their TextureClient is now
> invalid.

What worries me a bit is that having bi-directional references with a strong ref and a weak ref often ends up hard to maintain, especially with TextureClient which may be touched by several threads. If there is no other way, then so be it, but if we can manage with just keeping an empty TextureClient in the CairoImage until the CairoImage dies, then it's not too bad on our memory footprint. As long as users of TextureClient appropriately Lock/Unlock and handle locking failure, it's safe to have the channel closed under our feet.
If we can avoid complicated solutions for rare optimization it'd be nice.

Another question:
When we have a CairoImage, is it going through ImageClient::UpdateImage several times? If not we can just have the ImageClient check whether the forwarder is the same, copy into a new TextureClient and keep the copy (which will be thrown away or rewritten next time UpdateImage is called, though).

Comment 13

[Matt Woodrow (:mattwoodrow)]

(In reply to Nicolas Silva [:nical] from comment #12)

> 
> Ah, makes sense. We could hijack the buffer in non-cross-process cases and
> use MemoryTextureClient. But I wouldn't bother if it's a rare optimization.

Indeed, Shmem was the worst case.

> 
> You have a string reference to a TextureClient that will go away with the
> CairoImage eventually, no?

Eventually could be a long time away :) So, yes, that's true. But we could have arbitrarily many windows opened and closed before that happens.

I guess it's hard to believe we'd leak any consequential amount of memory this way, but it still seems bad to intentionally 'leak' memory.


> 
> What worries me a bit is that having bi-directional references with a strong
> ref and a weak ref often ends up hard to maintain, especially with
> TextureClient which may be touched by several threads. If there is no other
> way, then so be it, but if we can manage with just keeping an empty
> TextureClient in the CairoImage until the CairoImage dies, then it's not too
> bad on our memory footprint. As long as users of TextureClient appropriately
> Lock/Unlock and handle locking failure, it's safe to have the channel closed
> under our feet.
> If we can avoid complicated solutions for rare optimization it'd be nice.
> 
> Another question:
> When we have a CairoImage, is it going through ImageClient::UpdateImage
> several times? If not we can just have the ImageClient check whether the
> forwarder is the same, copy into a new TextureClient and keep the copy
> (which will be thrown away or rewritten next time UpdateImage is called,
> though).

That would work, yes. We'd be making a copy of it every frame though if the same layerized images exists in two windows.

Comment 14

[Nicolas Silva [:nical]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> (In reply to Nicolas Silva [:nical] from comment #12)
> 
> Eventually could be a long time away :) So, yes, that's true. But we could
> have arbitrarily many windows opened and closed before that happens.
> 
> I guess it's hard to believe we'd leak any consequential amount of memory
> this way, but it still seems bad to intentionally 'leak' memory.

An empty TextureClient is really not a lot of memory, so I would much rather keep it longer than needed (provided that we do deallocate it at some point along with the CairoImage) than making the system more complex by registering observers and keeping two way references (in a thread safe way).

> > Another question:
> > When we have a CairoImage, is it going through ImageClient::UpdateImage
> > several times? If not we can just have the ImageClient check whether the
> > forwarder is the same, copy into a new TextureClient and keep the copy
> > (which will be thrown away or rewritten next time UpdateImage is called,
> > though).
> 
> That would work, yes. We'd be making a copy of it every frame though if the
> same layerized images exists in two windows.

What's every frame, though? If the content of the Image changed, we'll need to copy it anyway, and if it didn't change, then maybe we should not be calling ImageClient::UpdateImage?

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Implement GetTextureClient for CairoImage

Comment 16

[Matt Woodrow (:mattwoodrow)]

(In reply to Nicolas Silva [:nical] from comment #14)
> 
> An empty TextureClient is really not a lot of memory, so I would much rather
> keep it longer than needed (provided that we do deallocate it at some point
> along with the CairoImage) than making the system more complex by
> registering observers and keeping two way references (in a thread safe way).

Ok, lets try the simple way and we can revisit this if it shows up in a bad way.

> 
> What's every frame, though? If the content of the Image changed, we'll need
> to copy it anyway, and if it didn't change, then maybe we should not be
> calling ImageClient::UpdateImage?

Well, I was thinking of an animated GIF that would keep changing the current Image in an ImageContainer, but often not to a new one.

Comment 17

[Matt Woodrow (:mattwoodrow)]

This fails a few tests involving mask layers.

Looks like we use the same Texture for multiple mask layers, and their lifetime overlaps. When we tear down the first one, we call CompositableHost::Detach() -> ImageHost::SetCompositor(nullptr) -> TextureHost::SetCompositor(nullptr).

This clears the compositor on the texture host, even though it's still attached to another compositable.

Then we try fail out of BufferTextureHost::Upload() since we don't have a compositor.

Nical: Any ideas how to fix this?

Do TextureHost's actually depend on having only one Compositor instance? Or do they just need any Compositor, as long as the type of it doesn't change?

Comment 18

[Nicolas Silva [:nical]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> This fails a few tests involving mask layers.
> 
> Looks like we use the same Texture for multiple mask layers, and their
> lifetime overlaps. When we tear down the first one, we call
> CompositableHost::Detach() -> ImageHost::SetCompositor(nullptr) ->
> TextureHost::SetCompositor(nullptr).
> 
> This clears the compositor on the texture host, even though it's still
> attached to another compositable.
> 

Hm. I think the whole clearing out the compositor when detaching was me conservatively making sure we would deallocate the texture data before destroying the widget and its underlying device/context. Later on we fixed the "having a texture alive after the context is dead" issue some other way, so clearing out the compositor might not be needed anymore.

> Then we try fail out of BufferTextureHost::Upload() since we don't have a
> compositor.
> 
> Nical: Any ideas how to fix this?

Try to not clear the compositor anymore

> 
> Do TextureHost's actually depend on having only one Compositor instance? Or
> do they just need any Compositor, as long as the type of it doesn't change?

They only depend on not having, say, a gl texture be used with a gl context that is not compatible with it.

Comment 19

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't clear compositor when detaching compositables

Indeed, it seems to work!

Comment 20

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e25912d6696
https://hg.mozilla.org/integration/mozilla-inbound/rev/34c95e937671

Comment 21

[Phil Ringnalda (:philor)]

Backed out along with bug 972703 in https://hg.mozilla.org/integration/mozilla-inbound/rev/89b9078be02a - between the two there's a b2g build failure, https://tbpl.mozilla.org/php/getParsedLog.php?id=34764895&tree=Mozilla-Inbound, and an assertion, https://tbpl.mozilla.org/php/getParsedLog.php?id=34765199&tree=Mozilla-Inbound / https://tbpl.mozilla.org/php/getParsedLog.php?id=34764908&tree=Mozilla-Inbound, to deal with.

Comment 22

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/405f0e05bf56
https://hg.mozilla.org/integration/mozilla-inbound/rev/fccf9c2aee88

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/405f0e05bf56
https://hg.mozilla.org/mozilla-central/rev/fccf9c2aee88