If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add ImageContainer to ShadowLayers

RESOLVED WONTFIX

Status

()

Core
Graphics
RESOLVED WONTFIX
6 years ago
5 years ago

People

(Reporter: mattwoodrow, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:-)

Details

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Created attachment 568342 [details] [diff] [review]
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?
Attachment #568342 - Flags: feedback?(jones.chris.g)
(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.
(Reporter)

Comment 3

6 years ago
Created attachment 569582 [details] [diff] [review]
Part 1: Add ImplData to ImageContainer
Attachment #568342 - Attachment is obsolete: true
Attachment #568342 - Flags: feedback?(jones.chris.g)
Attachment #569582 - Flags: review?(roc)
(Reporter)

Comment 4

6 years ago
Created attachment 569583 [details] [diff] [review]
Part 2: Add ShadowImplData

Separates the Shadow specific code from BasicImplData and uses it in BasicImageContainer
Attachment #569583 - Flags: review?(jones.chris.g)
Attachment #569582 - Flags: review?(roc) → review+
(Reporter)

Comment 5

6 years ago
Created attachment 569584 [details] [diff] [review]
Part 3: Add Flush() and Disconnect() methods to ImageContainer
Attachment #569584 - Flags: review?(jones.chris.g)
(Reporter)

Comment 6

6 years ago
Created attachment 569585 [details] [diff] [review]
Part 4: Move BasicShadowableLayer into ImageLayers.h
Attachment #569585 - Flags: review?(jones.chris.g)
(Reporter)

Comment 7

6 years ago
Created attachment 569586 [details] [diff] [review]
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.
Attachment #569586 - Flags: review?(jones.chris.g)
Attachment #569583 - Flags: review?(jones.chris.g) → review+
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.
Attachment #569584 - Flags: review?(jones.chris.g) → review+
Attachment #569585 - Flags: review?(jones.chris.g) → review+
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.
Attachment #569586 - Flags: review?(jones.chris.g)
(Reporter)

Comment 10

6 years ago
Created attachment 570174 [details] [diff] [review]
Part 5: Add Shadow(able)ImageContainer v2

Fixed review comments, added OGL support and deleted lots of unneeded code.
Attachment #569586 - Attachment is obsolete: true
Attachment #570174 - Flags: review?(jones.chris.g)
(Reporter)

Comment 11

6 years ago
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 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.
Attachment #570174 - Flags: review?(jones.chris.g) → review+
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?
(Reporter)

Comment 14

6 years ago
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.
(Reporter)

Comment 15

6 years ago
Created attachment 578958 [details] [diff] [review]
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?
Attachment #578958 - Flags: review?(joe)
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?
Attachment #578958 - Flags: review?(joe) → review-
(Reporter)

Updated

6 years ago
Blocks: 598873

Updated

6 years ago
Blocks: 706499
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
Whiteboard: inbound-leave-open
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)'
}
(Reporter)

Comment 19

6 years ago
Created attachment 582976 [details] [diff] [review]
Part 6: Clear cached ImageContainers
Attachment #578958 - Attachment is obsolete: true
Attachment #582976 - Flags: review?(joe)
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 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.
Attachment #582976 - Flags: review?(joe) → review+
Keywords: fennecnative-betablocker
This would be very nice to have, but doesn't necessarily block Fennec beta.
Keywords: fennecnative-betablocker
No longer blocks: 598873
Blocks: 598873
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.
Blocks: 745135
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
Not blocking, since there's no particular problem this is addressing. Would totally love to have it though.
blocking-basecamp: ? → -
blocking-kilimanjaro: ? → ---
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.

Updated

5 years ago
Whiteboard: inbound-leave-open
(Reporter)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.