Last Comment Bug 715785 - Make ImageContainers independent of LayerManagers
: Make ImageContainers independent of LayerManagers
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla13
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
: 713552 (view as bug list)
Depends on: 723618 726580 730066 744063 778385 778642
Blocks: omtc
  Show dependency treegraph
 
Reported: 2012-01-05 20:02 PST by Bas Schouten (:bas.schouten)
Modified: 2012-08-01 07:16 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Majority of refactoring patch (100.78 KB, patch)
2012-01-05 20:03 PST, Bas Schouten (:bas.schouten)
roc: feedback+
Details | Diff | Splinter Review
Refactor imagecontainers to be layer-manager-independent (160.99 KB, patch)
2012-01-18 15:40 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Refactor imagecontainers to be layer-manager-independent v2 (164.09 KB, patch)
2012-01-18 16:26 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Refactor imagecontainers to be layer-manager-independent v3 (171.99 KB, patch)
2012-01-18 20:02 PST, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review

Description Bas Schouten (:bas.schouten) 2012-01-05 20:02:15 PST
In order to be able to use image containers properly, asynchronous to rendering it would be a lot easier if they were independent of layermanagers. This would also significantly reduce code duplication between layer manager backends.

It makes some optimizations a little harder though and has some performance advantages and some drawbacks.
Comment 1 Bas Schouten (:bas.schouten) 2012-01-05 20:03:43 PST
Created attachment 586332 [details] [diff] [review]
Majority of refactoring patch

This is the majority of the patch to make this happen. It excluses OGL layers/some slight changes to plugins as they still need MacIOSurfaces fixed to work right.
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-10 18:32:22 PST
Comment on attachment 586332 [details] [diff] [review]
Majority of refactoring patch

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

Remind me why PlanarYCbCrD3D10 doesn't upload to the GPU in SetData()?

I see that ImageLayerOGL still needs to be updated. LayerManagerOGL might need an ImageFactory to implement the memory recycling that GL does. Or maybe we should implement that memory recycling in PlanarYCbCrImage? (It's a big performance win on Mac.)

Overall, this looks fantastic. It's a great simplification.

How is this going to work with ShadowLayers/OMTC? Matt was working on having ShadowImageContainers; can that still work correctly when ImageContainers are independent of LayerManagers? I assume so, just checking. We will need to do something to ensure that ImageContainer::SetCurrentImage works correctly when called from the main thread during a layer manager transaction (i.e., becomes part of the transaction). We had talked about splitting SetCurrentImage into two methods, maybe a version SetCurrentImageInTransaction(LayerManager*)? Might be tricky. Not this bug though.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +2421,5 @@
>      nsContentUtils::PersistentLayerManagerForDocument(OwnerDoc());
>    if (!manager)
>      return nsnull;
>  
>    mImageContainer = manager->CreateImageContainer();

We don't need to get the layer manager here anymore. We can just create a container directly. This is one of the nice benefits of the new approach.

::: gfx/layers/ImageLayers.cpp
@@ +171,5 @@
> +void
> +PlanarYCbCrImage::SetData(const Data &aData)
> +{
> +  Data mData;
> +  gfxIntSize mSize;

These are unused, and incorrectly named.

::: gfx/layers/ImageLayers.h
@@ +126,5 @@
>  
> +  virtual already_AddRefed<gfxASurface> GetAsSurface() = 0;
> +  virtual gfxIntSize GetSize() = 0;
> +
> +  nsAutoPtr<ImageUserData> mUserData[LayerManager::LAYERS_LAST];

Put this behind accessor methods.

Let's not call this UserData since it really isn't. (Since "users" of Image would be in layout.) Let's call it BackendData?

@@ +157,5 @@
> +/**
> + * A class that manages Image creation for a LayerManager. The only reason
> + * we need a separate class here is that LayerMananers aren't threadsafe
> + * (because layers can only be used on the main thread) and we want to
> + * be able to set create images from any thread, to facilitate video playback

"to create"

@@ +163,5 @@
> + * Different layer managers can implement child classes of this making it
> + * possible to create layer manager specific images.
> + * This class is not meant to be used directly but rather can be set on an
> + * image container. This is usually done by the layer system internally and
> + * not explicitly by users.

Document what this default implementation does.

@@ +231,5 @@
>     * when accessing thread-shared state.
>     * Implementations must call CurrentImageChanged() while holding
>     * mReentrantMonitor.
>     */
> +  already_AddRefed<Image> GetCurrentImage();

We can probably inline this now, along with some of the other methods.

@@ +281,3 @@
>  
> +  void SetImageFactory(ImageFactory *aFactory)
> +  { mImageFactory = aFactory ? aFactory : new ImageFactory(); }

This needs to hold the monitor.

@@ +474,5 @@
>    /**
>     * This makes a copy of the data buffers.
>     * XXX Eventually we will change this to not make a copy of the data,
>     * Right now it doesn't matter because the BasicLayer implementation
>     * does YCbCr conversion here anyway.

Fix comment?

@@ +544,5 @@
>     * This can only be called on the main thread. It may add a reference
>     * to the surface (which will eventually be released on the main thread).
>     * The surface must not be modified after this call!!!
>     */
> +  virtual void SetData(const Data& aData)

Likewise I don't know why this is virtual.

::: gfx/layers/Layers.h
@@ +428,5 @@
>     */
>    virtual already_AddRefed<ReadbackLayer> CreateReadbackLayer() { return nsnull; }
>  
>    /**
>     * Can be called anytime

", from any thread."

@@ +1387,5 @@
> +    NS_ASSERTION(NS_IsMainThread(),
> +                 "Can only add a reference on the main thread");
> +    aRawRef->AddRef();
> +  }
> +};

Can we put this somewhere else, in some utils header file? It doesn't really belong in Layers.h.

::: gfx/layers/basic/BasicImages.cpp
@@ +149,5 @@
>      return result.forget();
>    }
>  
>    // XXX: If we forced delayed conversion, are we ever going to hit this?
>    // We may need to implement the conversion here.

remove comment

::: gfx/layers/basic/BasicLayers.h
@@ +208,5 @@
>    nsRefPtr<gfxContext> mDefaultTarget;
>    // The context to draw into.
>    nsRefPtr<gfxContext> mTarget;
> +  // Image factory we use.
> +  nsRefPtr<ImageFactory> mFactory;

How about making this a global singleton?

::: image/src/RasterImage.cpp
@@ +943,5 @@
>  NS_IMETHODIMP
>  RasterImage::GetImageContainer(LayerManager* aManager,
>                                 ImageContainer **_retval)
>  {
> +  if (mImageContainer) {

As earlier, we don't need aManager here. It should be removed.

::: layout/generic/nsObjectFrame.cpp
@@ +1514,3 @@
>    }
>  
>    container = manager->CreateImageContainer();

Again, let's remove 'manager'.

::: layout/generic/nsVideoFrame.cpp
@@ +202,5 @@
>    // to do anything here. Otherwise we need to set up a temporary
>    // ImageContainer, capture the video data and store it in the temp
>    // container. For now we also check if the manager is equal since not all
>    // image containers are manager independent yet.
> +  if (!container)

This can't fail now. The element's GetImageContainer should never fail and should always return a container we can use. So, this entire fallback path can be removed.
Comment 3 Bas Schouten (:bas.schouten) 2012-01-11 22:02:26 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> Comment on attachment 586332 [details] [diff] [review]
> Majority of refactoring patch
> 
> Review of attachment 586332 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Remind me why PlanarYCbCrD3D10 doesn't upload to the GPU in SetData()?

It didn't seem to make a real perf difference, I want to re-introduce it in a separate patch if we think it's worth it (i.e. a D3D10 ImageFactory and all)

> I see that ImageLayerOGL still needs to be updated. LayerManagerOGL might
> need an ImageFactory to implement the memory recycling that GL does. Or
> maybe we should implement that memory recycling in PlanarYCbCrImage? (It's a
> big performance win on Mac.)

I've done ImageLayerOGL for all except MacIOSurface. PlanarYCbCrImage texture recycling I moved into PlanarYCbCrOGLBackendData. Memory recycling I'm planning to move into PlanarYCbCrImage like you suggest.

> 
> Overall, this looks fantastic. It's a great simplification.
> 
> How is this going to work with ShadowLayers/OMTC? Matt was working on having
> ShadowImageContainers; can that still work correctly when ImageContainers
> are independent of LayerManagers?

I don't foresee any real problems, we just need to explicitly share them to a process.

> I assume so, just checking. We will need
> to do something to ensure that ImageContainer::SetCurrentImage works
> correctly when called from the main thread during a layer manager
> transaction (i.e., becomes part of the transaction). We had talked about
> splitting SetCurrentImage into two methods, maybe a version
> SetCurrentImageInTransaction(LayerManager*)? Might be tricky. Not this bug
> though.

Yeah, something like the latter seems like it might be sensible, I guess we should solve it in a follow-up bug.
Comment 4 Bas Schouten (:bas.schouten) 2012-01-18 15:39:14 PST
I'm about to upload a patch, I dealt with all comments except for a couple:

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> Comment on attachment 586332 [details] [diff] [review]
> Majority of refactoring patch
> 
> Review of attachment 586332 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +231,5 @@
> >     * when accessing thread-shared state.
> >     * Implementations must call CurrentImageChanged() while holding
> >     * mReentrantMonitor.
> >     */
> > +  already_AddRefed<Image> GetCurrentImage();
> 
> We can probably inline this now, along with some of the other methods.

This particular function gains a bit of work with the cross-platform stuff. It's probably best to keep it stand-alone.

> ::: gfx/layers/Layers.h
> @@ +428,5 @@
> >     */
> >    virtual already_AddRefed<ReadbackLayer> CreateReadbackLayer() { return nsnull; }
> >  
> >    /**
> >     * Can be called anytime
> 
> ", from any thread."
> 
> @@ +1387,5 @@
> > +    NS_ASSERTION(NS_IsMainThread(),
> > +                 "Can only add a reference on the main thread");
> > +    aRawRef->AddRef();
> > +  }
> > +};
> 
> Can we put this somewhere else, in some utils header file? It doesn't really
> belong in Layers.h.

What place do you suggest? I looked but saw nothing obvious where I it would really 'fit'.

> ::: gfx/layers/basic/BasicImages.cpp
> @@ +149,5 @@
> >      return result.forget();
> >    }
> >  
> >    // XXX: If we forced delayed conversion, are we ever going to hit this?
> >    // We may need to implement the conversion here.
> 
> remove comment
> 
> ::: gfx/layers/basic/BasicLayers.h
> @@ +208,5 @@
> >    nsRefPtr<gfxContext> mDefaultTarget;
> >    // The context to draw into.
> >    nsRefPtr<gfxContext> mTarget;
> > +  // Image factory we use.
> > +  nsRefPtr<ImageFactory> mFactory;
> 
> How about making this a global singleton?

Hrm, since this should still also be a refcounted object, how do you suggest we go about this?
Comment 5 Bas Schouten (:bas.schouten) 2012-01-18 15:40:19 PST
Created attachment 589680 [details] [diff] [review]
Refactor imagecontainers to be layer-manager-independent

This processes a bunch of the review comments and contains the OGL work, many thanks to Benoit Girard for helping me out with the MacIOSurface here!
Comment 6 Bas Schouten (:bas.schouten) 2012-01-18 16:26:10 PST
Created attachment 589699 [details] [diff] [review]
Refactor imagecontainers to be layer-manager-independent v2

Updated to include missing file.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-18 18:32:31 PST
(In reply to Bas Schouten (:bas) from comment #4)
> > @@ +1387,5 @@
> > > +    NS_ASSERTION(NS_IsMainThread(),
> > > +                 "Can only add a reference on the main thread");
> > > +    aRawRef->AddRef();
> > > +  }
> > > +};
> > 
> > Can we put this somewhere else, in some utils header file? It doesn't really
> > belong in Layers.h.
> 
> What place do you suggest? I looked but saw nothing obvious where I it would
> really 'fit'.

gfxASurface.h I think.

> > ::: gfx/layers/basic/BasicLayers.h
> > @@ +208,5 @@
> > >    nsRefPtr<gfxContext> mDefaultTarget;
> > >    // The context to draw into.
> > >    nsRefPtr<gfxContext> mTarget;
> > > +  // Image factory we use.
> > > +  nsRefPtr<ImageFactory> mFactory;
> > 
> > How about making this a global singleton?
> 
> Hrm, since this should still also be a refcounted object, how do you suggest
> we go about this?

We'd need a static Shutdown method but that's not hard to do, just call it where we call the gfxPlatform shutdown.

But I thought you decided we needed per-layer-manager factories after all?
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-18 18:37:46 PST
> Memory recycling I'm planning to move into PlanarYCbCrImage like you suggest.

I don't see that in the patch.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-18 18:37:56 PST
Comment on attachment 589699 [details] [diff] [review]
Refactor imagecontainers to be layer-manager-independent v2

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

::: gfx/layers/ImageLayers.h
@@ +130,5 @@
> +
> +  ImageBackendData* GetBackendData(LayerManager::LayersBackend aBackend)
> +  { return mBackendData[aBackend]; }
> +  void SetBackendData(LayerManager::LayersBackend aBackend, ImageBackendData* aData)
> +  { mBackendData[aBackend] = aData; }

Do these need locking?

@@ +170,5 @@
> + * possible to create layer manager specific images.
> + * This class is not meant to be used directly but rather can be set on an
> + * image container. This is usually done by the layer system internally and
> + * not explicitly by users. The default implementation set by default on new
> + * containers will creates images that perform 'normally' on all backends.

This isn't clear enough.

I think you should say that the default implementation creates images that live in main memory.

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +201,2 @@
>  {
> +  GLTexture mTexture;

Isn't a destructor needed to release this?

::: gfx/layers/opengl/ImageLayerOGL.h
@@ +184,2 @@
>  {
> +  CairoOGLBackendData() : mLayerProgram(gl::RGBALayerProgramType), mTiling(false) {}

Don't you need a destructor to clean up here?

::: image/public/imgIContainer.idl
@@ +192,2 @@
>     */
> +  [noscript] ImageContainer getImageContainer();

rev IID
Comment 10 Bas Schouten (:bas.schouten) 2012-01-18 19:33:47 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Comment on attachment 589699 [details] [diff] [review]
> Refactor imagecontainers to be layer-manager-independent v2
> 
> Review of attachment 589699 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ImageLayers.h
> @@ +130,5 @@
> > +
> > +  ImageBackendData* GetBackendData(LayerManager::LayersBackend aBackend)
> > +  { return mBackendData[aBackend]; }
> > +  void SetBackendData(LayerManager::LayersBackend aBackend, ImageBackendData* aData)
> > +  { mBackendData[aBackend] = aData; }
> 
> Do these need locking?

I doubt it, I'm not sure if images will work right when rendered from multiple threads at the same time anyway. If we decide that is a requirement then yes, it does :) But I'd need to give the whole class a closer look.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-18 19:51:50 PST
Makes sense. don't worry about it.
Comment 12 Bas Schouten (:bas.schouten) 2012-01-18 20:02:42 PST
Created attachment 589762 [details] [diff] [review]
Refactor imagecontainers to be layer-manager-independent v3

Adjusted for review comments.
Comment 13 Bas Schouten (:bas.schouten) 2012-01-31 18:23:05 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/67b0e13d7a62
Comment 14 Ed Morley [:emorley] 2012-02-02 08:16:27 PST
https://hg.mozilla.org/mozilla-central/rev/67b0e13d7a62
Comment 15 Benoit Girard (:BenWa) 2012-02-02 10:26:58 PST
\o/ Nice!
Comment 16 Benoit Girard (:BenWa) 2012-02-07 10:56:41 PST
*** Bug 713552 has been marked as a duplicate of this bug. ***
Comment 17 Steven Michaud [:smichaud] (Retired) 2012-03-19 12:44:40 PDT
(In reply to comment #2)

> This can't fail now. The element's GetImageContainer should never
> fail and should always return a container we can use. So, this
> entire fallback path can be removed.

This turns out not to be true.  See bug 730066, particularly bug
730066 comment #7.

Bas, I've taken the liberty of assigning that bug to you :-)

Note You need to log in before you can comment on or make changes to this bug.