695845 - Add ImageContainer to ShadowLayers

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

We should start mirroring ImageContainer (I don't think we need a separate class for Image) through IPC.

Currently we have the performance bug where we always draw the ImageContainer into the ImageLayer buffer even if it hasn't changed. We could attempt to track this with timestamps etc, but it gets more complicated if multiple layers are using the same ImageContainer.

Bug 695610 lets us share ImageContainers between multiple ImageLayers, and it would be nice to take advantage of this possible memory improvement.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: PLayers protocol changes

Chris: Assuming you like this idea, do the above changes to the PLayers protocol seem like a good start?

The only part I'm not sure about would be sending the OpPaintImageContainer from ImageContainer::SetImage. Can we send these messages at any time or does it need to be within a paint?

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> Created attachment 568342 [details] [diff] [review] [diff] [details] [review]
> PLayers protocol changes
> 
> The only part I'm not sure about would be sending the OpPaintImageContainer
> from ImageContainer::SetImage. Can we send these messages at any time or
> does it need to be within a paint?

Doesn't that happen off the main thread with video?  Can't do that.  You can send SetImage whenever you want.  Instead of re-using PLayers:Update which processes lots of stuff per transaction, you might want to use a new message that directly corresponds to SetImage.  One potential problem with doing that is that ImageLayer changes might not be applied atomically with changes from other sibling layers.

Is it possible to do something like mark ImageContainers as "dirty" when there's a SetImage and then unset the dirty flag in ::Paint, and not do the copy when the container isn't dirty?  That seems simpler.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Add ImplData to ImageContainer

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Add ShadowImplData

Separates the Shadow specific code from BasicImplData and uses it in BasicImageContainer

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Add Flush() and Disconnect() methods to ImageContainer

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Move BasicShadowableLayer into ImageLayers.h

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Add Shadow(able)ImageContainer

The main piece of work for this bug.

I ran out of ideas to split this up into more manageable pieces that still compile/work standalone. Will happily do this if anyone has ideas.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 569584 [details] [diff] [review]
Part 3: Add Flush() and Disconnect() methods to ImageContainer

>diff --git a/gfx/layers/ImageLayers.h b/gfx/layers/ImageLayers.h
 
>   /**
>+   * Make sure the current image is ready for rendering by this container.
>+   * May only be called from the main thread.
>+   */
>+  virtual void Flush() {}

Flush() isn't terribly descriptive ... how about EnsureCurrentImage()
or something along those lines?  Better ideas welcome.

r=me with more descriptive name.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 569586 [details] [diff] [review]
Part 5: Add Shadow(able)ImageContainer

>+class BasicShadowableImageContainer : public BasicImageContainer,
>+                                      public BasicShadowableLayer

Is there any reason to have a separate BasicImageContainer?  Can all
BasicImageContainers be shadowable?  The *Shadowable*Layer gunk was
created back in the dark days of MOZ_IPC, when we needed to be able to
build the layers code without shadow layers.  That restriction is
gone, so we don't need to perpetuate that crud unless there's a good
reason to.

>+void
>+BasicShadowableImageContainer::Flush()
>+{
>+  NS_ASSERTION(BasicManager()->HasShadowManager(), "Can only flush ImageContainers when shadowing!");
>+  if (!mCreatedShadow) {
>+    PLayerChild* shadow = BasicManager()->ConstructShadowFor(this);
>+    // XXX error handling
>+    NS_ABORT_IF_FALSE(shadow, "failed to create shadow");
>+
>+    SetShadow(shadow);
>+    BasicManager()->CreatedImageContainer(this);
>+    BasicManager()->Hold(this);
>+    mCreatedShadow = true;
>+  }
>+
>+  if (!mImage || !mDirty) {

Need to hold the monitor while accessing mImage, right?

>diff --git a/gfx/layers/basic/BasicLayers.cpp b/gfx/layers/basic/BasicLayers.cpp

>-class BasicShadowImageLayer : public ShadowImageLayer, public BasicImplData {
>+class BasicShadowImageContainer : public ShadowImageContainer {

It feels a bit awkward to define BasicImageContainer in
BasicImages.cpp but BasicShadowImageContainer in BasicLayers.cpp.  Was
there something that made doing that unpalatable?

> void
> BasicShadowImageLayer::Paint(gfxContext* aContext)
> {
>-  if (!IsSurfaceDescriptorValid(mFrontBuffer)) {
>+#if 0

Que?

>diff --git a/gfx/layers/basic/BasicLayers.h b/gfx/layers/basic/BasicLayers.h
>--- a/gfx/layers/basic/BasicLayers.h
>+++ b/gfx/layers/basic/BasicLayers.h
>@@ -33,24 +33,25 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #ifndef GFX_BASICLAYERS_H
> #define GFX_BASICLAYERS_H
> 
>+

Nix this plz.

>diff --git a/gfx/layers/opengl/LayerManagerOGL.h b/gfx/layers/opengl/LayerManagerOGL.h

>+  virtual already_AddRefed<ShadowImageContainer> CreateShadowImageContainer() { return NULL; }
> 

Is the implementation for this coming up soon?  We'd like to get GL
compositing on for android soon.  I'm somewhat loath to land these
patches without GL support, but if it's a big enough win OK.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Add Shadow(able)ImageContainer v2

Fixed review comments, added OGL support and deleted lots of unneeded code.

Comment 11

[Matt Woodrow (:mattwoodrow)]

We still have a shutdown crash with this patch (and the patches from bug 695610) because imgIContainer is holding onto an ImageContainer reference too long.

Does anybody know the lifetime of imgIContainer, and have ideas as to how we could force this reference to be released before we close the IPC pipe?

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 570174 [details] [diff] [review]
Part 5: Add Shadow(able)ImageContainer v2

>diff --git a/gfx/thebes/gfxUtils.cpp b/gfx/thebes/gfxUtils.cpp
>     gfxContext::GraphicsOperator op = aContext->CurrentOperator();
>     if ((op == gfxContext::OPERATOR_OVER || workaround.PushedGroup()) &&
>         aFormat == gfxASurface::ImageFormatRGB24) {
>-        aContext->SetOperator(OptimalFillOperator());
>+        //aContext->SetOperator(OptimalFillOperator());

What's this?

The rest looks ok, r=me with that sorted out.

Comment 13

[Joe Drew (not getting mail)]

imgIContainer lives as long as there is a reference to the image anywhere on any web page, and additionally will persist references longer in the image cache (LRU-style).

How is the ImageContainer pointer living longer than the object? Isn't the reference added in bug 695610 strong?

Comment 14

[Matt Woodrow (:mattwoodrow)]

I don't think the ImageContainer is living longer than the imgIContainer, but instead the imgIContainer (and thus ImageContainer) is living longer than the layer system.

The shadow layer shutdown code expect all shadowable objects (the ImageContainer) to have been destroyed at this point.

We either need a way to signal all imgIContainers to drop their cached ImageContainer references, make sure all imgIContainers are destroyed at this point, or remove this expectation.

The first is definitely the preferred option.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Shutdown Imagelib and clear cached ImageContainers

Does this make sense (for some definition of sense)?

Obviously the imglib_Shutdown() declaration needs to go somewhere more sensible, ideas?

Comment 16

[Joe Drew (not getting mail)]

Comment on attachment 578958 [details] [diff] [review]
Shutdown Imagelib and clear cached ImageContainers

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

I realize that at some point I probably said you should shut down imagelib from the shutdown of another module, but oh my goodness I take it back. Is there some way we can enforce ordering? Or maybe just have a call that invalidates all the ImageContainer pointers from within Layers shutdown?

Comment 17

[Benoit Girard (:BenWa)]

Landed p1-p4 to facilitate OMTC work. Thanks Matt!

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9a5736cd6bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/86e91202ef51
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8004eb22252
https://hg.mozilla.org/integration/mozilla-inbound/rev/76dcfabe0a2a

Comment 18

[Ed Morley [:emorley]]

Backed out for build failures on Windows:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de8dffddb799

https://tbpl.mozilla.org/php/getParsedLog.php?id=8038483&tree=Mozilla-Inbound
{
e:/builds/moz2_slave/m-in-w32-dbg/build/gfx/layers/d3d9/ImageLayerD3D9.cpp(143) : error C2668: 'mozilla::layers::ImageContainer::ImageContainer' : ambiguous call to overloaded function
e:\builds\moz2_slave\m-in-w32-dbg\build\gfx\layers\ImageLayers.h(311): could be 'mozilla::layers::ImageContainer::ImageContainer(mozilla::layers::LayerManager *,void *)'
e:\builds\moz2_slave\m-in-w32-dbg\build\gfx\layers\ImageLayers.h(139): or 'mozilla::layers::ImageContainer::ImageContainer(void *)'
        while trying to match the argument list '(long)'
}

Comment 19

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Clear cached ImageContainers

Comment 20

[Bas Schouten (:bas.schouten)]

Fwiw, I'm moving ImageContainers to no longer be part of specific layer managers? Can we delay this, it basically means I'll have to start all over again. I'm willing to do this work if needed after I've done the refactor of image containers.

Comment 21

[Joe Drew (not getting mail)]

Comment on attachment 582976 [details] [diff] [review]
Part 6: Clear cached ImageContainers

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

::: image/public/ImageOthers.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-

Perhaps this could be called "ImageHelpers"

@@ +17,5 @@
> + *
> + * The Initial Developer of the Original Code is
> + * Netscape Communications Corporation.
> + * Portions created by the Initial Developer are Copyright (C) 2001
> + * the Initial Developer. All Rights Reserved.

This license block needs to be updated :) Contributors too.

::: image/src/imgLoader.cpp
@@ +1549,5 @@
>  }
>  
> +void imgLoader::ClearCachedLayers(imgCacheTable &aCacheToClear)
> +{
> +  LOG_STATIC_FUNC(gImgLog, "imgLoader::ClearCachedLayers table");

"table" isn't needed here since there's only one version of ClearCachedLayers.

@@ +1554,5 @@
> +
> +  // We have to make a temporary, since RemoveFromCache removes the element
> +  // from the queue, invalidating iterators.
> +  nsTArray<nsRefPtr<imgCacheEntry> > entries;
> +  aCacheToClear.Enumerate(EnumEvictEntries, &entries);

We don't need to make a temporary here - we're not calling RemoveFromCache.

Comment 22

[Joe Drew (not getting mail)]

This would be very nice to have, but doesn't necessarily block Fennec beta.

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This work allows us to cut down on memory used for in-content images, and additionally allows us to highly optimize games that would use the same source sprite image in several different locations.

Comment 24

[Dietrich Ayala (:dietrich)]

Not blocking, since there's no particular problem this is addressing. Would totally love to have it though.

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

There's a problem, but it's not clear yet how important it is.  Not sure of a better way to track this, but no reason for it to linger on blocker nom lists, I guess.