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.

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

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

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?

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!

Updated to include missing file.

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

> Memory recycling I'm planning to move into PlanarYCbCrImage like you suggest. I don't see that in the patch.

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

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

Makes sense. don't worry about it.

Adjusted for review comments.

\o/ Nice!

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