987498 - [Stingray] Support hardware rendering on MediaStream

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Shelly Lin [:shelly]]

Due to different requirements, some platforms prefer rendering the media contents by their own hardware driver, e.g. drop frames is not allowed in TV playback.

We like to create a abstract layer between the source coming from Tuner, and its consumer (the video tag).

Can be named to "HWOverlayMediaStream" or something better else.

Comment 1

[Shelly Lin [:shelly]]

Attachment: The relationship overview of TVManagerService and MediaStream

This graph simply depicts the flow from an user select a tv channel to the stage where the selected tv content is displayed on screen.

Comment 2

[Shelly Lin [:shelly]]

Attachment: The relationship overview of TVManagerService and MediaStream

Comment 3

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: Create hardware overlay MediaStream and other related modules

Assume that all methods run on main thread for now.

Comment 4

[Shelly Lin [:shelly]]

Comment on attachment 8423687 [details] [diff] [review]
[WIP] Part1: Create hardware overlay MediaStream and other related modules

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

Hey Gene,

Assuming that TVManager is the owner of TVManagerService, and TVManagerService is responsible for creating MediaStream. Does that mean that the TVManager is going to own and manage a bunch of MediaStream?

::: content/media/hwoverlay/FakeTVManagerService.cpp
@@ +50,5 @@
> +  // TODO: Generate an unique Id.
> +  // TODO: Get the inner window.
> +  HWOverlayStreamID uniqueId = 111;
> +  nsRefPtr<DOMHWOverlayMediaStream> stream =
> +    DOMHWOverlayMediaStream::CreateDOMHWOverlayMediaStream(nullptr/*nsIDOMWindow**/, this, uniqueId);

So here is where it creates a MediaStream. Apparently it needs an window object, is there any way to get it through TVManagerService?

Comment 5

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8423687 [details] [diff] [review]
[WIP] Part1: Create hardware overlay MediaStream and other related modules

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

My current design is the TVManagerService is going to be running on the parent process and the TVManager is going to be running on the child process. They will communicate with each via IPC. As my very first investigation, DOMMediaStream doesn't handle IPC stuff so I started thinking we should let TVManager new the DOMMediaStream.

::: content/media/hwoverlay/FakeTVManagerService.cpp
@@ +50,5 @@
> +  // TODO: Generate an unique Id.
> +  // TODO: Get the inner window.
> +  HWOverlayStreamID uniqueId = 111;
> +  nsRefPtr<DOMHWOverlayMediaStream> stream =
> +    DOMHWOverlayMediaStream::CreateDOMHWOverlayMediaStream(nullptr/*nsIDOMWindow**/, this, uniqueId);

That's exactly one potential bottle neck we'd face here because we cannot pass the window through the IPC. We have to create MediaStream on the TVManager which is running on the content window/process.

Comment 6

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: Create hardware overlay MediaStream and other related modules

This patch includes the ability to test through getUserMedia.

Comment 7

[Shelly Lin [:shelly]]

http://people.mozilla.org/~slin/tv/test_tv.html

Hey Solomon, feel free to test it with the above test page. Thanks!

Comment 8

[Shelly Lin [:shelly]]

Attachment: [WIP] Part2: Indicate the source of an element is hardware overlay or not

Solomon, please see this patch as well, thanks.

Comment 9

[Shelly Lin [:shelly]]

Attachment: The overview of TVManager and HWOverlayMediaStream

Comment 10

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: Create hardware overlay MediaStream and other related modules

Comment 11

[Shelly Lin [:shelly]]

Attachment: [WIP] Part2: Invoke hardware overlay MediaStream w/ gUM

This is for testing purpose only.

Comment 12

[Shelly Lin [:shelly]]

Comment on attachment 8446967 [details] [diff] [review]
[WIP] Part1: Create hardware overlay MediaStream and other related modules

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

Hi Roc, 
I've been away for the Panasonic workshop, and they all seem to buy our proposal of using hardware overlay media stream. This is my first draft of HWOverlayMediaStream, and I've referenced most of them from our camera preview module. Our goal is this design will fit not only on TV platform, but also any platform that desire using hardware rendering, bypassing the gecko's media pipeline. Could you give me some feedback on the implementing direction or the design structure?

Please find the first attachment "The overview of TVManager and HWOverlayMediaStream" for more details.

We've also dug out more problems from them too:
- When playing a recorded tv program, our MediaDecoder package (DecoderRecoder, MediaResource(?)) needs to support hardware overlay.
- Also applies on playing Internet streaming (e.g. RTSP).
- When playing EME content, our MSE needs to support hardware overlay as well?

Oh and Henri suggested we should use "underlay" over "overlay", make sense to me, I don't have any strong preference of using "overlay" though.

Thank you :)

Comment 13

[Shelly Lin [:shelly]]

Attachment: Test page of using hardware overlay media stream with gUM

Comment 14

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

(In reply to Shelly Lin [:shelly] from comment #12)
> I've been away for the Panasonic workshop, and they all seem to buy our
> proposal of using hardware overlay media stream. This is my first draft of
> HWOverlayMediaStream, and I've referenced most of them from our camera
> preview module. Our goal is this design will fit not only on TV platform,
> but also any platform that desire using hardware rendering, bypassing the
> gecko's media pipeline. Could you give me some feedback on the implementing
> direction or the design structure?
> 
> Please find the first attachment "The overview of TVManager and
> HWOverlayMediaStream" for more details.
> 
> We've also dug out more problems from them too:
> - When playing a recorded tv program, our MediaDecoder package
> (DecoderRecoder, MediaResource(?)) needs to support hardware overlay.
> - Also applies on playing Internet streaming (e.g. RTSP).
> - When playing EME content, our MSE needs to support hardware overlay as
> well?
> 
> Oh and Henri suggested we should use "underlay" over "overlay", make sense
> to me, I don't have any strong preference of using "overlay" though.

I think earlier I had suggested using a new Image type, say HWStreamImage, to represent a stream of video images being produced by hardware. What did you think of that idea? Can you compare it to your proposal here? If we do that we probably don't need new MediaStream types or new Layer types, and we can use it for <video> elements playing both recorded media resources and MediaStreams.

Comment 15

[Shelly Lin [:shelly]]

With only a new Image type (e,g, HWStreamImage), how does Gecko know whether this video should use the HWStreamImage or not? Should there be a TVMediaStream, similar to the CameraPreviewMediaStream, but has an attribute setting whether it should use hardware overlay or not?

Comment 16

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

The producer of video frames decides whether to use HWStreamImage. If we have a MediaStream coming from the TV tuner API, then the TV tuner code will be responsible for producing video frames into a SourceMediaStream and it should produce one or more HWStreamImages and set them as the image(s) for the SourceMediaStream.

If we were using the hardware to decode and render regular <video> resources, then the *Reader class wrapping that hardware would be responsible for stuffing HWStreamImage(s) into the ImageContainer that we're rendering into.

Comment 17

[Shelly Lin [:shelly]]

Attachment: [Draft] Using HW overlay image type

Would the structure be something like this, if using a new type of image, say the HWStreamImage? How about the rendering part? I'm not familiar with the rendering, is there any missing modules that I should take care of?

Comment 18

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

(In reply to Shelly Lin [:shelly] from comment #17)
> Would the structure be something like this, if using a new type of image,
> say the HWStreamImage? How about the rendering part? I'm not familiar with
> the rendering, is there any missing modules that I should take care of?

That looks right, although of course there's a lot of other things to change.

For rendering you'll need a way to pass your HWStreamImage to the compositor process. I assume these things will have some kind of ID or descriptor associated with the underlying hardware. That will require changes to PImageBridge and other places in layers, but hopefully not much. Nicolas Silva can give you more advice on that. And then in the compositor, once it's built a complete layer tree for the entire screen, you'll need to actually expose the image geometry to the underlying low-level compositor/hardware. This basically means exposing the size of the image and the transform affecting the ImageLayer (combined with the transforms of all its ancestor layers) to the underlying low-level compositor/hardware. That's going to require coordination with the hardware vendor I suppose.

In Taipei we talked about building a simplified layer tree that gets passed to the low-level compositor. I don't know if that's still the plan, and if so, if anyone's working o nit.

Comment 19

[Shelly Lin [:shelly]]

Got it, thank you very much!

Comment 20

[Solomon Chiu [:schiu]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> (In reply to Shelly Lin [:shelly] from comment #17)
> > Would the structure be something like this, if using a new type of image,
> > say the HWStreamImage? How about the rendering part? I'm not familiar with
> > the rendering, is there any missing modules that I should take care of?
> 
> That looks right, although of course there's a lot of other things to change.
> 
> For rendering you'll need a way to pass your HWStreamImage to the compositor
> process. I assume these things will have some kind of ID or descriptor
> associated with the underlying hardware. That will require changes to
> PImageBridge and other places in layers, but hopefully not much. Nicolas
> Silva can give you more advice on that. And then in the compositor, once
> it's built a complete layer tree for the entire screen, you'll need to
> actually expose the image geometry to the underlying low-level
> compositor/hardware. This basically means exposing the size of the image and
> the transform affecting the ImageLayer (combined with the transforms of all
> its ancestor layers) to the underlying low-level compositor/hardware. That's
> going to require coordination with the hardware vendor I suppose.
> 
> In Taipei we talked about building a simplified layer tree that gets passed
> to the low-level compositor. I don't know if that's still the plan, and if
> so, if anyone's working o nit.

I am modifying the design of bug#1002823 according your previous comments, below are the corresponding plans:

1. To pass to new kind of image(HWStreamImage or OverlayImage) to compositor: whenever TV screen change occurs(such as resizing, input change, or change to PIP/POP/PAP mode...etc), the class DOMTVMediaStream will be notified, and then use VideoFrameContainer::SetCurrentFrame() to pass the size and relative handle to ImageContainer. We will need a new kind of operations - says OpOverlayImageChange, to pass the information from content side to compositor side. Then CompositableParentManager can expose those information of OverlayImage to compositor side.

2. The final composition need to overlay UI(RGBA framebuffer from CompositorOGL) with TV inputs(one or multiple). By introducing HwcComposer2D (which is not used in original vendor’s prototype), we can perform the the overlay function through Prepare() and Commit(). As the design in Android gonk, the vendor should implement driver manipulation job in the HWC HAL, such that HwcComposer2D can calls corresponding  HAL functions - set(), prepare(), and query().

Comment 21

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: Core structure of how MediaStream supports images rendered by hardware

Comment 22

[Shelly Lin [:shelly]]

Attachment: [WIP] Part2: Request a fake TVTuner w/ gUM

Comment 23

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: Core structure of how MediaStream supports images rendered by hardware

Rebase to the latest m-c.

Comment 24

[Shelly Lin [:shelly]]

Comment on attachment 8464598 [details] [diff] [review]
[WIP] Part1: Core structure of how MediaStream supports images rendered by hardware

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

::: content/media/tv/HwMediaStreamListener.cpp
@@ +89,5 @@
> +        // Append this image to a VideoSegment.
> +        VideoSegment segment;
> +        segment.AppendFrame(result.forget(),
> +                            FAKE_VIDEO_FRAME_DURATION,
> +                            FAKE_VIDEO_FRAME_SIZE);

We will create and append a HWStreamImage (or OverlayImage, whatever it'll be named, worked in bug 1002823), and pass an ID to that image instance.

::: content/media/tv/HwMediaStreamListener.h
@@ +20,5 @@
> +                           MediaStreamGraphEvent aEvent) MOZ_OVERRIDE;
> +
> +  // This notification is called by the TVTuner whenever there is a track
> +  // changed, and will also update the track table maintained by
> +  // DOMTVMediaStream, thus, it should be called on the main thread.

This is a temporary solution, it will depend on the design of TVTuner.

Comment 25

[Shelly Lin [:shelly]]

Attachment: Structure Overview of Rendering TV Streams

Since bug 998872 and bug 1002823 are done, we have a clearer view on the overall structure design. Please reference the attachment - Structure Overview of Rendering TV Streams - for more details.

---
[Blue] Design for partners to implement their propitiatory services, and suggest them follow our interface design.

[Purple] Modules of TV Manager API, these are/will checked-in to master.

[Green] Modules related with hardware rendering, these will checked-in to master.

[Grey] Existed gecko modules.

---
This bug will be divided into the following parts:
1. TVTuner should send notifications about the size change of source images, and should response for requests of overlay IDs.

2. DOMHwMediaStream is the base class of MediaStream who renders OverlayImage to video outputs. DOMTVMediaStream is the sub class of DOMHwMediaStream, specializing for the usage of TVTuner.

3. HwMediaStreamListener listeners to notifications sent from MSG, and send requests to DOMHwMediaStream.

Comment 26

[Shelly Lin [:shelly]]

Attachment: Structure Overview of Rendering TV Streams (v2)

Updated per discussion with Roc during Portland work week.

Comment 27

[Shelly Lin [:shelly]]

Attachment: [TEST] Remove the permission check of tv api

Comment 28

[Shelly Lin [:shelly]]

Attachment: [TEST] Undef the build flag for OverlayImage

Comment 29

[Shelly Lin [:shelly]]

Attachment: [WIP] Implementation of DOMTVMediaStream

Comment 30

[Shelly Lin [:shelly]]

Attachment: [TEST] Show the OverlayImage for debugging purpose

Comment 31

[Shelly Lin [:shelly]]

Comment on attachment 8573035 [details] [diff] [review]
[WIP] Implementation of DOMTVMediaStream

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

::: dom/media/DOMMediaStream.cpp
@@ +665,5 @@
> +  nsRefPtr<Image> img = mImageContainer->CreateImage(ImageFormat::OVERLAY_IMAGE);
> +  mOverlayImage = static_cast<OverlayImage*>(img.get());
> +  mImageData.mOverlayId = 12345;
> +  mImageData.mSize.width = 300;
> +  mImageData.mSize.height = 400;

Should request an overlay id and the size of image from TVService in reality.

::: dom/media/HwMediaStreamListener.cpp
@@ +26,5 @@
> +}
> +
> +void
> +HwMediaStreamListener::NotifyConsumptionChanged(MediaStreamGraph* aGraph,
> +                                                Consumption aConsuming)

Maybe we should keep this in order to inform this HwMediaStream is about to be consumed by a video, and then create a segment with its OverlayImage, append this segment to its track to start rendering.

VideoSegment segment;
segment.AppendFrame(overlayimage.forget(), delta, size);
aSourceMediaStream->AppendToTrack(aID, &segment);

::: dom/media/MediaStreamGraph.cpp
@@ +2071,5 @@
>    GraphImpl()->AppendMessage(new Message(this, aKey));
>  }
>  
>  void
> +MediaStream::AddVideoOutputImpl(already_AddRefed<VideoFrameContainer> aContainer)

This should be removed.

::: dom/media/MediaStreamGraph.h
@@ +155,5 @@
>      EVENT_FINISHED,
>      EVENT_REMOVED,
>      EVENT_HAS_DIRECT_LISTENERS, // transition from no direct listeners
>      EVENT_HAS_NO_DIRECT_LISTENERS,  // transition to no direct listeners
> +    EVENT_VIDEO_OUTPUT_ADDED,

This should be removed.

@@ +163,5 @@
>     * Notify that an event has occurred on the Stream
>     */
>    virtual void NotifyEvent(MediaStreamGraph* aGraph, MediaStreamGraphEvent aEvent) {}
>  
> +  virtual void NotifyVideoOutputEvent(VideoFrameContainer* aVideoFrameContainer,

This should be removed.

Comment 32

[Shelly Lin [:shelly]]

Release to Daosheng since he is the one who is working on this bug.

Comment 33

[Daosheng Mu[:daoshengmu]]

Attachment: [WIP] Enable playing video through OverlayImage on DOMTVMediaStream

This patch has done that:
1. Append overlayImage while adding tracks to mediaStream.
2. While playing the video of mediaStream, it will show the overlayImage at the first few frames. I am still working on figure out what is the best length of overlayImage frames. If the length is too short, the overlayImage will not be played.

However, I have some points that I want someone can give me information.
First, I notice that OverlayImage just draw a transparent rect on the gfx layer. If we want to support playing video on overlayImage, we need to implement the method to let it can copy the pixels of image of videoframe. Second, at HTMLMediaElement::SetupSrcMediaStreamPlayback, it will create another ImageContainer to copy the tracks from DOMTVMediaStream. But the ImageContainer type is always CompositableType::IMAGE, we need the type be unified with the overlayImage. Therefore, I make a little bit modification on this part. I am not sure this way whether will bring problems or not. Lastly, do we still need HwMediaStreamListener? As far as I know, if we need to play tracks at real-time, we have to notify current the graphTime to the mediaStream and ask it appends new frames to this track.

Comment 34

[Shelly Lin [:shelly]]

(In reply to Daosheng Mu[:daoshengmu] from comment #33)
> Created attachment 8597766 [details] [diff] [review]
> [WIP] Enable playing video through OverlayImage on DOMTVMediaStream
> 
> This patch has done that:
> 1. Append overlayImage while adding tracks to mediaStream.
> 2. While playing the video of mediaStream, it will show the overlayImage at
> the first few frames. I am still working on figure out what is the best
> length of overlayImage frames. If the length is too short, the overlayImage
> will not be played.
> 
> However, I have some points that I want someone can give me information.
> First, I notice that OverlayImage just draw a transparent rect on the gfx
> layer. If we want to support playing video on overlayImage, we need to
> implement the method to let it can copy the pixels of image of videoframe.
> Second, at HTMLMediaElement::SetupSrcMediaStreamPlayback, it will create
> another ImageContainer to copy the tracks from DOMTVMediaStream. But the
> ImageContainer type is always CompositableType::IMAGE, we need the type be
> unified with the overlayImage. Therefore, I make a little bit modification
> on this part. I am not sure this way whether will bring problems or not.
> Lastly, do we still need HwMediaStreamListener? As far as I know, if we need
> to play tracks at real-time, we have to notify current the graphTime to the
> mediaStream and ask it appends new frames to this track.

Hi Daosheng,

Could you resubmit a patch that is base on the top of current master but my WIP patch?
I think it's okay to have HTMLMediaElement create another image container base on the image type used by MediaStream, if that's necessary.
Please remove the HwMediaStreamListener if that's unnecessary.

Comment 35

[Shelly Lin [:shelly]]

Comment on attachment 8597766 [details] [diff] [review]
[WIP] Enable playing video through OverlayImage on DOMTVMediaStream

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

::: dom/tv/DOMTVMediaStream.cpp
@@ +104,5 @@
>        case MediaSegment::VIDEO:
> +        srcStream->AddTrack(aTrackId, 0, new VideoSegment());  // 0 -> 600 to solve no frame
> +
> +		 if (aTrackId == TRACK_VIDEO_PRIMARY) {
> +		   const StreamTime delta = 10000000;

Try not to work out a magic number, instead, figure out the root cause. Have you try giving it an offset of time when the stream is copy to playback stream?

Comment 36

[Daosheng Mu[:daoshengmu]]

Attachment: [WIP] Enable playing video through OverlayImage on DOMTVMediaStream

Resubmit the patch that is based on the top of current master and remove Shelly's modifications. I am still working on figuring out the length of videoFrame StreamTime.

Comment 37

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8598501 [details] [diff] [review]
[WIP] Enable playing video through OverlayImage on DOMTVMediaStream

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

In this patch, I am working on which is the best way to playback the overlayImage. Because in DOMTVMediaStream, I just need the first frame to be played, and it can display a transparent layer on the framebuffer. Therefore, I think that I needn't to append frames while it is consuming. However, I have a problem about the frame size of OverlayImage.

I know DOMTVMediaStream is a special case, it doesn't append frames at run-time as others. I have an idea if DOMTVMediaStream can be similar to CameraPreviewMediaStream to bypass creating TrackUnionStream as its video output. Because DOMTVMediaStream's output is always as itself, it needn't to create TrackUnionStream.

::: dom/tv/DOMTVMediaStream.cpp
@@ +27,5 @@
> +
> +const int DOMTVMediaStream::overlayimgId = 0x01;
> +const int DOMTVMediaStream::overlayimgWidth = 300;
> +const int DOMTVMediaStream::overlayimgHeight = 400;
> +

The width and height of overlayImage will be set from TVTuner API. I will update this part in next patch. How is the overlayimgId? Is it acceptable to use hardcoded 0x01?

@@ +114,5 @@
> +        srcStream->AddTrack(aTrackId, 0, new VideoSegment());
> +
> +		 if (aTrackId == TRACK_VIDEO_PRIMARY) {
> +#ifdef MOZ_WIDGET_GONK
> +			 const StreamTime delta = 10000000;

This magic number is because I have to give the videoSegment suitable frame size to avoid the data is run out before it is enabled. MediaStreamGraph will process the sourceMediaStream of DOMTVMediaStream before it is assigned as TrackUnionStream's input. Therefore, there will be possible to run out the overlayImage.

Comment 38

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

I think using a single frame with a very long duration is a fine thing to do for now.

> How is the overlayimgId? Is it acceptable to use hardcoded 0x01?

I don't know anything about this.

Comment 39

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 1: Support OverlayImage rendering on DOMTVMediaStream.

Part1:
 - Implement DOMTVMediaStream.
 - Append single frame with OverlayImage format to DOMTVMediaStream.
 - At playback, that will only play first frame once to save performance.

Next step:
In the part2, I will connect DOMTVMediaStream with TVTuner to get the correct screen size for OverlayImage. Moreover, I will fix OverlayImage ClearAllImages function as well. Depend on #bug1163473.

Comment 40

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 1: Support OverlayImage rendering on DOMTVMediaStream. (V2 patch)

Created attachment 8605746 [details] [diff] [review] [diff] [review]
Bug 987498 - Part 1: Support OverlayImage rendering on DOMTVMediaStream.

Part1:
 - Implement DOMTVMediaStream.
 - Append single frame with OverlayImage format to DOMTVMediaStream.
 - At playback, that will only play the first frame once to save performance.

V2 patch:
 - Remove unused code.

Next step:
In the part2, I will connect DOMTVMediaStream with TVTuner to get the correct screen size for OverlayImage. Moreover, I will fix OverlayImage ClearAllImages function as well. Depend on #bug1163473.

Comment 41

[Daosheng Mu[:daoshengmu]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> I think using a single frame with a very long duration is a fine thing to do
> for now.
> 
> > How is the overlayimgId? Is it acceptable to use hardcoded 0x01?
> 
> I don't know anything about this.

Thanks for your reply. I currently use STREAM_TIME_MAX as the duration, and I have checked it will only be rendered once. I think that is good. Please help me review my part1 patch.

Comment 42

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

Comment on attachment 8605765 [details] [diff] [review]
Bug 987498 - Part 1: Support OverlayImage rendering on DOMTVMediaStream. (V2 patch)

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

Please split this patch up more. We can split it at least into two pieces: layers support, and media element support. And maybe DOM support can be separated out too.

::: dom/tv/TVTuner.cpp
@@ +203,2 @@
>  
> +  mStream = stream.forget();  

trailing whitespace

::: dom/webidl/TVTuner.webidl
@@ -26,5 @@
>    readonly attribute MediaStream? stream;
>  
> -  [Throws]
> -  Promise<void> setImageSize(double width, double height);
> -

This can be removed in a separate patch right?

::: gfx/layers/ImageContainer.cpp
@@ +153,5 @@
> +
> +    // Image client type must be changed to IMAGE_OVERLAY to show the overlay image. But, while displaying the general image,
> +    // we still have to use IMAGE
> +    if (bOverlayImg)
> +    	mImageClient = ImageBridgeChild::GetSingleton()->CreateImageClient(CompositableType::IMAGE_OVERLAY).take();

Why do we need a special client type here?

Comment 43

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 1: Layers support OverlayImage. r=roc

Part1:
 - Fix bug while ImageContainer creates OverlayImage on Layers

Comment 44

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 1: Layers support OverlayImage. r=roc

Part1:
 - Fix bug while ImageContainer creates OverlayImage on Layers

Comment 45

[Daosheng Mu[:daoshengmu]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42)
> Comment on attachment 8605765 [details] [diff] [review]
> Bug 987498 - Part 1: Support OverlayImage rendering on DOMTVMediaStream. (V2
> patch)
> 
> Review of attachment 8605765 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please split this patch up more. We can split it at least into two pieces:
> layers support, and media element support. And maybe DOM support can be
> separated out too.
> 
> ::: dom/tv/TVTuner.cpp
> @@ +203,2 @@
> >  
> > +  mStream = stream.forget();  
> 
> trailing whitespace
I will fix it.
> 
> ::: dom/webidl/TVTuner.webidl
> @@ -26,5 @@
> >    readonly attribute MediaStream? stream;
> >  
> > -  [Throws]
> > -  Promise<void> setImageSize(double width, double height);
> > -
> 
> This can be removed in a separate patch right?
Yes. I think it can be removed.
> 
> ::: gfx/layers/ImageContainer.cpp
> @@ +153,5 @@
> > +
> > +    // Image client type must be changed to IMAGE_OVERLAY to show the overlay image. But, while displaying the general image,
> > +    // we still have to use IMAGE
> > +    if (bOverlayImg)
> > +    	mImageClient = ImageBridgeChild::GetSingleton()->CreateImageClient(CompositableType::IMAGE_OVERLAY).take();
> 
> Why do we need a special client type here?
That is because ImageClientOverlay has its own specific implementations. For example, ImageClientOverlay does not use TextureHost, it is just according to the width and height to draw a transparent rectangle on the screen. However, ImageContainer doesn't have a good strategy to determinate use OverlayImageClient or not. So, we will have an issue while an ImageContainer copy its OverlayImage to another ImageContainer that doesn't use OverlayImage.

Comment 46

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 2: TMLMediaElement supports playback OverlayImage. r=roc

Part2:
  - While DOMMediaStream is DOMHwMediaStream, we must create ImageClientOverlay on TrackUnionStream to make it can copy track data from DOMHwMediaStream.

Comment 47

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 3: Implement DOMTVMediaStream. r=roc

Part3:
  - Implement DOMTVMediaStream and be managed by TVTuner.

Next part:
  I will connect DOMTVMediaStream with TVTuner to get the correct screen size for OverlayImage.

Comment 48

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

Comment on attachment 8606902 [details] [diff] [review]
Bug 987498 - Part 1: Layers support OverlayImage. r=roc

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

::: gfx/layers/ImageContainer.h
@@ +298,1 @@
>  

Let's change 'flag' to an enum Mode with values SYNCHRONOUS, ASYNCHRONOUS, ASYNCHRONOUS_OVERLAY.

::: gfx/layers/Layers.h
@@ +447,4 @@
>     * available, the returned ImageContainer will forward images within layer
>     * transactions, just like if it was created with CreateImageContainer().
>     */
> +  static already_AddRefed<ImageContainer> CreateAsynchronousImageContainer(bool bUseOverlayImage = false);

Let's just have CreateImageContainer and pass the ImageContainer mode enum into it.

Comment 49

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

Comment on attachment 8606918 [details] [diff] [review]
Bug 987498 - Part 2: TMLMediaElement supports playback OverlayImage. r=roc

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

OK modulo the other changes.

Comment 50

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

Comment on attachment 8606926 [details] [diff] [review]
Bug 987498 - Part 3: Implement DOMTVMediaStream. r=roc

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

Why do we need separate DOMHwMediaStream and DOMTvMediaStream?

It seems to me we could just just have DOMHwMediaStream and use it for TV and anything else that's a MediaStream that renders through an overlay. Are we going to add new TV-only APIs to the DOMTvMediaStream?

Comment 51

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

Also I think we should be a bit more consistent about names and use DOMHWMediaStream and DOMTVMediaStream.

Comment 52

[Daosheng Mu[:daoshengmu]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #48)
> Comment on attachment 8606902 [details] [diff] [review]
> Bug 987498 - Part 1: Layers support OverlayImage. r=roc
> 
> Review of attachment 8606902 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ImageContainer.h
> @@ +298,1 @@
> >  
> 
> Let's change 'flag' to an enum Mode with values SYNCHRONOUS, ASYNCHRONOUS,
> ASYNCHRONOUS_OVERLAY.
> 
> ::: gfx/layers/Layers.h
> @@ +447,4 @@
> >     * available, the returned ImageContainer will forward images within layer
> >     * transactions, just like if it was created with CreateImageContainer().
> >     */
> > +  static already_AddRefed<ImageContainer> CreateAsynchronousImageContainer(bool bUseOverlayImage = false);
> 
> Let's just have CreateImageContainer and pass the ImageContainer mode enum
> into it.

Great! That is a good idea. That will help our interface more explicit, I will do it.

Comment 53

[Daosheng Mu[:daoshengmu]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #51)
> Also I think we should be a bit more consistent about names and use
> DOMHWMediaStream and DOMTVMediaStream.

For now, I think we can use DOMHwMediaStream to cover all the usage of MediaStream that renders through an overlayimage. For the future, we can extend DOMHwMediaStream while we find something needs to be adjusted.

Shelly, do you agree that we can remove DOMTVMediaStream and just remain DOMHwMediaStream?

Comment 54

[Shelly Lin [:shelly]]

AddTrack/RemoveTrack and management of tracks are special for DOMTVMediaStream, actually...I prefer using DOMTVMediaStream over DOMHwMediaStream, since TV is the only case of using this kind of MediaStream.

Comment 55

[Daosheng Mu[:daoshengmu]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #51)
> Also I think we should be a bit more consistent about names and use
> DOMHWMediaStream and DOMTVMediaStream.

As Comment 54, the AddTrack/RemoveTrack APIs that are only needed on TVs. Therefore, I think we separate DOMTVMediaStream with DOMHwMediaStream would be a good decision. How do you feel about that, Robert?

Comment 56

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 4: Set image size support on DOMTVMediaStream. r=roc

Part4:
  - Implement SetImageSize function on DOMTVMediaStream to allow TVTuner change the size adaptively.
  - Clear the old video segment and append a new one.
  - Update the trackInfo with the new image size.

Comment 57

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 4: Set image size support on DOMTVMediaStream (V2). r=roc

Part 4:
  - Implement SetImageSize function on DOMTVMediaStream to allow TVTuner can change the size adaptively.
  - Clear the old video segment while changing the image size and append a new one.
  - Update the trackInfo with the new image size.

V2:
  - Remove unused interface. (getImageContainer of DOMTVMediaStream)

Comment 58

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

What does DOMTVMediaStream::AddTrack/RemoveTrack actually do? Can you point me to the spec?

Comment 59

[Marco Chen [:mchen]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #58)
> What does DOMTVMediaStream::AddTrack/RemoveTrack actually do? Can you point
> me to the spec?

Shelly was on PTO today so I help to answer the question.

A DOMTVMediaStream will be bound to a specific TV Tuner referring to [1] & [2].
When user chooses the different channels in this TV Tuner, the tracks list will be different channel by channel (even program by program in a single channel). Then there should be interfaces for updating the tracks information into DOMTVMediaStream.

Or any other suggestion based on this requirement? Thanks.

[1] http://seanyhlin.github.io/TV-Manager-API/#widl-TVTuner-stream
[2] Bug 998872 - [Stingray] TV Manager API

Comment 60

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

If the TVMediaStream represents the output of a TVTuner, wouldn't you change channels by calling methods on the TVTuner? Indeed that spec has a method TVTuner.setCurrentChannel.

Maybe you're saying that AddTrack/RemoveTrack will not be exposed via WebIDL, they're just private C++ methods to add tracks to the DOMMediaStream object. But DOMMediaStream already monitors its underlying MediaStream and adds and removes DOM Track objects based on the tracks that are currently in the MediaStream (see DOMMediaStream::StreamListener).

Comment 61

[Marco Chen [:mchen]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #60)
> wouldn't you change
> channels by calling methods on the TVTuner? Indeed that spec has a method
> TVTuner.setCurrentChannel.

Yes, this is right.

> 
> Maybe you're saying that AddTrack/RemoveTrack will not be exposed via
> WebIDL, they're just private C++ methods to add tracks to the DOMMediaStream
> object. But DOMMediaStream already monitors its underlying MediaStream and
> adds and removes DOM Track objects based on the tracks that are currently in
> the MediaStream (see DOMMediaStream::StreamListener).

After discussing with Daosheng, we agree your points here. Thanks.
Will double check with Shelly then report here.

Comment 62

[Shelly Lin [:shelly]]

I see, I'm good with adding/removing tracks directly with DOMTVMediaStream->GetStream().

One thing worth to follow is that DOMTVMediaStream will deal with tracks of in-band subtitles/cc too, which is unknown to me whether gecko should implement this kind of support.

Comment 63

[Daosheng Mu[:daoshengmu]]

Regarding to subtitles, I think we can extend MediaStreamTrack to make a SubtitleStreamTrack class, likes AudioStreamTrack and VideoStreamTrack. It also would be monitored by DOMMediaStream::StreamListener. While we call SourceMediaStream::AddSubtitleTrack, it will tell DOMMediaStream::StreamListener to notify DOMMediaStream to create a CreateSubtitleTrack.

Comment 64

[Marco Chen [:mchen]]

As we discussed before, MediaElement already has track type about subtitle - text track but mediastream doesn't. Maybe we should keep them align with each other because in case of TV broadcasting stream, there are additional cc/subtitle tracks and even in-band data tracks. [1] [2]

Possible directions and priority:
  1. introduce new track type of text into media element in gecko level not webidl.
  2. To integrate the multi-tracks API of MediaElement to multi-tracks in mediastream in gecko level. (then we can control tracks selection via MediaElement but not MediaStream from Content JS)
  3. To feedback this issue to W3C and try to add this support to spec of MediaStream.

Hi roc,

May I know your suggestion about this? Thanks.

[1] http://www.w3.org/2011/webtv/wiki/MPTF/MPTF_Requirements#R3._Handling_of_In-band_Tracks
[2] https://www.w3.org/Bugs/Public/show_bug.cgi?id=13359

Comment 65

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

I think it's fine to say that a TVMediaStream can have a text track even if we don't expose that through MediaStream APIs. When such a TVMediaStream is used as the source for an HTMLMediaElement, the element should get a TextTrack and an application can use that (or Gecko can render the captions). I don't think we need to add anything to specs before implementing that. That's Marco's option #2.

When implementing text tracks for MediaStreams, I think we should try to avoid involving MediaStreamGraph. Instead we could probably just forward the text-track data through the DOMMediaStreams. That will probably be simpler and doesn't lose anything in performance since the main thread is currently responsible for rendering subtitles anyway.

But you should probably check with our partners to see what kind of performance requirements they have around subtitles. If they have very strict requirements for synchronizing subtitles with content then we might have to do something to enable compositor-driven subtitle rendering.

Comment 66

[Marco Chen [:mchen]]

(In reply to Marco Chen [:mchen] from comment #64)
>   1. introduce new track type of text into media element in gecko level not
> webidl.

new track type of text into "MediaStream" not "MediaElement". sorry for the typo

Comment 67

[Marco Chen [:mchen]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #65)
> ... I don't think we need to add anything to specs before
> implementing that. That's Marco's option #2.

Will open another bug for tracking this work.

> ... If they have very
> strict requirements for synchronizing subtitles with content then we might
> have to do something to enable compositor-driven subtitle rendering.

Got it and will check this.

Thanks for the consultant in this discussion, roc.

Comment 68

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 1: Layers support OverlayImage (V2). r=roc

Part1:
 - Fix bug while ImageContainer creates OverlayImage on Layers

V2 patch:
 - Refer to comment 48, CreateImageContainer and pass the ImageContainer mode enum into it.

Comment 69

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

Comment on attachment 8612083 [details] [diff] [review]
Bug 987498 - Part 1: Layers support OverlayImage (V2). r=roc

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

Almost there :-)

::: gfx/layers/ImageContainer.cpp
@@ +151,3 @@
>      // the refcount of this ImageClient is 1. we don't use a RefPtr here because the refcount
>      // of this class must be done on the ImageBridge thread.
> +    if (flag == ASYNCHRONOUS_OVERLAY) {

switch (flag) {
  case ASYNCHRONOUS_OVERLAY:
etc

check all cases and assert if we have an unknown flag value

::: gfx/layers/Layers.h
@@ +441,2 @@
>     */
> +  static already_AddRefed<ImageContainer> CreateImageContainer(int flag = 0);

Make this "ImageContainer::Mode flag"

Comment 70

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 1: Layers support OverlayImage (V3). r=roc

Created attachment 8612083 [details] [diff] [review] [diff] [review]
Bug 987498 - Part 1: Layers support OverlayImage (V2). r=roc

Part1:
 - Fix bug while ImageContainer creates OverlayImage on Layers

V3 patch:
 - Refer to comment 69. Adding switch-case statements for all situations in ImageContainer constructor. And using ImageContainer::Mode as the flag's variable type.

Comment 71

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 1: Layers support OverlayImage (V3). r=roc

Part1:
 - Fix bug while ImageContainer creates OverlayImage on Layers

V3 patch:
 - Refer to comment 69. Adding switch-case statements for all situations in ImageContainer constructor. And using ImageContainer::Mode as the flag's variable type.

Comment 72

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 2: HTMLMediaElement supports playback OverlayImage (V2). r=roc

Part2:
  - While DOMMediaStream is DOMHwMediaStream, we must create ImageClientOverlay on TrackUnionStream to make it can copy track data from DOMHwMediaStream.

V2:
  - Due to the change of CreateImageContainer naming.

Comment 73

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #72)
> Created attachment 8612670 [details] [diff] [review]
> Bug 987498 - Part 2: HTMLMediaElement supports playback OverlayImage (V2).
> r=roc
> 
> Part2:
>   - While DOMMediaStream is DOMHwMediaStream, we must create
> ImageClientOverlay on TrackUnionStream to make it can copy track data from
> DOMHwMediaStream.
> 
> V2:
>   - Due to the change of CreateImageContainer naming.

Hi roc,
Please help me review this patch again, it is just a little bit modification because the function name changed.

Comment 74

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 3: Implement DOMHwMediaStream (V2). r=roc

Part3:
  - Implement DOMHwMediaStream and be managed by TVTuner.

V2:
  - Refers to comment 50, removing DOMTvMediaStream and using DOMHwMediaStream to provide the APIs that TV needs.

Comment 75

[Daosheng Mu[:daoshengmu]]

Hi roc,

Could be help me review this patch? There are four patches for this bug totally. Thanks for wasting your time on me.

Comment 76

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

Sure. I've been away traveling but I'm back now.

Comment 77

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

Comment on attachment 8612670 [details] [diff] [review]
Bug 987498 - Part 2: HTMLMediaElement supports playback OverlayImage (V2). r=roc

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

::: dom/html/HTMLMediaElement.cpp
@@ +3085,5 @@
>    ChangeDelayLoadStatus(false);
>    GetSrcMediaStream()->AddAudioOutput(this);
>    SetVolumeInternal();
> +
> +  bool bUseOverlayImage = (mSrcStream->AsDOMHwMediaStream() != nullptr);

Don't need ()

@@ +3086,5 @@
>    GetSrcMediaStream()->AddAudioOutput(this);
>    SetVolumeInternal();
> +
> +  bool bUseOverlayImage = (mSrcStream->AsDOMHwMediaStream() != nullptr);
> +  VideoFrameContainer* container = nullptr;

Don't initialize this here since it's initialized on every path below.

@@ +3091,5 @@
> +
> +  if (bUseOverlayImage) {
> +    container = GetOverlayImageVideoFrameContainer();
> +  }
> +  else {

} else {

Comment 78

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

Comment on attachment 8612675 [details] [diff] [review]
Bug 987498 - Part 3: Implement DOMHwMediaStream (V2). r=roc

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

::: dom/media/DOMMediaStream.cpp
@@ +680,5 @@
> +{
> +  mImageContainer = LayerManager::CreateImageContainer(ImageContainer::ASYNCHRONOUS_OVERLAY);
> +  nsRefPtr<Image> img = mImageContainer->CreateImage(ImageFormat::OVERLAY_IMAGE);
> +
> +#ifdef MOZ_WIDGET_GONK

On non-GONK platforms I think this will produce a warning about img not being used. I suggest moving 'img' into the #ifdef block too.

@@ +752,5 @@
> +}
> +
> +#ifdef MOZ_WIDGET_GONK
> +  // Clear the old segment.
> +  track->GetSegment()->Clear();

What code does this function belong to???

Comment 79

[Daosheng Mu[:daoshengmu]]

Hi roc,

Could be help me review this patch? There are four patches for this bug totally. Thanks for wasting your time on me.(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #77)
> Comment on attachment 8612670 [details] [diff] [review]
> Bug 987498 - Part 2: HTMLMediaElement supports playback OverlayImage (V2).
> r=roc
> 
> Review of attachment 8612670 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/html/HTMLMediaElement.cpp
> @@ +3085,5 @@
> >    ChangeDelayLoadStatus(false);
> >    GetSrcMediaStream()->AddAudioOutput(this);
> >    SetVolumeInternal();
> > +
> > +  bool bUseOverlayImage = (mSrcStream->AsDOMHwMediaStream() != nullptr);
> 
> Don't need ()
> 
> @@ +3086,5 @@
> >    GetSrcMediaStream()->AddAudioOutput(this);
> >    SetVolumeInternal();
> > +
> > +  bool bUseOverlayImage = (mSrcStream->AsDOMHwMediaStream() != nullptr);
> > +  VideoFrameContainer* container = nullptr;
> 
> Don't initialize this here since it's initialized on every path below.
> 
> @@ +3091,5 @@
> > +
> > +  if (bUseOverlayImage) {
> > +    container = GetOverlayImageVideoFrameContainer();
> > +  }
> > +  else {
> 
> } else {

Ok. I will fix it.

Comment 80

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 3: Implement DOMHwMediaStream (V3). r=roc

Part3:
  - Implement DOMHwMediaStream and be managed by TVTuner.

V3:
  - Refers to comment 78, moving 'img' into the #ifdef block and remove the function that does not belong.

Comment 81

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

Comment on attachment 8616576 [details] [diff] [review]
Bug 987498 - Part 3: Implement DOMHwMediaStream (V3). r=roc

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

::: dom/media/DOMMediaStream.cpp
@@ +673,5 @@
>  }
>  
> +const int DOMHwMediaStream::DEFAULT_IMAGE_ID = 0x01;
> +const int DOMHwMediaStream::DEFAULT_IMAGE_WIDTH = 400;
> +const int DOMHwMediaStream::DEFAULT_IMAGE_HEIGHT = 300;

You can define these in the class declaration itself.

Comment 82

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 4: Set image size support on DOMTVMediaStream (V3). r=roc

Part 4:
  - Implement SetImageSize function on DOMHWMediaStream to allow TVTuner can change the size adaptively.
  - Clear the old video segment while changing the image size

V3:
  - Replace DOMTVMediaStream with DOMHWMediaStream.

Comment 83

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

Comment on attachment 8616618 [details] [diff] [review]
Bug 987498 - Part 4: Set image size support on DOMTVMediaStream (V3). r=roc

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

Some minor changes required.

::: dom/media/DOMMediaStream.cpp
@@ +778,5 @@
> +  mozilla::gfx::IntSize size = image->GetSize();
> +  VideoSegment segment;
> +
> +  segment.AppendFrame(image.forget(), delta, size);
> +  srcStream->AppendToTrack(TRACK_VIDEO_PRIMARY, &segment);

You're not really supposed to wipe out the existing contents of a track's segment. That can horribly confuse consumers of MediaStreams, although in this particular case maybe you can get away with it since you can't really have consumers of these TV streams currently, except for rendering them directly to a media element.

At some point this year I should be able to refactor MSG video so we're not storing video frames in track segments at all, at which point this problem should go away.

If this code works for you, then I'll accept it if you add big XXX comments explaining that clearing the segment of a track is Very Bad Thing.

::: dom/tv/TVTuner.h
@@ +31,4 @@
>  
>    static already_AddRefed<TVTuner> Create(nsPIDOMWindow* aWindow,
>                                            nsITVTunerData* aData);
> +  nsresult NotifyImageSizeChanged(uint32_t aWidth, uint32_t aHeight);

What's going to call this?

Comment 84

[Daosheng Mu[:daoshengmu]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #81)
> Comment on attachment 8616576 [details] [diff] [review]
> Bug 987498 - Part 3: Implement DOMHwMediaStream (V3). r=roc
> 
> Review of attachment 8616576 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/DOMMediaStream.cpp
> @@ +673,5 @@
> >  }
> >  
> > +const int DOMHwMediaStream::DEFAULT_IMAGE_ID = 0x01;
> > +const int DOMHwMediaStream::DEFAULT_IMAGE_WIDTH = 400;
> > +const int DOMHwMediaStream::DEFAULT_IMAGE_HEIGHT = 300;
> 
> You can define these in the class declaration itself.

Hi roc,

Do you mean I should make these variables as private const variables. So I just need to initialize them while the class is declared. Like this way:

class DOMHwMediaStream
{

private:
  const int DEFAULT_IMAGE_ID = 0x01;
  const int DEFAULT_IMAGE_WIDTH = 400;
  const int DEFAULT_IMAGE_HEIGHT = 300;
}

Comment 85

[Daosheng Mu[:daoshengmu]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #83)
> Comment on attachment 8616618 [details] [diff] [review]
> Bug 987498 - Part 4: Set image size support on DOMTVMediaStream (V3). r=roc
> 
> Review of attachment 8616618 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some minor changes required.
> 
> ::: dom/media/DOMMediaStream.cpp
> @@ +778,5 @@
> > +  mozilla::gfx::IntSize size = image->GetSize();
> > +  VideoSegment segment;
> > +
> > +  segment.AppendFrame(image.forget(), delta, size);
> > +  srcStream->AppendToTrack(TRACK_VIDEO_PRIMARY, &segment);
> 
> You're not really supposed to wipe out the existing contents of a track's
> segment. That can horribly confuse consumers of MediaStreams, although in
> this particular case maybe you can get away with it since you can't really
> have consumers of these TV streams currently, except for rendering them
> directly to a media element.
> 
> At some point this year I should be able to refactor MSG video so we're not
> storing video frames in track segments at all, at which point this problem
> should go away.
> 
> If this code works for you, then I'll accept it if you add big XXX comments
> explaining that clearing the segment of a track is Very Bad Thing.
> 
That is why I call track->GetSegment()->Clear(), I suppose to this way can let me clear the existing content of this track's segment, and it exactly set mDuration to 0 and clear mChunks. Lastly, I give this track a new segment to playback the overlayImage that has new size. In this way, does this track still not be cleared? 

> ::: dom/tv/TVTuner.h
> @@ +31,4 @@
> >  
> >    static already_AddRefed<TVTuner> Create(nsPIDOMWindow* aWindow,
> >                                            nsITVTunerData* aData);
> > +  nsresult NotifyImageSizeChanged(uint32_t aWidth, uint32_t aHeight);
> 
> What's going to call this?

TVTuner::NotifyImageSizeChanged will be called by nsITVService.idl to adjust the image size of overlayImage.

Comment 86

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 2: HTMLMediaElement supports playback OverlayImage (V3). r=roc

Part2:
  - While DOMMediaStream is DOMHwMediaStream, we must create ImageClientOverlay on TrackUnionStream to make it can copy track data from DOMHwMediaStream.

V3:
  - According to Comment 77, adjusting coding style.

Comment 87

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #84)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #81)
> > Comment on attachment 8616576 [details] [diff] [review]
> > Bug 987498 - Part 3: Implement DOMHwMediaStream (V3). r=roc
> > 
> > Review of attachment 8616576 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/media/DOMMediaStream.cpp
> > @@ +673,5 @@
> > >  }
> > >  
> > > +const int DOMHwMediaStream::DEFAULT_IMAGE_ID = 0x01;
> > > +const int DOMHwMediaStream::DEFAULT_IMAGE_WIDTH = 400;
> > > +const int DOMHwMediaStream::DEFAULT_IMAGE_HEIGHT = 300;
> > 
> > You can define these in the class declaration itself.
 
 Hi roc,
 
 Do you mean I should make these variables as private const variables. So I
 just need to initialize them while the class is declared. Like this way:
 
 class DOMHwMediaStream
 {
 
 private:
   const int DEFAULT_IMAGE_ID = 0x01;
   const int DEFAULT_IMAGE_WIDTH = 400;
   const int DEFAULT_IMAGE_HEIGHT = 300;
 }

Comment 88

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #85)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #83)
> > Comment on attachment 8616618 [details] [diff] [review]
> > Bug 987498 - Part 4: Set image size support on DOMTVMediaStream (V3). r=roc
> > 
> > Review of attachment 8616618 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Some minor changes required.
> > 
> > ::: dom/media/DOMMediaStream.cpp
> > @@ +778,5 @@
> > > +  mozilla::gfx::IntSize size = image->GetSize();
> > > +  VideoSegment segment;
> > > +
> > > +  segment.AppendFrame(image.forget(), delta, size);
> > > +  srcStream->AppendToTrack(TRACK_VIDEO_PRIMARY, &segment);
> > 
> > You're not really supposed to wipe out the existing contents of a track's
> > segment. That can horribly confuse consumers of MediaStreams, although in
> > this particular case maybe you can get away with it since you can't really
> > have consumers of these TV streams currently, except for rendering them
> > directly to a media element.
> > 
> > At some point this year I should be able to refactor MSG video so we're not
> > storing video frames in track segments at all, at which point this problem
> > should go away.
> > 
> > If this code works for you, then I'll accept it if you add big XXX comments
> > explaining that clearing the segment of a track is Very Bad Thing.
> > 
 That is why I call track->GetSegment()->Clear(), I suppose to this way can
 let me clear the existing content of this track's segment, and it exactly
 set mDuration to 0 and clear mChunks. Lastly, I give this track a new
 segment to playback the overlayImage that has new size. In this way, Is
 this track still not cleared? 
 
> > ::: dom/tv/TVTuner.h
> > @@ +31,4 @@
> > >  
> > >    static already_AddRefed<TVTuner> Create(nsPIDOMWindow* aWindow,
> > >                                            nsITVTunerData* aData);
> > > +  nsresult NotifyImageSizeChanged(uint32_t aWidth, uint32_t aHeight);
> > 
> > What's going to call this?
> 
 TVTuner::NotifyImageSizeChanged will be called by nsITVService.idl to adjust
 the image size of overlayImage.

Comment 89

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

(In reply to Daosheng Mu[:daoshengmu] from comment #84)
> Hi roc,
> 
> Do you mean I should make these variables as private const variables. So I
> just need to initialize them while the class is declared. Like this way:
> 
> class DOMHwMediaStream
> {
> 
> private:
>   const int DEFAULT_IMAGE_ID = 0x01;
>   const int DEFAULT_IMAGE_WIDTH = 400;
>   const int DEFAULT_IMAGE_HEIGHT = 300;
> }

Yes

Comment 90

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

(In reply to Daosheng Mu[:daoshengmu] from comment #88)
>  That is why I call track->GetSegment()->Clear(), I suppose to this way can
>  let me clear the existing content of this track's segment, and it exactly
>  set mDuration to 0 and clear mChunks. Lastly, I give this track a new
>  segment to playback the overlayImage that has new size. In this way, Is
>  this track still not cleared? 

I'm just saying you're not supposed to change existing content in a track.

> > > ::: dom/tv/TVTuner.h
> > > @@ +31,4 @@
> > > >  
> > > >    static already_AddRefed<TVTuner> Create(nsPIDOMWindow* aWindow,
> > > >                                            nsITVTunerData* aData);
> > > > +  nsresult NotifyImageSizeChanged(uint32_t aWidth, uint32_t aHeight);
> > > 
> > > What's going to call this?
> > 
>  TVTuner::NotifyImageSizeChanged will be called by nsITVService.idl to adjust
>  the image size of overlayImage.

OK

Comment 91

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 3: Implement DOMHwMediaStream (V4). r=roc

Part3:
  - Implement DOMHwMediaStream and be managed by TVTuner.

V4:
  - Refers to comment 81, define some variables in the class declaration itself.

Comment 92

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 4: Set image size support on DOMTVMediaStream (V4). r=roc

Part 4:
  - Implement SetImageSize function on DOMHWMediaStream to allow TVTuner can change the size adaptively.
  - Clear the old video segment while changing the image size

V4:
  - According to comment 83, add some comments to explain that clearing the segment of a track is a Bad thing.

Comment 93

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 987498 - Part 3: Implement DOMHwMediaStream (V5). r=roc

Part3:
  - Implement DOMHwMediaStream and be managed by TVTuner.

V5:
  - Adjust #ifdef MOZ_WIDGET_GONK location to resolve try-server build failed.

Comment 94

[Daosheng Mu[:daoshengmu]]

Try link, https://treeherder.mozilla.org/#/jobs?repo=try&revision=b89be4c33953

Comment 95

[Daosheng Mu[:daoshengmu]]

Please land attachment 8612151 [details] [diff] [review], attachment 8617754 [details] [diff] [review], attachment 8624056 [details] [diff] [review], and attachment 8622390 [details] [diff] [review] to m-c.

Comment 96

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2052319b3658
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d2443335673
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dd150ccfc44
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fba7d43e2df

Comment 97

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2052319b3658
https://hg.mozilla.org/mozilla-central/rev/8d2443335673
https://hg.mozilla.org/mozilla-central/rev/7dd150ccfc44
https://hg.mozilla.org/mozilla-central/rev/6fba7d43e2df