1002823 - [Stingray] Add a new kind of image layer without graphic buffer for overlaying video input

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Solomon Chiu [:schiu]]

In the platform of Stingray, the video input(tuner/HDMI) is overlayed by a dedicated 2d hardware accelerator. While composing, we only need to config the size/position to the driver without pixel data transfering. Thus we need a new kind of layer(says HWOverlayLayer) to implement above function. It should be created by the HWOverlayMediaStream(bug#987498), and then attach itself to the layer tree in proper place. And the configuration to overlay H/W should be performed in RenderLayer.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

This is one of those times where we want a design and design review before the implementation is started.  When you get to that point, could you please put those together for review? Similar to what was done for the frame uniformity work. The conclusions made in Comment 0 may very well be correct, but it isn't clear to my how we came to those conclusions and who was involved in them.

Comment 2

[Solomon Chiu [:schiu]]

Hi Milan, ROC,

I just finish the first draft of design document, the URL is:

https://docs.google.com/a/mozilla.com/document/d/1E6oUsIyHkK9Lk6w_JIpwKINVOxBZIALQCOplYbI82Ko/edit?usp=sharing

Would you please give some advice and comment for me?

Thanks.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Bas, Jeff, could you review and comment?

Comment 4

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

I think it would be simpler to keep using ImageLayers and introduce a new Image type representing the video overlay. This new Image type --- let's call it OverlayImage for now --- would not need to manage buffers and we would only need one instance per video stream. i.e. we would not call SetCurrentImage for every frame, only when necessary for size changes or other metadata changes.

Comment 5

[Henri Sivonen (:hsivonen)]

What are the use cases that require the TV/HDMI layer to be an *over*lay? It seems to me that if the TV/HDMI layer doesn't need to be transparent itself, it would be a lot simpler to make it an *under*lay to avoid the GPU having to have readback protections or taint propagation for OpenGL.

That is, the normal OpenGL pipeline would end up in an RGBA final (as far as OpenGL sees) frame buffer instead of ending up in a final RGB buffer. When there's no TV/HDMI video on the screen, this normal world frame buffer would be completely opaque.

However, when there is TV/HDMI video on the screen, the normal world would "paint" the video frame by filling the pseudo-final RGBA buffer with transparent black where the video frame is supposed to be. Thereafter, the normal world could turn that area partially opaque again, when painting e.g. captions or video controls over that area.

Then there'd be another buffer, a TV/HDMI underlay, underneath the RGBA end result of the OpenGL pipeline. The TV/HDMIv underlay would be inaccessible to OpenGL and there'd be an out-of-band mechanism for requesting particular video positioning and scaling on the underlay. Video decoded into the underlay would show through the alpha hole made in the RGBA buffer acting as the end of the normal-world OpenGL pipeline.

I believe this arrangement would address all the real use cases (assuming that real world use cases don't involve the TV/HDMI video frames being translucent themselves [and don't really even involve rotation, either]) and there's the benefit that there is no need to have any sort of readback protections or taint propagation in the OpenGL pipeline to support HDCP in the HDMI input. Readbacks would return data from the psedo-final normal-world framebuffer and never from the TV/HDMI underlay.

FWIW, Mac Performa 630 did this sort of thing, albeit with only 1-bit alpha, to punch an arbitrary-shaped hole in the QuickDraw frame buffer to allow a hardware-accelerated TV tuner frame to show through.

That is, could we introduce a single-channel (alpha) TV/HDMI video placeholder layer whose behavior would be:

 * A TV/HDMI video causes the creation of a TV/HDMI video placeholder layer in the layer stack.

 * The layer starts with all pixels zeroed.

 * When Gecko paints a placeholder for a TV/HDMI video frame, it turns the pixels occupied by the video frame to 1 in the TV/HDMI video placeholder layer.

 * When the layer is composited with layers below, pixels that are 0 don't affect pixels underneath it all and pixels that are 1 burn a hole through everything underneath by turning the corresponding pixels in the normal-world pseudo-final frame buffer into transparent black.

 * Layers composited over the TV/HDMI video placeholder layer work normally and can composite opaqueness over the hole burned into the pseudo-final frame buffer in the previous step.

Comment 6

[Bas Schouten (:bas.schouten)]

(In reply to Solomon Chiu [:schiu] from comment #2)
> Hi Milan, ROC,
> 
> I just finish the first draft of design document, the URL is:
> 
> https://docs.google.com/a/mozilla.com/document/d/
> 1E6oUsIyHkK9Lk6w_JIpwKINVOxBZIALQCOplYbI82Ko/edit?usp=sharing
> 
> Would you please give some advice and comment for me?
> 
> Thanks.

I can't seem to access this document without a Google account, could you make it accessible without? I don't think I have one.

Comment 7

[Shelly Lin [:shelly]]

Solomon, as for the rendering side, please see bug 987498, comment 18 for more details, thanks.

Comment 8

[Solomon Chiu [:schiu]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> I think it would be simpler to keep using ImageLayers and introduce a new
> Image type representing the video overlay. This new Image type --- let's
> call it OverlayImage for now --- would not need to manage buffers and we
> would only need one instance per video stream. i.e. we would not call
> SetCurrentImage for every frame, only when necessary for size changes or
> other metadata changes.

Hi ROC,

I just finish my changes of design document according your previous comments, which only introduce a new kind of Image class, instead of a new layer design. Could you give me some feedback on the implementing direction or the design structure?

Thanks.

Comment 9

[Solomon Chiu [:schiu]]

(In reply to Bas Schouten (:bas.schouten) from comment #6)
> (In reply to Solomon Chiu [:schiu] from comment #2)
> > Hi Milan, ROC,
> > 
> > I just finish the first draft of design document, the URL is:
> > 
> > https://docs.google.com/a/mozilla.com/document/d/
> > 1E6oUsIyHkK9Lk6w_JIpwKINVOxBZIALQCOplYbI82Ko/edit?usp=sharing
> > 
> > Would you please give some advice and comment for me?
> > 
> > Thanks.
> 
> I can't seem to access this document without a Google account, could you
> make it accessible without? I don't think I have one.

Hi Bas,

Just changed the permission to world readable, would please check it again?

Thanks.

Comment 10

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

Sorry about the delay reviewing this document. You should remind me if I haven't responded within a few days.

Comment 11

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

The diagram on the first page still refers to HWOverlayLayer. I assume that's just a mistake.

The rest of it looks mostly fine. However:

1) It's confusing as to whether the HW TV frames exposed through OverlayImage are actually an overlay or an underlay. This needs to be made very clear. If the content we draw is drawn over the top of the TV signal (which is what happens, as far as I know), then the TV rendering is an underlay so all terminology should refer to underlay not overlay.

2) The discussion in "high level design" section 1c suggests that our compositor will prepare an RGBA framebuffer with transparency where TV rendering is supposed to be shown through (the underlay). But in our discussions in Taiwan, my understanding was that our compositor would prepare a simplified layer tree --- the sort of simplified layer tree that Android HWC accepts --- and the HWC would be able to composite that. In the HWC model, the TV rendering not specifically an underlay nor an overlay, but just another layer that can be composed with Gecko content in any order. If we're going to support CSS transforms on TV content, and multiple TV content streams rendered at once, the HWC model is much preferred. Please confirm which model you're planning to implement.

Comment 12

[Marco Chen [:mchen]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> But in our
> discussions in Taiwan, my understanding was that our compositor would
> prepare a simplified layer tree.

Last time we visited to partner's office, they confirmed their proposal in our media workweek will not be done for the upcoming product on Setember this year. And there is no schedule for it yet.
So we need the proposal in this bug first as the basic support for HWOver/Underlay. 

By the way, I think not every TV partners need this advanced solution which depends on their improved HWC capability.

> If
> we're going to support CSS transforms on TV content, and multiple TV content
> streams rendered at once, the HWC model is much preferred. Please confirm
> which model you're planning to implement.

I think for multiple TV content streams rendered at once the solution in this bug can support it (ex: image layer with over/underlay type should contain the stream ID which can be recognized by overlay engine in kernel) too but not for CSS transforms.

Comment 13

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

OK.

Comment 14

[Henri Sivonen (:hsivonen)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> 2) The discussion in "high level design" section 1c suggests that our
> compositor will prepare an RGBA framebuffer with transparency where TV
> rendering is supposed to be shown through (the underlay).

FWIW, I believe having this capability in Gecko would also be useful for a non-Stingray B2G project.

Comment 15

[Solomon Chiu [:schiu]]

Attachment: WIP-1: new image type for TV, and relative ImageLayer modification

Comment 16

[Solomon Chiu [:schiu]]

Attachment: layer-test.zip

Test app for WIP-1. Initially the app loads a GIF animation with TV noise picture and show it on the screen. To let ImageLayer run the OverlayImage relative function, please do: "adb shell setprop debug.tv.layer 1". Then the noise picture will turn to a black rectangle to indicate a transparent box was created for this ImageLayer.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Solomon's design document (as of 31 july 2014)

Comment 18

[Chiajung Hung [:chiajung]]

Attachment: WIP V1.1

A complete implementation based on solomon's patch. This patch contains the client side flow which was missing in solomon's patch. And I removed those Android property hacks and replace it with a new BlendMode in Compositor.

However, the implementation is somewhat weird, since Image should be a buffer inside a Stream(ImageClient/ImageHost), I think a better implementation may be introducing a new ImageClient type that support opaque stream.

I will create new ImageClient in next version.

Comment 19

[Chiajung Hung [:chiajung]]

Attachment: Test patch

This patch uses the new Image to share a opaque data...
It just for test. I don't know the real implementation for this part, yet.

Comment 20

[Chiajung Hung [:chiajung]]

Attachment: Test webapp

Update the test app to reflect other patchs' change.

Comment 21

[Matt Woodrow (:mattwoodrow)]

Adding ni?nical since that touches a lot of textures/compositables code that he owns.

I agree that conceptually that the TV stream is closer to an ImageClient/Host rather than a static texture, but it's probably still cleaner to implement it as a TextureClient/Host. That way we don't need most of the ImageClient changes, since we can just as GetTextureClient on the OverlayImage. It also makes it simple to switch to a different TV stream without having to recreate the compositables. I don't think it's a disaster to have an PTexture implementation that represents a dynamic image (stream) that doesn't need explicit updates through ipc.

Comment 22

[Chiajung Hung [:chiajung]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> Adding ni?nical since that touches a lot of textures/compositables code that
> he owns.
> 
> I agree that conceptually that the TV stream is closer to an
> ImageClient/Host rather than a static texture, but it's probably still
> cleaner to implement it as a TextureClient/Host. That way we don't need most

Agree. 

> of the ImageClient changes, since we can just as GetTextureClient on the

We do not have such API for Images now, and the ImageClient send the image differently with different message based on ImageType. So new TextureClient/Host may not needed.

> OverlayImage. It also makes it simple to switch to a different TV stream
> without having to recreate the compositables. I don't think it's a disaster
> to have an PTexture implementation that represents a dynamic image (stream)
> that doesn't need explicit updates through ipc.

The WIP exactly share the stream in a way similar to PTexture without recreate PCompositable. If HTMLVideoElement does not kill mVideoFrameContainer.

So let's stick to current implementation for simplicity.

If you mean to make PTexture support something like streaming texture, then I can try to do it later.

Comment 23

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8467546 [details] [diff] [review]
WIP V1.1

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

Looks good.

::: gfx/layers/ImageContainer.cpp
@@ +83,5 @@
>  #endif
> +  if (aFormat == ImageFormat::OVERLAY_IMAGE) {
> +    img = new OverlayImage();
> +    return img.forget();
> +  }

move to MOZ_WIDGET_GONK

::: gfx/layers/ImageContainer.h
@@ +931,5 @@
> +  TemporaryRef<gfx::SourceSurface> GetAsSourceSurface() { return nullptr; } ;
> +  uint32_t GetSourceId() { return mSourceId; }
> +
> +  gfx::IntSize GetSize() { return mSize; }
> +  gfx::IntSize mSize;

Should be private?

::: gfx/layers/ImageTypes.h
@@ +72,5 @@
>     * IDirect3DTexture9 in RGB32 layout.
>     */
> +  D3D9_RGB32_TEXTURE,
> +
> +  OVERLAY_IMAGE

Add a comment describes what purpose of this type.

::: gfx/layers/composite/CompositableHost.cpp
@@ +128,5 @@
>  
>  void
> +CompositableHost::UseImageSource(ImageSource aSource)
> +{
> +  return;

useless code

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1021,4 @@
>    }
>  
>    gl()->fScissor(intClipRect.x, FlipY(intClipRect.y + intClipRect.height),
> +      intClipRect.width, intClipRect.height);

nit: wrong indent.

Comment 24

[Nicolas Silva [:nical]]

Sorry for the late feedback. Conceptually this is closer to a stream than a texture so I am keen on having this implemented at the compositable level. If having it implemented as a TextureClient is simpler I'm okay with it too. The patch doesn't look too intrusive, to me so I don't have too many things to say, except that the separation between code that is useful for general image layers and whats specific to the overlay image stuff is not very clear. It probably deserves a few more comments here and there and maybe a specific ImageClient/Host class or have the overlay specific stuff in specific methods, but I didn't spend very long looking into this so it's up to you. I just want to insist on that it'd be nice to keep a clear separation between things that are general and things that are specific to this feature, through the structure of the code and/or through documentation. Also I find the "ImageSource" terminology a bit confusing (in part because we have TextureSource classes nearby and because it is not obvious by the name that it is a overlay image specific thing).
I didn't give it a thorough review but overall this looks good to me.

Comment 25

[Peter Chang[:pchang]]

(In reply to Nicolas Silva [:nical] from comment #24)
> Sorry for the late feedback. Conceptually this is closer to a stream than a
> texture so I am keen on having this implemented at the compositable level.
> If having it implemented as a TextureClient is simpler I'm okay with it too.
> The patch doesn't look too intrusive, to me so I don't have too many things
> to say, except that the separation between code that is useful for general
> image layers and whats specific to the overlay image stuff is not very
> clear. It probably deserves a few more comments here and there and maybe a
> specific ImageClient/Host class or have the overlay specific stuff in
> specific methods, but I didn't spend very long looking into this so it's up
> to you. I just want to insist on that it'd be nice to keep a clear
> separation between things that are general and things that are specific to
> this feature, through the structure of the code and/or through
> documentation. Also I find the "ImageSource" terminology a bit confusing (in
> part because we have TextureSource classes nearby and because it is not
> obvious by the name that it is a overlay image specific thing).
> I didn't give it a thorough review but overall this looks good to me.

nical, chiajung, before I also tried to send the image rotation inforamtion to compositableHost. My idea is to extend PictureRect[1] as more generic image attributes but not try to add another new layer messages. Therefore, we could still reuse original ImageClient to carry this ImageAttributes which enable the capability to carry more information. How do you think?

struct ImageAttributes {
  nsRect pictureRect;
  union {
    int rotation;
    source id;
  }
}

http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/LayersMessages.ipdlh?from=LayersMessages.ipdlh&case=true#326

Comment 26

[Chiajung Hung [:chiajung]]

peter, 

That's a way to remove those changes in CompositableTransactionParent but the structure is still not very good for me. Since we are not sending an Image in this case.

In this project, the TVstream descriptor(an int32 typed opaque handle) is a stream-like object conceptually. It tells the hardware where/how to retrieve new frames, it handle streaming controls itself. FPS control, seeking, buffering, etc. Almost everything normal stream need is opaque. 

Since it do not contain buffer(Image), it's a little weird to send such information in a way that normal Images use.

Matt pointed out that we can create new kind TextureClient/Host. The idea looks very similar to EGLImage to me, which could be a compromise solution.

Comment 27

[Peter Chang[:pchang]]

(In reply to Chiajung Hung [:chiajung] from comment #26)
> peter, 
> 
> That's a way to remove those changes in CompositableTransactionParent but
> the structure is still not very good for me. Since we are not sending an
> Image in this case.
> 
> In this project, the TVstream descriptor(an int32 typed opaque handle) is a
> stream-like object conceptually. It tells the hardware where/how to retrieve
> new frames, it handle streaming controls itself. FPS control, seeking,
> buffering, etc. Almost everything normal stream need is opaque. 
> 
> Since it do not contain buffer(Image), it's a little weird to send such
> information in a way that normal Images use.
> 
Agree, it is more clear if we create a new ImageClient, OverlayImageClient to send the source information.
> Matt pointed out that we can create new kind TextureClient/Host. The idea
> looks very similar to EGLImage to me, which could be a compromise solution.

Comment 28

[Chiajung Hung [:chiajung]]

Attachment: WIP v2 rebased

Create a new kind of ImageClient/Host to handle no buffer case.

Comment 29

[Chiajung Hung [:chiajung]]

Attachment: Test patch v2

update to reflect some naming change

Comment 30

[Boris Chiou [:boris]]

Comment on attachment 8471345 [details] [diff] [review]
WIP v2 rebased

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

::: gfx/layers/ImageContainer.h
@@ +939,5 @@
> +
> +  gfx::IntSize GetSize() { return mSize; }
> +
> +private:
> +  uint32_t mOverlayId;

Should we also use int32_t here?

::: gfx/layers/LayersTypes.h
@@ +137,4 @@
>  #ifdef MOZ_WIDGET_GONK
>    // surface to render
>    android::sp<android::GraphicBuffer> mSurface;
> +  int32_t mOverlayId;

I think we should also initialize mOverlayId to 0 in the constructor.

Comment 31

[Chiajung Hung [:chiajung]]

Attachment: WIP v2.1

Fix the initial value, and type mentioned by boris.

Comment 32

[Chiajung Hung [:chiajung]]

Attachment: WIP V2.2

Fix one more initial value, and a possible leak.

Comment 33

[Boris Chiou [:boris]]

Comment on attachment 8472014 [details] [diff] [review]
WIP V2.2

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

::: gfx/layers/ImageContainer.h
@@ +925,5 @@
> +    int32_t mOverlayId;
> +    gfx::IntSize mSize;
> +  };
> +
> +  OverlayImage() : Image(nullptr, ImageFormat::OVERLAY_IMAGE) { mOverlayId = 0; }

We should also initialize this to INVALID_OVERLAY. ex. 
, mOverlayId(INVALID_OVERLAY)

::: gfx/layers/LayersTypes.h
@@ +96,4 @@
>      , mHasOwnOffset(false)
>      , mSurface(nullptr)
>      , mTexture(nullptr)
> +    , mOverlayId(INVALID_OVERLAY)

While building this patch, I got some warnings:
LayersTypes.h: In constructor 'mozilla::layers::LayerRenderState::LayerRenderState()':
LayersTypes.h:148:16: warning: 'mozilla::layers::LayerRenderState::mTexture' will be initialized after [-Wreorder]
LayersTypes.h:145:11: warning:   'int32_t mozilla::layers::LayerRenderState::mOverlayId' [-Wreorder]
LayersTypes.h:93:3: warning:   when initialized here [-Wreorder]

So I suggest the initialization list should follow the data member declaration order. ex.
, mSurface(nullptr)
, mOverlayId(INVALID_OVERLAY)
, mTexture(nullptr)

Comment 34

[Chiajung Hung [:chiajung]]

Attachment: WIP V2.3

Fix more init ordering, initial values.

Comment 35

[Jenny Liu[:jennyliu]]

We have committed this feature for v2.1 and plan to land it by the end of sprint 3.
Please let me know if you need any help from me, thanks!

Comment 36

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8472096 [details] [diff] [review]
WIP V2.3

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

Looks good

::: gfx/layers/composite/ImageHost.cpp
@@ +324,5 @@
> +ImageHostOverlay::GetRenderState()
> +{
> +  LayerRenderState state;
> +  if (mOverlay.handle().type() == OverlayHandle::Tint32_t)
> +  {

nit: left bracket should not placed at a new line

Comment 37

[Chiajung Hung [:chiajung]]

Attachment: V1

Fix the bracket.

Comment 38

[Nicolas Silva [:nical]]

Comment on attachment 8472125 [details] [diff] [review]
V1

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

Looks good overall, the doc is a bit lacking though. Please add some doc comments explaining not only that we are passing an id around, but why we are doing that and on which platforms if you think that it is a relevant thing to know when reasoning about this code. You can put one big comment somewhere and refer to it ("// see comment in Foo.h") if you want to. I'd like anyone who glances at this code to know where to look for documentation in order to understand this.

r=me with some more doc.

::: gfx/layers/ImageContainer.h
@@ +918,5 @@
>    RemoteImageData::Format mFormat;
>  };
>  
> +#ifdef MOZ_WIDGET_GONK
> +class OverlayImage : public Image {

Please add a bit of documentation here

::: gfx/layers/ImageTypes.h
@@ +73,5 @@
>     */
> +  D3D9_RGB32_TEXTURE,
> +
> +  /**
> +   * An Image type carries an opaque handle once for each stream

Needs to be a bit more precise in the comments: What's the handle for, etc.

::: gfx/layers/client/ImageClient.h
@@ +182,5 @@
>    ShadowableLayer* mLayer;
>  };
>  
> +#ifdef MOZ_WIDGET_GONK
> +class ImageClientOverlay : public ImageClient

Please add some documentation

::: gfx/layers/composite/ImageHost.cpp
@@ +296,5 @@
> +                            const gfx::Matrix4x4& aTransform,
> +                            const gfx::Filter& aFilter,
> +                            const gfx::Rect& aClipRect,
> +                            const nsIntRegion* aVisibleRegion)
> +{

You need to check that mCompositor is not null and return (with a warning) if it is.
Most Compositable types need to deal with this case so I suppose this one should too.
Oterwise add a fatal assertion here to make it clear that having a non-null compositor here is a hard prerequisite (but you may have to deal with this case at a hight level if you choose do do this, which may be tricky).

::: gfx/layers/composite/ImageHost.h
@@ +95,5 @@
>  };
>  
> +#ifdef MOZ_WIDGET_GONK
> +
> +class ImageHostOverlay : public CompositableHost {

Please add some documentation here

::: gfx/layers/ipc/CompositableForwarder.h
@@ +107,5 @@
>                                   const nsIntRect& aRect) = 0;
>  
> +#ifdef MOZ_WIDGET_GONK
> +  virtual void UseOverlaySource(CompositableClient* aCompositabl,
> +                                OverlaySource& aOverlay) = 0;

nit: const OverlaySource& aOverlay

::: gfx/layers/ipc/ImageBridgeChild.h
@@ +225,5 @@
>                                           TextureClient* aClientOnBlack,
>                                           TextureClient* aClientOnWhite) MOZ_OVERRIDE;
> +#ifdef MOZ_WIDGET_GONK
> +  virtual void UseOverlaySource(CompositableClient* aCompositable,
> +                                OverlaySource& aOverlay) MOZ_OVERRIDE;

nit: const OverlaySource& aOverlay

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +434,5 @@
>  
> +#ifdef MOZ_WIDGET_GONK
> +void
> +ShadowLayerForwarder::UseOverlaySource(CompositableClient* aCompositable,
> +                                       OverlaySource& aOverlay)

nit: const OverlaySource& aOverlay

::: gfx/layers/ipc/ShadowLayers.h
@@ +288,5 @@
>                                           TextureClient* aClientOnBlack,
>                                           TextureClient* aClientOnWhite) MOZ_OVERRIDE;
> +#ifdef MOZ_WIDGET_GONK
> +  virtual void UseOverlaySource(CompositableClient* aCompositable,
> +                                OverlaySource& aOverlay) MOZ_OVERRIDE;

nit: const OverlaySource& aOverlay

Comment 39

[Jenny Liu[:jennyliu]]

Move to v2.2 with bug 1049296, so the whole feature can be enabled then.

Comment 40

[Chiajung Hung [:chiajung]]

Comment on attachment 8472125 [details] [diff] [review]
V1

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

Thanks for review, I will upload new version and push to try server later.

::: gfx/layers/ImageContainer.h
@@ +918,5 @@
>    RemoteImageData::Format mFormat;
>  };
>  
> +#ifdef MOZ_WIDGET_GONK
> +class OverlayImage : public Image {

Done.

::: gfx/layers/ImageTypes.h
@@ +73,5 @@
>     */
> +  D3D9_RGB32_TEXTURE,
> +
> +  /**
> +   * An Image type carries an opaque handle once for each stream

Done.

::: gfx/layers/client/ImageClient.h
@@ +182,5 @@
>    ShadowableLayer* mLayer;
>  };
>  
> +#ifdef MOZ_WIDGET_GONK
> +class ImageClientOverlay : public ImageClient

Done.

::: gfx/layers/composite/ImageHost.cpp
@@ +296,5 @@
> +                            const gfx::Matrix4x4& aTransform,
> +                            const gfx::Filter& aFilter,
> +                            const gfx::Rect& aClipRect,
> +                            const nsIntRegion* aVisibleRegion)
> +{

Done.

::: gfx/layers/composite/ImageHost.h
@@ +95,5 @@
>  };
>  
> +#ifdef MOZ_WIDGET_GONK
> +
> +class ImageHostOverlay : public CompositableHost {

Done.

::: gfx/layers/ipc/CompositableForwarder.h
@@ +107,5 @@
>                                   const nsIntRect& aRect) = 0;
>  
> +#ifdef MOZ_WIDGET_GONK
> +  virtual void UseOverlaySource(CompositableClient* aCompositabl,
> +                                OverlaySource& aOverlay) = 0;

Done.

::: gfx/layers/ipc/ImageBridgeChild.h
@@ +225,5 @@
>                                           TextureClient* aClientOnBlack,
>                                           TextureClient* aClientOnWhite) MOZ_OVERRIDE;
> +#ifdef MOZ_WIDGET_GONK
> +  virtual void UseOverlaySource(CompositableClient* aCompositable,
> +                                OverlaySource& aOverlay) MOZ_OVERRIDE;

Done.

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +434,5 @@
>  
> +#ifdef MOZ_WIDGET_GONK
> +void
> +ShadowLayerForwarder::UseOverlaySource(CompositableClient* aCompositable,
> +                                       OverlaySource& aOverlay)

Done.

::: gfx/layers/ipc/ShadowLayers.h
@@ +288,5 @@
>                                           TextureClient* aClientOnBlack,
>                                           TextureClient* aClientOnWhite) MOZ_OVERRIDE;
> +#ifdef MOZ_WIDGET_GONK
> +  virtual void UseOverlaySource(CompositableClient* aCompositable,
> +                                OverlaySource& aOverlay) MOZ_OVERRIDE;

Done.

Comment 41

[Chiajung Hung [:chiajung]]

Attachment: V1 final

Add some comment, fix commit log.
Carry r+.

Comment 42

[Chiajung Hung [:chiajung]]

Try ticket:
https://tbpl.mozilla.org/?tree=Try&rev=dff685c8dd5f

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73d3b14d6b01

Comment 44

[Markus Stange [:mstange]]

Comment on attachment 8474981 [details] [diff] [review]
V1 final

>diff --git a/gfx/layers/opengl/CompositorOGL.cpp b/gfx/layers/opengl/CompositorOGL.cpp
>index 648e79c4..74e33c4 100644
>--- a/gfx/layers/opengl/CompositorOGL.cpp
>+++ b/gfx/layers/opengl/CompositorOGL.cpp
>@@ -977,6 +977,8 @@ static bool SetBlendMode(GLContext* aGL, gfx::CompositionOp aBlendMode, bool aIs
> 
>   GLenum srcBlend;
>   GLenum dstBlend;
>+  GLenum srcColorBlend = LOCAL_GL_ONE;
>+  GLenum dstColorBlend = LOCAL_GL_ONE;
> 
>   switch (aBlendMode) {
>     case gfx::CompositionOp::OP_OVER:
>@@ -994,13 +996,19 @@ static bool SetBlendMode(GLContext* aGL, gfx::CompositionOp aBlendMode, bool aIs
>       srcBlend = LOCAL_GL_DST_COLOR;
>       dstBlend = LOCAL_GL_ONE_MINUS_SRC_ALPHA;
>       break;
>+    case gfx::CompositionOp::OP_SOURCE:
>+      srcBlend = LOCAL_GL_ONE;
>+      dstBlend = LOCAL_GL_ZERO;
>+      srcColorBlend = LOCAL_GL_ONE;
>+      dstColorBlend = LOCAL_GL_ZERO;
>+      break;
>     default:
>       MOZ_ASSERT_UNREACHABLE("Unsupported blend mode!");
>       return false;
>   }
> 
>   aGL->fBlendFuncSeparate(srcBlend, dstBlend,
>-                          LOCAL_GL_ONE, LOCAL_GL_ONE);
>+                          srcColorBlend, dstColorBlend);
>   return true;
> }

I just landed a patch for bug 1055585 which slightly changes this part:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff5178d4e9f9

So if for some reason you need to back this bug out, you'll need to back out my change along with it.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73d3b14d6b01

Comment 46

[Chiajung Hung [:chiajung]]

Attachment: Test app(reupload)

The old app can not be unzipped, re-upload.

Comment 47

[Kai-Chih Hu [:khu]]

Use feature-b2g:2.2? rather than just 2.2.