For DirectVideo (TM), we'll want content processes to have in their layer trees some layer that corresponds to video nsIFrames, but refers to a layer that lives in another PLayers connection (the one through which video frames are pushed). PlaceholderLayer will need - all normal ImageLayer attributes, manipulable by the content-process main thread - some unique ID across all processes by which to refer to its "real" layer in the compositor We might be able to teach ImageLayer to do this depending on how we publish frames from video decoder threads. We'll want this for plugin layers though. Details TBD.

Ref Layer without painting and holding global Layer ID.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #0) > We might be able to teach ImageLayer to do this depending on how we publish > frames from video decoder threads. Did this prove to be infeasible? I was hope that we'd use ImageContainer to solve this. Instead of changing the layer off the main thread, we'd leave it alone and essentially make ImageContainer::SetCurrentImage change the current image without having to sync with the content process main thread.

I'm not doing anything with Layers off-main thread.. RefLayer just holding Global ID and I use ImageContainer to pass that ID to decoder thread. I had previous implementation where I did set some mNoCopy mode flag to image container, and was ignoring painting in ShadowableImageLayer when it had container with current image with that flag..., but that was looking a bit messy.. Here is the main media bridge implementation which is using this LayerID, and pushing frames directly to Chrome process... https://bug598868.bugzilla.mozilla.org/attachment.cgi?id=560824 May be that helps to understand whole picture...

Added path to call MediaBridge directly from Image/Image container

Is there a reason why BasicImageContainer::CreateImage can't set up the right MediaBridge state automatically, when the image container is for a shadowed image layer? I'm not sure how MediaBridge is supposed to work. I was hoping that ImageLayers with shadows could automatically set up their ImageContainers for "media bridging", and that those ImageContainers would automatically set up their Images for "media bridging" too, so users of ImageLayer/ImageContainer will not need to change at all and we won't need new public APIs on ImageLayer/ImageContainer.

we can do that, but in that case we going to push all ImageLayers throug MediaBridge... is it ok to do that? What else is using imageLayers? ImageFrame? anything else?

Ok, I agree it should that way for all ImageLayers... I did it this way because previous impl was tied to MediaDecoder calls, but now when ImageContainer and Image does work in auto mode, I can remove that CreateImageLayer Api change too.

Plugins and transformed single images --- which will both be calling SetCurrentImage on the main thread (for now!). What are the threading requirements around MediaBridge and the underlying IPDL? Would it be helpful for the creator of an ImageLayer to specify the thread that will perform all ImageContainer::SetCurrentImage calls? Because we can do that. We do have to allow ImageContainer::CreateImage and Image::SetData to be called on a different thread to the thread that calls ImageContainer::SetCurrentImage. Hope that's OK.

Ok, here is version which creates RefLayer automatically for content process ShadowableLayer... Content process check is needed there because we do call that also from Content process when create ImageLayer for remote videport checker board... don't know why..

Looking better, but what are these globalIDs? and what is an ImageRefLayer?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9) > Plugins and transformed single images --- which will both be calling > SetCurrentImage on the main thread (for now!). For plugins we are not going to call any Image data stuff at all in ContentProcess... data will fly directly from Plugin-child to Chrome process.. (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11) > Looking better, but what are these globalIDs? and what is an ImageRefLayer? Will rename that, currently Shadow/Shadowable Image layers connected to each other with PLayerChild/Parent protocol, that ID is unique ID which help other protocols like Media/Plugin bridge to connect ShadowLayer with related ShadowableLayer and it's element like MediaElement or PluginInstance.

How is this scheme going to work with multiple content and plugin processes?

IIUC each contentChild process will have own bridge protocol...

I think for windowless plugins we should have something like a PlaceholderLayer, but we should call it a PluginLayer. The need for that is clear because the content process will never see the images. I see no need to generalize that beyond plugins.

(In reply to Oleg Romashin (:romaxa) from comment #14) > IIUC each contentChild process will have own bridge protocol... If you have a placeholder ID X in the compositor process, how do you know where to look up the surface backing X? That wasn't clear to me from scanning these patches, sorry. (Didn't look really closely.)

Ok, here is Unique Layer ID API + ShadowImageLayers tracker which would allow us to register multiple bridge protocols

Comment on attachment 561121 [details] [diff] [review] ShadowImageLayers tracker + listener IFACE err. this is bad version

This should be better, + added instance auto-destroy when no listeners and layers are registered

oh, I'm stupid, ImageAttributes can be called multiple times, and array is not hash... prevent multiple addition of layer into array

I still don't know why global IDs need to be public API in ImageLayers.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22) > I still don't know why global IDs need to be public API in ImageLayers. 1) In order to share it to plugin instance https://bugzilla.mozilla.org/attachment.cgi?id=561122&action=edit 2) For IShadowImageLayersListener we can make it friend probably, not sur if In should make it friend for pluginInstance...

Can we create a PluginLayer right now that inherits from ImageLayer (for now) and exposes the uniqueID? Also, do we need SetUniqueID as a public API? It should be able to be just on ShadowImageLayer I think.

Comment on attachment 561580 [details] [diff] [review] Unique Shadow/Shadowable Layer ID counter + tracker Review of attachment 561580 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/basic/BasicLayers.cpp @@ +2354,5 @@ > BasicImageLayer(aManager) > { > MOZ_COUNT_CTOR(BasicShadowableImageLayer); > + static PRUint32 uniqueLayerID = 0; > + mUniqueID = ++uniqueLayerID; Should IDs be 64bit? What if we have multiple content processes sending layer updates to the same chrome process? Don't we need a way to ensure their IDs are distinct? ::: gfx/layers/ipc/ShadowLayers.h @@ +639,5 @@ > + * And providing access to related ShadowImageLayer by ID > + * Bridge protocol's can use these Layers in order to call Init/Swap and deliver > + * Image frames from other processes or threads, depend on protocol implementation > + */ > + class ImagesTracker Does this need to be in the .h file? Can't we just put this entire class into ShadowLayers.cpp? Maybe we don't even need a class, just a global variable and some static functions?

Comment on attachment 561934 [details] [diff] [review] Unique ImageContainer ID counter + GetShadowImageLayerByID API Review of attachment 561934 [details] [diff] [review]: ----------------------------------------------------------------- This makes much more sense to me now :-) ::: dom/ipc/ContentChild.h @@ +165,5 @@ > + * Return Unique Global ID as result of operation > + * aLocalUniqueID << 16 | mProcessUniqueID > + * @parama LocalUniqueID must be 48bit value > + */ > + PRUint64 GetGlobalUniqueID(PRUint64& aLocalUniqueID); Why don't you just make this GetProcessUniqueID and let the caller be responsible for combining that with some other local ID? ::: dom/ipc/ContentParent.cpp @@ +225,5 @@ > + unused << SendProcessUniqueID(++processUniqueID); > + if (processUniqueID == PR_UINT16_MAX) { > + // Too many processes per session > + NS_WARNING("Reset processUniqueID counter to 0"); > + processUniqueID = 0; You really should track somewhere which IDs are in use and make sure you return a unique one. ::: dom/ipc/PContent.ipdl @@ +136,5 @@ > * Start accessibility engine in content process. > */ > ActivateA11y(); > > + ProcessUniqueID(PRUint16 id); Call this SetProcessUniqueID. ::: gfx/layers/ImageLayers.h @@ +280,5 @@ > mPreviousImagePainted = PR_TRUE; > } > } > > + virtual PRUint64 GetUniqueID(void) { return 0; } Call this GetCompositorUniqueID and document that it returns 0 if there is no separate compositor process. Also document that it's unique across all ImageContainers known to the compositor process. ::: gfx/layers/basic/BasicImages.cpp @@ +265,5 @@ > nsRefPtr<Image> mImage; > gfxIntSize mScaleHint; > gfxImageFormat mOffscreenFormat; > PRPackedBool mDelayed; > + PRUint64 mUniqueID; This could be in ImageContainer itself and GetUniqueID could then be nonvirtual. ::: gfx/layers/ipc/ShadowLayers.cpp @@ +64,5 @@ > + * And providing access to related ShadowImageLayer by ID > + * Bridge protocol's can use these Layers in order to call Init/Swap and deliver > + * Image frames from other processes or threads, depend on protocol implementation > + */ > +typedef nsDataHashtable<nsUint32HashKey, ShadowImageLayer*> LayersHash; you need a 64bit key surely? @@ +93,5 @@ > +{ > + NS_ASSERTION(aLayer->GetUniqueID(), "Layer must have unique ID"); > + LayersHash& layers = GetShadowLayers(); > + if (!layers.Get(aLayer->GetUniqueID())) { > + layers.Put(aLayer->GetUniqueID(), aLayer); I think you don't need the Get here, you can just do the Put and that will be more efficient. You probably should assert that either there's no existing entry for the ID, or the existing entry already points to this layer. ::: gfx/layers/ipc/ShadowLayers.h @@ +439,5 @@ > +/** > + * Return ShadowLayer by UniqueID > + * Supposed to be used by Media/Plugin bridges > + */ > +ShadowImageLayer* GetShadowImageLayerByID(const PRUint64& aLayerID); Don't pass PRUint64s by reference. Just pass them by value. @@ +664,5 @@ > + void RegisterShadowImageLayer(ShadowImageLayer* aLayer); > + /** > + * Remove Image layer from tracker > + */ > + void UnRegisterShadowImageLayer(ShadowImageLayer* aLayer); Shouldn't these either be static or not take an aLayer parameter? @@ +672,5 @@ > + virtual void SetUniqueID(const PRUint64& aUniqueID) > + { > + if (mUniqueID != aUniqueID) { > + mUniqueID = aUniqueID; > + RegisterShadowImageLayer(this); Unregister with the previous mUniqueID

Comment on attachment 562244 [details] [diff] [review] Unique ImageContainer ID counter + GetShadowImageLayerByID API Review of attachment 562244 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentParent.cpp @@ +242,5 @@ > + mUniqueProcessID = processUniqueID; > + unused << SendSetProcessUniqueID(mUniqueProcessID); > + if (processUniqueID == PR_UINT16_MAX) { > + processUniqueID = 0; > + } This is overcomplicated. Since processUniqueID is PRUint16 you don't need to reset it to 0 explicitly. You also don't need to abort above if we wrap around, that's fine because of the uniqueness check. However, you do need to check for the situation that we have created 2^16 processes and all available IDs are full. You can just write PRUint16 prevID = processUniqueID; while (!IsIdUnique(++processUniqueID)) { if (processUniqueID == prevID) { NS_RUNTIMEABORT("Too many processes"). } } ::: gfx/layers/basic/BasicImages.cpp @@ +131,5 @@ > void SetOffscreenFormat(gfxImageFormat aFormat) { mOffscreenFormat = aFormat; } > gfxImageFormat GetOffscreenFormat() { return mOffscreenFormat; } > > PRUint32 GetDataSize() { return mBuffer ? mDelayedConversion ? mBufferSize : mSize.height * mStride : 0; } > + void SetContainerUniqueID(PRUint64 aCompositorUniqueID) { mCompositorUniqueID = aCompositorUniqueID; } Why do Images need IDs now? That doesn't seem right. @@ +253,5 @@ > + while (GetShadowImageLayerByID(++uniqueID)) { > + if (uniqueID == uniqueIDMAX) { > + NS_RUNTIMEABORT("This code does not supports more than 2^48 ShadowImagelayers"); > + } > + } I think you can just assert that uniqueID < (PRUint64(1) << 48). No need to call GetShadowImageLayerByID. In fact, isn't calling GetShadowImageLayerByID wrong here, since we're in the content process so we don't have any shadow layers. 2^48 is > 256 trillion. If we create 1000 ImageContainers every second, it will take 8 years to wrap around. That shouldn't be a problem. @@ +254,5 @@ > + if (uniqueID == uniqueIDMAX) { > + NS_RUNTIMEABORT("This code does not supports more than 2^48 ShadowImagelayers"); > + } > + } > + mCompositorUniqueID = uniqueID << 16 | processID; () around uniqueID << 16. @@ +257,5 @@ > + } > + mCompositorUniqueID = uniqueID << 16 | processID; > + if (uniqueID == uniqueIDMAX) { > + uniqueID = 0; > + } No need for this either. ::: gfx/layers/ipc/ShadowLayers.cpp @@ +84,5 @@ > +{ > + LayersHash& layers = GetShadowLayers(); > + NS_ASSERTION(mCompositorUniqueID, "Layer must have unique ID"); > + NS_ASSERTION(layers.Get(mCompositorUniqueID) != this, > + "Layer already registered"); You should assert that there is no entry at all. If there's an entry with a different layer, we have a bug. ::: gfx/layers/ipc/ShadowLayers.h @@ +635,5 @@ > +/** > + * Return ShadowLayer by UniqueID > + * Supposed to be used by Media/Plugin bridges > + */ > +ShadowImageLayer* GetShadowImageLayerByID(const PRUint64 aLayerID); const non-reference parameters are a bit of a waste of time since the callee can't modify them anyway. @@ +669,5 @@ > + /** > + * Remove Image layer from tracker by ID > + * @param aCompositorID if 0 then GetCompositorUniqueID() will be used > + */ > + void UnRegisterShadowImageLayer(PRUint64 aCompositorID = 0); "Unregister" --- "Un" is not a word :-) @@ +674,5 @@ > + > + virtual void SetCompositorUniqueID(const PRUint64& aCompositorUniqueID) > + { > + if (mCompositorUniqueID != aCompositorUniqueID) { > + UnRegisterShadowImageLayer(aCompositorUniqueID); This is really confusing. Why are we unregistering the layer that has aCompositorUniqueID? Surely no layer should already have aCompositorUniqueID?

> > + NS_ASSERTION(layers.Get(mCompositorUniqueID) != this, > > + "Layer already registered"); > You should assert that there is no entry at all. If there's an entry with a > different layer, we have a bug. > > + if (mCompositorUniqueID != aCompositorUniqueID) { > > + UnRegisterShadowImageLayer(aCompositorUniqueID); > > This is really confusing. Why are we unregistering the layer that has > aCompositorUniqueID? Surely no layer should already have aCompositorUniqueID? We have situation sometime when one container jumping from one layer to another, and I receive Register for new layer before previous layer destroyed.

hmmm ok.

> Why do Images need IDs now? That doesn't seem right. hmm, I was planning to push image frames into MediaBridge in SetData call, http://hg.mozilla.org/users/romaxa_gmail.com/cross-video-layer/file/a893a099a191/media_bridge.diff#l1253

Hope all comments addressed.

Ups, minor copy/paste typo bug

Ok, let's do it without Image ID trick, because it is related to another part of this implementation

Probably this could be done using some other API, is there are some other API which could trigger ShadowLayers tree paint?

Comment on attachment 562499 [details] [diff] [review] Unique ImageContainer ID counter + GetShadowImageLayerByID API Review of attachment 562499 [details] [diff] [review]: ----------------------------------------------------------------- On the content-process side the IDs are associated with ImageContainers, which I think is right, but on the compositor-process side they're associated with ShadowImageLayers. I think this is acceptable but please add a comment somewhere explaining this. ::: dom/ipc/ContentChild.cpp @@ +782,5 @@ > > +bool > +ContentChild::RecvSetProcessUniqueID(const PRUint16& aID) > +{ > + NS_ASSERTION((aID - 1) >> 16 == 0, "Process ID must not be bigger than 16bit number"); This assertion is unnecessary since a PRUint16 can't be bigger than 16 bits ::: dom/ipc/ContentParent.cpp @@ +192,5 @@ > aArray = *gContentParents; > } > > +bool > +ContentParent::IsIdUnique(const PRUint16& aID) IsIDInUse @@ +243,5 @@ > + mUniqueProcessID = processUniqueID; > + unused << SendSetProcessUniqueID(mUniqueProcessID); > + if (processUniqueID == PR_UINT16_MAX) { > + processUniqueID = 0; > + } Comment that this avoids using ID 0 for any process. ::: gfx/layers/ipc/ShadowLayers.h @@ +670,5 @@ > + > + virtual void SetCompositorUniqueID(const PRUint64& aCompositorUniqueID) > + { > + if (mCompositorUniqueID != aCompositorUniqueID) { > + UnregisterShadowImageLayer(aCompositorUniqueID); Add a comment indicating that aCompositorUniqueID might currently be assigned to another layer and that we need to reassign to this layer because the old layer might not have been destroyed yet.

Comment on attachment 562560 [details] [diff] [review] LayerTree Update API Review of attachment 562560 [details] [diff] [review]: ----------------------------------------------------------------- Can you add a commit message explaining what this patch actually does? ::: gfx/layers/ipc/ShadowLayers.h @@ +402,3 @@ > protected: > + ShadowLayerManager() > + : mShadowLayersParent(NULL) {} nsnull ::: gfx/layers/ipc/ShadowLayersParent.cpp @@ +131,5 @@ > > ShadowLayersParent::~ShadowLayersParent() > { > MOZ_COUNT_DTOR(ShadowLayersParent); > + mLayerManager->SetShadowLayersParent(NULL); nsnull @@ +138,5 @@ > void > ShadowLayersParent::Destroy() > { > mDestroyed = true; > + mLayerManager->SetShadowLayersParent(NULL); nsnull

Comment on attachment 562667 [details] [diff] [review] LayerTree Update API Review of attachment 562667 [details] [diff] [review]: ----------------------------------------------------------------- You still need a commit message describing what this actually does ::: gfx/layers/ipc/ShadowLayers.cpp @@ +601,5 @@ > +ShadowLayerManager::UpdateLayers(void) > +{ > + if (!IsDestroyed() && mShadowLayersParent) { > + BeginTransactionWithTarget(NULL); > + EndTransaction(NULL, NULL); nsnull

I still don't understand what this patch is doing.

Media/Plugin Bridge pushing frames from plugin-process/decode thread to compositor process, looking for layer by ID, calling Swap and after that we should ask compositor to paint new pushed frame, so that is why we need to trigger layer update, call RenderFrameParent -> ShadowLayersUpdated().

Fixed in bug 745148.