Closed Bug 1002823 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Graphics: Layers, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: schiu, Assigned: chiajung, NeedInfo)

References

Details

(Whiteboard: [FT:Stream3])

Attachments

(4 files, 10 obsolete files)

311.06 KB, application/pdf
Details
3.53 KB, patch
Details | Diff | Splinter Review
23.35 KB, patch
chiajung
: review+
Details | Diff | Splinter Review
8.61 MB, application/octet-stream
Details
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.
Blocks: 980768
Depends on: 987498
Assignee: nobody → schiu
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.
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.
Flags: needinfo?(roc)
Flags: needinfo?(milan)
Bas, Jeff, could you review and comment?
Flags: needinfo?(milan)
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(bas)
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.
Flags: needinfo?(roc)
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.
(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.
Flags: needinfo?(bas)
Whiteboard: [FT:Stream3]
Blocks: 1026358
No longer blocks: 980768
Blocks: 980768
feature-b2g: --- → 2.1
Solomon, as for the rendering side, please see bug 987498, comment 18 for more details, thanks.
(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.
Flags: needinfo?(roc)
(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.
Blocks: Stream3_2.1
Sorry about the delay reviewing this document. You should remind me if I haven't responded within a few days.
Flags: needinfo?(roc)
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.
(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.
(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.
Attached file layer-test.zip (obsolete) —
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.
Attachment #8465154 - Attachment description: WIP: new image type for TV, and relative ImageLayer modification → WIP-1: new image type for TV, and relative ImageLayer modification
No longer depends on: 987498
Blocks: 987498
QA Whiteboard: [2.1-feature-qa+]
Attached patch WIP V1.1 (obsolete) — Splinter Review
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.
Attachment #8465154 - Attachment is obsolete: true
Attached patch Test patch (obsolete) — Splinter Review
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.
Attached file Test webapp (obsolete) —
Update the test app to reflect other patchs' change.
Attachment #8465157 - Attachment is obsolete: true
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.
Flags: needinfo?(nical.bugzilla)
Attachment #8467546 - Flags: feedback?(mtseng)
(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.
Assignee: schiu → chung
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.
Attachment #8467546 - Flags: feedback?(mtseng) → feedback+
Blocks: 1049296
QA Whiteboard: [2.1-feature-qa+] → [2.1-feature-qa?]
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.
Flags: needinfo?(nical.bugzilla)
(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
Flags: needinfo?(nical.bugzilla)
Flags: needinfo?(chung)
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.
Flags: needinfo?(chung)
(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.
Flags: needinfo?(nical.bugzilla)
QA Whiteboard: [2.1-feature-qa?]
Whiteboard: [FT:Stream3] → [FT:Stream3][2.1-feature-qa+]
Flags: in-moztrap?(fyen)
QA Contact: fyen
Attached patch WIP v2 rebased (obsolete) — Splinter Review
Create a new kind of ImageClient/Host to handle no buffer case.
Attachment #8467546 - Attachment is obsolete: true
Attachment #8471345 - Flags: feedback?(mtseng)
Attached patch Test patch v2Splinter Review
update to reflect some naming change
Attachment #8467550 - Attachment is obsolete: true
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.
Attached patch WIP v2.1 (obsolete) — Splinter Review
Fix the initial value, and type mentioned by boris.
Attachment #8471345 - Attachment is obsolete: true
Attachment #8471345 - Flags: feedback?(mtseng)
Attachment #8471443 - Flags: feedback?(mtseng)
Attached patch WIP V2.2 (obsolete) — Splinter Review
Fix one more initial value, and a possible leak.
Attachment #8471443 - Attachment is obsolete: true
Attachment #8471443 - Flags: feedback?(mtseng)
Attachment #8472014 - Flags: feedback?(mtseng)
Attachment #8472014 - Flags: feedback?(boris.chiou)
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)
Attachment #8472014 - Flags: feedback?(boris.chiou)
Attached patch WIP V2.3 (obsolete) — Splinter Review
Fix more init ordering, initial values.
Attachment #8472014 - Attachment is obsolete: true
Attachment #8472014 - Flags: feedback?(mtseng)
Attachment #8472096 - Flags: feedback?(mtseng)
Attachment #8472096 - Flags: feedback?(boris.chiou)
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!
Target Milestone: --- → 2.1 S3 (29aug)
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
Attachment #8472096 - Flags: feedback?(mtseng) → feedback+
Attached patch V1 (obsolete) — Splinter Review
Fix the bracket.
Attachment #8472096 - Attachment is obsolete: true
Attachment #8472096 - Flags: feedback?(boris.chiou)
Attachment #8472125 - Flags: review?(nical.bugzilla)
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
Attachment #8472125 - Flags: review?(nical.bugzilla) → review+
Move to v2.2 with bug 1049296, so the whole feature can be enabled then.
feature-b2g: 2.1 → 2.2
Whiteboard: [FT:Stream3][2.1-feature-qa+] → [FT:Stream3]
Target Milestone: 2.1 S3 (29aug) → ---
No longer blocks: Stream3_2.1
Flags: in-moztrap?(fyen)
QA Contact: fyen
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.
Attached patch V1 finalSplinter Review
Add some comment, fix commit log.
Carry r+.
Attachment #8472125 - Attachment is obsolete: true
Attachment #8474981 - Flags: review+
Keywords: checkin-needed
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.
https://hg.mozilla.org/mozilla-central/rev/73d3b14d6b01
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Attached file Test app(reupload)
The old app can not be unzipped, re-upload.
Attachment #8467551 - Attachment is obsolete: true
Use feature-b2g:2.2? rather than just 2.2.
feature-b2g: 2.2 → 2.2?
Flags: needinfo?(ying.xu)
Flags: needinfo?(ying.xu)
feature-b2g: 2.2? → ---
Blocks: TV_FxOS2.5
No longer blocks: TV_FxOS2.5
You need to log in before you can comment on or make changes to this bug.