598872 - Create a new PlaceholderLayer or RefLayer type

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

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.

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: Image Container Parent RefLayer ID API

Comment 2

[Oleg Romashin (:romaxa)]

Attachment: Shadowable ImageRefLayer

Ref Layer without painting and holding global Layer ID.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 4

[Oleg Romashin (:romaxa)]

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

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Image Container Parent RefLayer ID API

Added path to call MediaBridge directly from Image/Image container

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 7

[Oleg Romashin (:romaxa)]

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?

Comment 8

[Oleg Romashin (:romaxa)]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 10

[Oleg Romashin (:romaxa)]

Attachment: Shadowable ImageRefLayer

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

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 12

[Oleg Romashin (:romaxa)]

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

Comment 13

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

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

Comment 14

[Oleg Romashin (:romaxa)]

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

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 16

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

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

Comment 17

[Oleg Romashin (:romaxa)]

Attachment: Provide UniqueShadowImageLayerID API

Comment 18

[Oleg Romashin (:romaxa)]

Attachment: ShadowImageLayers tracker + listener IFACE

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

Comment 19

[Oleg Romashin (:romaxa)]

Comment on attachment 561121 [details] [diff] [review]
ShadowImageLayers tracker + listener IFACE

err. this is bad version

Comment 20

[Oleg Romashin (:romaxa)]

Attachment: ShadowImageLayers tracker + listener IFACE

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

Comment 21

[Oleg Romashin (:romaxa)]

Attachment: ShadowImageLayers tracker + listener IFACE

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

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 23

[Oleg Romashin (:romaxa)]

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

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 25

[Oleg Romashin (:romaxa)]

Attachment: Unique Shadow/Shadowable Layer ID counter + tracker

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 27

[Oleg Romashin (:romaxa)]

Attachment: Unique Shadow/Shadowable Layer ID counter + tracker

Comment 28

[Oleg Romashin (:romaxa)]

Attachment: Unique ImageContainer ID counter + GetShadowImageLayerByID API

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 30

[Oleg Romashin (:romaxa)]

Attachment: Unique ImageContainer ID counter + GetShadowImageLayerByID API

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 32

[Oleg Romashin (:romaxa)]

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

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

hmmm ok.

Comment 34

[Oleg Romashin (:romaxa)]

> 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

Comment 35

[Oleg Romashin (:romaxa)]

Attachment: Unique ImageContainer ID counter + GetShadowImageLayerByID API

Hope all comments addressed.

Comment 36

[Oleg Romashin (:romaxa)]

Attachment: Unique ImageContainer ID counter + GetShadowImageLayerByID API

Ups, minor copy/paste typo bug

Comment 37

[Oleg Romashin (:romaxa)]

Attachment: Unique ImageContainer ID counter + GetShadowImageLayerByID API

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

Comment 38

[Oleg Romashin (:romaxa)]

Attachment: LayerTree Update API

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

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 41

[Oleg Romashin (:romaxa)]

Attachment: LayerTree Update API

Comment 42

[Oleg Romashin (:romaxa)]

Attachment: Unique ImageContainer ID counter + GetShadowImageLayerByID API

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 44

[Oleg Romashin (:romaxa)]

Attachment: LayerTree Update API

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 46

[Oleg Romashin (:romaxa)]

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

Comment 47

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

Fixed in bug 745148.