1201363 - Stop buffering video in the MediaStreamGraph

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect, P1)

Description

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

Right now we buffer video frames in VideoSegments in MediaStream track data. This is unnecessary, adds latency, and reduces flexibility since once frames are buffered you can't modify them.

Better approach: for each SourceMediaStream video track producing video, have the MSG identify all the VideoFrameContainers that are rendering the output of that track, and directly set those VideoFrameContainers on the SourceMediaStream track in a thread-safe way. Every time the source produces a new video frame, iterate through the current VideoFrameContainers and set it on them. Better still, allow the source to produce a set of timestamped video frames (like media elements do now), and set that set of timestamped frames on each output VideoFrameContainer. This will eliminate video output latency. It will also let sources use their own buffering policies: e.g. media elements can decode a long queue of frames and expose that queue to all consumers, but low-end mobile cameras can force the queue to be length 1, so output VideoFrameContainers always contain only the most recent frame.

To support WebRTC, MediaRecorder and Foxeye video monitoring, we would abstract out a VideoFrameSink base class for VideoFrameContainer that just exposes the SetCurrentFrame(s) and ClearFrames() methods as pure virtual, and then create a monitoring subclasses to receive those frames.

I imagine the MSG would mostly continue working as now, just without video track data in StreamBuffer. Every tick of the MSG, if the graph configuration has changed, we'd walk the stream graph to recompute the VideoFrameSinks for each source video track.

This will be much easier to implement when we removing blocking from the MSG in bug 1189506.

For Foxeye video processing, we would have a VideoFrameSink that sends the callback to the worker. Frames produced by the worker would go into a new source video track and be propagated to its connected VideoFrameSinks --- all asynchronously with the MSG itself.

Comment 1

[Andreas Pehrson [:pehrsons]]

Sounds like this will let us throw out CameraPreviewMediaStream.

Comment 2

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Thanks for roc's explanation. Below is my plan based on comment #0.
Analysis:
The use case of not using NotifyQueuesTrackChange for TRACK_EVENT_CREATED and TRACK_EVENT_ENDED.
1. HTMLMediaElement::StreamSizeListener => To get frame size. Might be able to get the size when |SetCurrentFrame(s)| is called to set first non-zero frame. Could use original VideoFrameContainer.
2. MediaEncoder => Used in VideoTrackEncoder. Might need a subclass of VideoFrameSink.
3. CaptureTask  => Take one preview frame in webcam and encoded to jpeg. Might need a subclass of VideoFrameSink.
4. WebRTC, MediaPipelineTransimt::PipelineListener => gUM use direct listener. Others pass the frame data through NotifyQueuedTrackChange. Might need a subclass of VideoFrameSink.

According above analysis, my plan is below.
1. Implement VideoFrameSink family
2. Replace direct listener with subclass of VideoFrameSink in WebRTC gUM case.
3. Add NotifyQueuedAudioData for the usage of NotifyQueuedTrackChange but not TRACK_EVENT_CREATED/TRACK_EVENT_ENDED case.
4. Clean NotifyQueuedTrackChange to only support TRACK_EVENT_CREATED/TRACK_EVENT_ENDED
  4.1 HTMLMediaElement case => moving |PlayVideo| logic to |AppendToTrack|, replacing |NotifyQueuedTrackChange| with |NotifyQueuedAudioData| in |TrackUnionStream::CopyTrackData|, adding first frame size in SetCurrentFrame(s).
  4.2 MediaEncoder case => implementation of it's own SetCurrentFrame(s) and ClearFrames(), removing NotifyQueuedTrackChange in VideoTrackEncoder, adding VideoOuput, replacing |NotifyQueuedTrackChange| with |NotifyQueuedAudioData| in AudioTrackEncoder.
  4.3 CaptureTask => as 4.2
  4.4 WebRTC => as 4.2
  4.5 FoxEye => as 4.2

Please correct if anything wrong. :)

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (offline Oct 1-4) from comment #0)
> Right now we buffer video frames in VideoSegments in MediaStream track data.
> This is unnecessary, adds latency, and reduces flexibility since once frames
> are buffered you can't modify them.
> 
> Better approach: for each SourceMediaStream video track producing video,
> have the MSG identify all the VideoFrameContainers that are rendering the
> output of that track, and directly set those VideoFrameContainers on the
> SourceMediaStream track in a thread-safe way. Every time the source produces
> a new video frame, iterate through the current VideoFrameContainers and set
> it on them. Better still, allow the source to produce a set of timestamped
> video frames (like media elements do now), and set that set of timestamped
> frames on each output VideoFrameContainer. This will eliminate video output
> latency. It will also let sources use their own buffering policies: e.g.
> media elements can decode a long queue of frames and expose that queue to
> all consumers, but low-end mobile cameras can force the queue to be length
> 1, so output VideoFrameContainers always contain only the most recent frame.
> 
> To support WebRTC, MediaRecorder and Foxeye video monitoring, we would
> abstract out a VideoFrameSink base class for VideoFrameContainer that just
> exposes the SetCurrentFrame(s) and ClearFrames() methods as pure virtual,
> and then create a monitoring subclasses to receive those frames.
> 
> I imagine the MSG would mostly continue working as now, just without video
> track data in StreamBuffer. Every tick of the MSG, if the graph
> configuration has changed, we'd walk the stream graph to recompute the
> VideoFrameSinks for each source video track.
> 
> This will be much easier to implement when we removing blocking from the MSG
> in bug 1189506.
> 
> For Foxeye video processing, we would have a VideoFrameSink that sends the
> callback to the worker. Frames produced by the worker would go into a new
> source video track and be propagated to its connected VideoFrameSinks ---
> all asynchronously with the MSG itself.

Comment 3

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

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #2)
> The use case of not using NotifyQueuesTrackChange for TRACK_EVENT_CREATED
> and TRACK_EVENT_ENDED.
> 1. HTMLMediaElement::StreamSizeListener => To get frame size. Might be able
> to get the size when |SetCurrentFrame(s)| is called to set first non-zero
> frame. Could use original VideoFrameContainer.

Sounds good.

> 2. MediaEncoder => Used in VideoTrackEncoder. Might need a subclass of
> VideoFrameSink.

Definitely.

> 3. CaptureTask  => Take one preview frame in webcam and encoded to jpeg.
> Might need a subclass of VideoFrameSink.

Yes.

> 4. WebRTC, MediaPipelineTransimt::PipelineListener => gUM use direct
> listener. Others pass the frame data through NotifyQueuedTrackChange. Might
> need a subclass of VideoFrameSink.

Yes.

> According above analysis, my plan is below.
> 1. Implement VideoFrameSink family
> 2. Replace direct listener with subclass of VideoFrameSink in WebRTC gUM
> case.
> 3. Add NotifyQueuedAudioData for the usage of NotifyQueuedTrackChange but
> not TRACK_EVENT_CREATED/TRACK_EVENT_ENDED case.
> 4. Clean NotifyQueuedTrackChange to only support
> TRACK_EVENT_CREATED/TRACK_EVENT_ENDED

Sounds good so far...

>   4.1 HTMLMediaElement case => moving |PlayVideo| logic to |AppendToTrack|,

This is the hard part. We must avoid traversing the MSG on any thread other than the MSG thread. So give each SourceMediaStream a set of VideoFrameSinks protected by the SourceMediaStream mutex. Every MSG tick, update that set by traversing the MSG to identify all VideoFrameSinks being fed by that SourceMediaStream. Then you can modify AppendToTrack to update all the VideoFrameSinks from any thread by using that set directly. Better still, create a new method just for video that does that.

> replacing |NotifyQueuedTrackChange| with |NotifyQueuedAudioData| in
> |TrackUnionStream::CopyTrackData|, adding first frame size in
> SetCurrentFrame(s).
>   4.2 MediaEncoder case => implementation of it's own SetCurrentFrame(s) and
> ClearFrames(), removing NotifyQueuedTrackChange in VideoTrackEncoder, adding
> VideoOuput, replacing |NotifyQueuedTrackChange| with |NotifyQueuedAudioData|
> in AudioTrackEncoder.
>   4.3 CaptureTask => as 4.2
>   4.4 WebRTC => as 4.2
>   4.5 FoxEye => as 4.2
> 
> Please correct if anything wrong. :)

The rest sounds good.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #2)
> > The use case of not using NotifyQueuesTrackChange for TRACK_EVENT_CREATED
> > and TRACK_EVENT_ENDED.
> > 1. HTMLMediaElement::StreamSizeListener => To get frame size. Might be able
> > to get the size when |SetCurrentFrame(s)| is called to set first non-zero
> > frame. Could use original VideoFrameContainer.
> 
> Sounds good.

Note that size may change on any frame, so we may still need to do something additional here.  I haven't looked into what uses StreamSizeListener or the formal definition, though.

> > 4. WebRTC, MediaPipelineTransimt::PipelineListener => gUM use direct
> > listener. Others pass the frame data through NotifyQueuedTrackChange. Might
> > need a subclass of VideoFrameSink.
> 
> Yes.

Yes, and this appears to do what DirectListeners were meant to accomplish - avoid queuing delay in MSG.

> >   4.1 HTMLMediaElement case => moving |PlayVideo| logic to |AppendToTrack|,
> 
> This is the hard part. We must avoid traversing the MSG on any thread other
> than the MSG thread. So give each SourceMediaStream a set of VideoFrameSinks
> protected by the SourceMediaStream mutex. Every MSG tick, update that set by
> traversing the MSG to identify all VideoFrameSinks being fed by that
> SourceMediaStream. Then you can modify AppendToTrack to update all the
> VideoFrameSinks from any thread by using that set directly. Better still,
> create a new method just for video that does that.

IIRC from other work I did, it should be possible to create such a map of source->destinations affected that's adjusted (on the MSG thread) on each add, remove, or input port update (i.e. you only need to update on a graph change, and if you're smart you can do a partial update from the change point.  I'll search for the patch that did this (which IIRC never landed; it may have been an alternative to the simpler DirectListener approach).

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Bug 884365, and in particular patches 1-4 (3 and 4 are marked obsolete).  Patch 3 is the particularly relevant: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=884365&attachment=790635

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

There may be another patch I was thinking of; but that seems to be in the area

Comment 7

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

roc, jesup,
Thanks.
@jesup, the patch is quite useful.

(In reply to Randell Jesup [:jesup] from comment #5)
> Bug 884365, and in particular patches 1-4 (3 and 4 are marked obsolete). 
> Patch 3 is the particularly relevant:
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=884365&attachment=790635

Comment 8

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Take notes:
1. I got a solution to choose the time stamp for setCurrentFrames when AppendToTrack in video file use case. The solution is add a listener to get current timestamp of processed time for first frame and resumed frame. By using processed time and the VideoData::mTime, we can determine the first frame timestamp(t0). The time stamp of each frame will be the first frame timestamp(t0) + MiliSecondToTimeStamp(VideoData::mTime) in all case.
2. After Bug 1103188 landed, we should consider the case of addTrack/removeTrack which makes finding suitable source stream containing VideoFrameSink more complex. I will figure it out.

Comment 9

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Some issues found in video file case(using mozCaptureStream for MediaElement):
1. The SourceMediaStream will be created later than |AddVideoOuput| from |HTMLMediaElement::UpdateSrcMediaStreamPlaying|. So MediaStream need to keep the VideoFrameSink and hook to SourceMediaStream when SourceMediaStream is created.

2. Right now, the DOMMediaStreamTrack is created by |OwnedStreamListener::DoNotifyTrackCreated|. The function is triggered by |TrackUnionStream::AddTrack|. TrackUnionStream will check the StreamBuffer of input MediaStream to determine call |TrackUnionStream::AddTrack| or not. So we might still need to create an empty StreamBuffer for video case. Or another class instead of StreamBuffer.

3. When addTrack is called in JS, we need to hook the VideoFrameContainer from PlaybackStream to the SourceMediaStream of added DOMMediaStream.

Comment 10

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Hi, roc and andreas,
Does the comment #9 make sense?
Thanks.

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #9)
> Some issues found in video file case(using mozCaptureStream for
> MediaElement):
> 1. The SourceMediaStream will be created later than |AddVideoOuput| from
> |HTMLMediaElement::UpdateSrcMediaStreamPlaying|. So MediaStream need to keep
> the VideoFrameSink and hook to SourceMediaStream when SourceMediaStream is
> created.
> 
> 2. Right now, the DOMMediaStreamTrack is created by
> |OwnedStreamListener::DoNotifyTrackCreated|. The function is triggered by
> |TrackUnionStream::AddTrack|. TrackUnionStream will check the StreamBuffer
> of input MediaStream to determine call |TrackUnionStream::AddTrack| or not.
> So we might still need to create an empty StreamBuffer for video case. Or
> another class instead of StreamBuffer.
> 
> 3. When addTrack is called in JS, we need to hook the VideoFrameContainer
> from PlaybackStream to the SourceMediaStream of added DOMMediaStream.

Comment 11

[Andreas Pehrson [:pehrsons]]

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #9)
> Some issues found in video file case(using mozCaptureStream for
> MediaElement):
> 1. The SourceMediaStream will be created later than |AddVideoOuput| from
> |HTMLMediaElement::UpdateSrcMediaStreamPlaying|. So MediaStream need to keep
> the VideoFrameSink and hook to SourceMediaStream when SourceMediaStream is
> created.

This should be fine. The video output on a stream is valid for the whole stream and I guess when you propagate it to a SourceMediaStream you want to do it for a single TrackID so there is always only one track sending frames to the VideoFrameContainer.
So you'd probably have the logic for selecting which track to pass the VFC to in MediaStream and the logic for sending frames to a VFC in SourceMediaStream (locked to a certain TrackID). Also see below.


> 2. Right now, the DOMMediaStreamTrack is created by
> |OwnedStreamListener::DoNotifyTrackCreated|. The function is triggered by
> |TrackUnionStream::AddTrack|. TrackUnionStream will check the StreamBuffer
> of input MediaStream to determine call |TrackUnionStream::AddTrack| or not.
> So we might still need to create an empty StreamBuffer for video case. Or
> another class instead of StreamBuffer.

I think keeping it in an empty entry in the StreamBuffer is a good first step. You could even add some asserts to check that we're not buffering anything for video.


> 3. When addTrack is called in JS, we need to hook the VideoFrameContainer
> from PlaybackStream to the SourceMediaStream of added DOMMediaStream.

An addTrack in JS eventually boils down to TrackUnionStream::AddTrack so you can add the VideoFrameContainer there. You can proxy this through a method like MediaStream::AddVideoFrameContainerToTrack(vfc, trackID) that for TrackUnionStream forwards the call to the input track given the mapping in mTrackMap.

Comment 12

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

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #9)
> Some issues found in video file case(using mozCaptureStream for
> MediaElement):
> 1. The SourceMediaStream will be created later than |AddVideoOuput| from
> |HTMLMediaElement::UpdateSrcMediaStreamPlaying|. So MediaStream need to keep
> the VideoFrameSink and hook to SourceMediaStream when SourceMediaStream is
> created.
...
> 3. When addTrack is called in JS, we need to hook the VideoFrameContainer
> from PlaybackStream to the SourceMediaStream of added DOMMediaStream.

My idea was to update the VideoFrameSinks for each SourceMediaStream on every MSG tick. That should solve issues #1 and #3.

Comment 13

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

roc,
Thanks for the suggestion. It works. :)
About issue 2, I would like to rename StreamBuffer to StreamTracks. Because the video track still need a place to keep the track information in every MediaStream even StreamBuffer::Track::mSegment is empty.
WDYT?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #9)
> > Some issues found in video file case(using mozCaptureStream for
> > MediaElement):
> > 1. The SourceMediaStream will be created later than |AddVideoOuput| from
> > |HTMLMediaElement::UpdateSrcMediaStreamPlaying|. So MediaStream need to keep
> > the VideoFrameSink and hook to SourceMediaStream when SourceMediaStream is
> > created.
> ...
> > 3. When addTrack is called in JS, we need to hook the VideoFrameContainer
> > from PlaybackStream to the SourceMediaStream of added DOMMediaStream.
> 
> My idea was to update the VideoFrameSinks for each SourceMediaStream on
> every MSG tick. That should solve issues #1 and #3.

Comment 14

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

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #13)
> About issue 2, I would like to rename StreamBuffer to StreamTracks. Because
> the video track still need a place to keep the track information in every
> MediaStream even StreamBuffer::Track::mSegment is empty.
> WDYT?

That sounds good.

Comment 15

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Hi, roc,
Now I have a WIP changesets which pass all dom/media/test and dom/media/tests/mochitest/. And the logic is below.

1. A base class called VideoFrameSink which contains |SetCurrentFrame(s)| and ClearFrames. The function names might be discussable since the class will be used more than VideoFrameContainer only. But the functionality should cover send video frames with timestamp to the sub-class of VideoFrameSink and clear the frames.

2. A ImageSizeChangedListener for HTMLMediaElement::StreamSizeListener. Then we can make NotifyQueuedTrackChanges do less job.

3. Add NotifyQueuedAudioData into MediaStreamListener and make |MediaStreamListener::NotifyQueuedTrackChanges| only for TRACK_CREATE and TRACK_END

4. Rename StreamBuffer to StreamTracks for notify owned media stream in DOMMediaStream for the DOMMediaStreamTrack creation.

5. Add mVideoSinks(an array of VideoFrameSink) into SourceMediaStream.Every MediaStream still keep the VideoOutput as original, only change to an array of VideoFrameSink. MSG will walk through the graph(DFS) in |UpdateGraph| when the stream order is dirty and collect all VideoFrameSinks of the children of the SourceMediaStream. 

6. In original design, the |PlayVideo| will be merged to AppendToTrack. I change a little bit. I Add a function called |BroadcastToVideoFrameSink| to replace |PlayVideo|. It will be called in |ExtractPendingInput|. And the TimeStamp is calculated as mTracksStartTime plus accumulated durations and the value is the frame time in graph time. So I can use this value to transfer to the target time stamp based on current time stamp. Thanks to bug 1189506(removing blocking from MSG), it is much simpler than my imagination.
Above is current status of the WIP patches.
Some questions would like to raise.
1. Should I call |BroadcastToVideoFrameSink| in |AppendToTrack| in stead of |ExtractPendingInput|? The reason I move to |ExtractPendingInput| is the |AppendToTrack| will be called before the VideoFrameSink set of SourceMediaStream is updated. So the MSG need to copy the VideoSegments to UpdateTracks and calls |BroadcastToVideoFrameSink| in |ExtractPendingInput| in next MSG tick.

2. I will fix the cases of MediaEncoder, CaptureTask and WebRTC MediaPipelineTransimt::PipelineListener non-direct case. But I think the removing direct listener could be done in another follow-up bug.

3. And the FoxEye case will be fixed in bug 1108950 and bug 1183481.

Comment 16

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

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #15)
> Now I have a WIP changesets which pass all dom/media/test and
> dom/media/tests/mochitest/. And the logic is below.
> 
> 1. A base class called VideoFrameSink which contains |SetCurrentFrame(s)|
> and ClearFrames. The function names might be discussable since the class
> will be used more than VideoFrameContainer only. But the functionality
> should cover send video frames with timestamp to the sub-class of
> VideoFrameSink and clear the frames.

Probably should call it MediaStreamVideoSink to clearly distinguish it from VideoSink.

> 2. A ImageSizeChangedListener for HTMLMediaElement::StreamSizeListener. Then
> we can make NotifyQueuedTrackChanges do less job.
> 
> 3. Add NotifyQueuedAudioData into MediaStreamListener and make
> |MediaStreamListener::NotifyQueuedTrackChanges| only for TRACK_CREATE and
> TRACK_END
> 
> 4. Rename StreamBuffer to StreamTracks for notify owned media stream in
> DOMMediaStream for the DOMMediaStreamTrack creation.
> 
> 5. Add mVideoSinks(an array of VideoFrameSink) into SourceMediaStream.Every
> MediaStream still keep the VideoOutput as original, only change to an array
> of VideoFrameSink. MSG will walk through the graph(DFS) in |UpdateGraph|
> when the stream order is dirty and collect all VideoFrameSinks of the
> children of the SourceMediaStream. 
> 
> 6. In original design, the |PlayVideo| will be merged to AppendToTrack. I
> change a little bit. I Add a function called |BroadcastToVideoFrameSink| to
> replace |PlayVideo|. It will be called in |ExtractPendingInput|. And the
> TimeStamp is calculated as mTracksStartTime plus accumulated durations and
> the value is the frame time in graph time. So I can use this value to
> transfer to the target time stamp based on current time stamp. Thanks to bug
> 1189506(removing blocking from MSG), it is much simpler than my imagination.
>
> Above is current status of the WIP patches.
> Some questions would like to raise.
> 1. Should I call |BroadcastToVideoFrameSink| in |AppendToTrack| in stead of
> |ExtractPendingInput|? The reason I move to |ExtractPendingInput| is the
> |AppendToTrack| will be called before the VideoFrameSink set of
> SourceMediaStream is updated. So the MSG need to copy the VideoSegments to
> UpdateTracks and calls |BroadcastToVideoFrameSink| in |ExtractPendingInput|
> in next MSG tick.

It would be very good to avoid waiting for an MSG iteration to broadcast video frames to the sinks. If we get rid of that, we reduce video latency for WebRTC because the latest video frame for a call can be sent to the compositor as soon as it's decoded. So it would be good to call BroadcastToVideoFrameSink from the MSG only when a new sink is added to the set.

> 2. I will fix the cases of MediaEncoder, CaptureTask and WebRTC
> MediaPipelineTransimt::PipelineListener non-direct case. But I think the
> removing direct listener could be done in another follow-up bug.

I think we'll still need the direct listener for WebRTC audio.

This basically sounds very good. Thanks!

Comment 17

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)

> Probably should call it MediaStreamVideoSink to clearly distinguish it from
> VideoSink.
> 
Sounds good.

> > Some questions would like to raise.
> > 1. Should I call |BroadcastToVideoFrameSink| in |AppendToTrack| in stead of
> > |ExtractPendingInput|? The reason I move to |ExtractPendingInput| is the
> > |AppendToTrack| will be called before the VideoFrameSink set of
> > SourceMediaStream is updated. So the MSG need to copy the VideoSegments to
> > UpdateTracks and calls |BroadcastToVideoFrameSink| in |ExtractPendingInput|
> > in next MSG tick.
> 
> It would be very good to avoid waiting for an MSG iteration to broadcast
> video frames to the sinks. If we get rid of that, we reduce video latency
> for WebRTC because the latest video frame for a call can be sent to the
> compositor as soon as it's decoded. So it would be good to call
> BroadcastToVideoFrameSink from the MSG only when a new sink is added to the
> set.
No problem. Then the source should determine the target time stamp in stead of calculating by MSG.
Video file case => stream start time stamp + frame time should be enough.
gUM => determined by source. Could use current time stamp.

> 
> > 2. I will fix the cases of MediaEncoder, CaptureTask and WebRTC
> > MediaPipelineTransimt::PipelineListener non-direct case. But I think the
> > removing direct listener could be done in another follow-up bug.
> 
> I think we'll still need the direct listener for WebRTC audio.
Yes, you are right. Should only remove direct listener for WebRTC video case 

> 
> This basically sounds very good. Thanks!

Thanks for your feedback. :)

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #17)

> > > 2. I will fix the cases of MediaEncoder, CaptureTask and WebRTC
> > > MediaPipelineTransimt::PipelineListener non-direct case. But I think the
> > > removing direct listener could be done in another follow-up bug.
> > 
> > I think we'll still need the direct listener for WebRTC audio.
> Yes, you are right. Should only remove direct listener for WebRTC video case 

As part of the full-duplex audio work, we're likely removing DirectListener for audio as well, though this will take a bit for all the pieces to land.  We may be able to turn DirectListener off earlier though by moving some thread-bouncing to the listeners (if that can't cause delay buildup due to drift, as it does today - it might, so we may have to wait for all the full-duplex cubeb drivers to land).

Comment 19

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Some updates. I changed the WIP to |SetCurrentFrames| in |AppendToTrack|. It basically worked for most of case(VideoFrameContainer,ImageCaptrue, MediaPipeline and MediaRecorder) with some controllable minor bugs.
Now I am trying to fix the case of |AppendToTrack| will be called in |DecodedStream::SendVideo| before the VideoFrameContainer added to SourceMediaStream. After talk with JW, the solution is below:
1. Add |NotifyStreamOrderUpdated| in MediaStreamListener. This function will be called after |MediaStreamGraphImpl::UpdateStreamOrder| is done.
2. In |DecodedStream::CreateData|, the R will be created in |DecodedStreamGraphListener::NotifyStreamOrderUpdated|.
3. In |DecodedStream::SendData|, skip until the StreamOrder is updated.

In this approach, we can guarantee the the |AppendToTrack| will be called after the VideoFrameContainer is added into SourceMediaStream.

@JW, please correct me if anything wrong, thanks.
@roc, what do you think?


> > Above is current status of the WIP patches.
> > Some questions would like to raise.
> > 1. Should I call |BroadcastToVideoFrameSink| in |AppendToTrack| in stead of
> > |ExtractPendingInput|? The reason I move to |ExtractPendingInput| is the
> > |AppendToTrack| will be called before the VideoFrameSink set of
> > SourceMediaStream is updated. So the MSG need to copy the VideoSegments to
> > UpdateTracks and calls |BroadcastToVideoFrameSink| in |ExtractPendingInput|
> > in next MSG tick.
> 
> It would be very good to avoid waiting for an MSG iteration to broadcast
> video frames to the sinks. If we get rid of that, we reduce video latency
> for WebRTC because the latest video frame for a call can be sent to the
> compositor as soon as it's decoded. So it would be good to call
> BroadcastToVideoFrameSink from the MSG only when a new sink is added to the
> set.
>

Comment 20

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

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #19)
> Some updates. I changed the WIP to |SetCurrentFrames| in |AppendToTrack|. It
> basically worked for most of case(VideoFrameContainer,ImageCaptrue,
> MediaPipeline and MediaRecorder) with some controllable minor bugs.
> Now I am trying to fix the case of |AppendToTrack| will be called in
> |DecodedStream::SendVideo| before the VideoFrameContainer added to
> SourceMediaStream. After talk with JW, the solution is below:
> 1. Add |NotifyStreamOrderUpdated| in MediaStreamListener. This function will
> be called after |MediaStreamGraphImpl::UpdateStreamOrder| is done.
> 2. In |DecodedStream::CreateData|, the R will be created in
> |DecodedStreamGraphListener::NotifyStreamOrderUpdated|.

What's "R"?

> 3. In |DecodedStream::SendData|, skip until the StreamOrder is updated.

I'm not sure what you mean here.

What I expect to happen is that DecodedStream stores a set of VideoFrameSinks (maybe in the SourceMediaStream). SendData should always update those sinks with a new set of video frames; preferably *all* the frames that are in mVideoQueue.
NotifyStreamOrderUpdated would update the sink set with a new set of sinks, and at that time for any added sinks we should update their video frames to match mVideoQueue.

Comment 21

[JW Wang [:jwwang] [:jw_wang]]

It would be clearer if we have a WIP to reason about.

So what roc means is instead of calling SourceMediaStream::AppendToTrack() in DecodedStream::SendVideo(), we will call something like SourceMediaStream::AppendToVideoFrameSinks() which will update the sinks owned by the SourceMediaStream with a new set of video frames, right?

Comment 22

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - Rename StreamBuffer to StreamTracks. r?roc

Rename StreamBuffer to StreamTracks. Because we still need a place to keep the track information in every MediaStream, even the StreamBuffer::Track::mSegment is empty.

Review commit: https://reviewboard.mozilla.org/r/31377/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31377/

Comment 23

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - Add ImageSizeChangedListener to notify image size changed. r?jesup,r?pehrsons

HTMLMediaElement uses ImageSizeChangedListener to receive the image size changed event from VideoFrameContainer.

Review commit: https://reviewboard.mozilla.org/r/31379/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31379/

Comment 24

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - Add |MediaStreamListener::NotifyQueuedAudioData| and separate non TRACK_CREATE and TRACK_END part to NotifyQueuedAudioData. r?jesup,r?pehrsons

One of the goal of this bug is to simplified NotifyQueuedTrackChanges to notify TRACK_CREATE and TRACK_END event only. This changeset is trying to separate the audio data part first.

Review commit: https://reviewboard.mozilla.org/r/31381/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31381/

Comment 25

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Bug 1201363 - Adding base class MediaStreamVideoSink and changing VideoFrameContainer to be inherited from MediaStreamVideoSink.

MediaStreamVideoSink is the base class of VideoFrameContainer, CaptureTask(ImageCapture), MediaStreamVideoRecorderSink(MediaRecoreder) and PipelineVideoSink(WebRTC-MediaPipelineTransmit). In this patch, I change VideoFrameContainer only. The rest of cases will be changed in latter patches of this bug.

Review commit: https://reviewboard.mozilla.org/r/31383/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31383/

Comment 26

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - Replace VideoFrameContainer with MediaStreamVideoSink in MSG. r?roc

Replace the pointer of VideoFrameContainer with the pointer of MediaStreamVideoSink.

Review commit: https://reviewboard.mozilla.org/r/31385/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31385/

Comment 27

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - Update set of VideoFrameSink in SourceMediaStream when the stream order is dirty. r?roc

The MediaStreamVideoSink could be added in any thread and any MediaStream. The MSG will traverse from each SourceMediaStream to each own children and collect the mVideoOutput to the set of MediaStreamVideoSink in |UpdateStreamOrder|. The MSG also calculate the new added and removed sink in |UpdateMediaStreamVideoSinkSet|.

Review commit: https://reviewboard.mozilla.org/r/31387/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31387/

Comment 28

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - Call MediaStreamVideoSink::setCurrentFrames in SourceMediaStream::AppendToTrack. r?roc

In this patch, we first deal with the case of MediaElement. Now we replace |PlayVideo| with |VideoFrameContainer::SetCurrentFrames| in |SourceMediaStream::AppendToTrack|. The MSG use TimeStamp::Now() for the TimeStamp of each video frame in most of case except MediaElement case. Becasue the MediaElement has its own VideoQueue, we need to calucalte the correct Timestamp based on the StartTimeStamp of this MediaStream and the elpased time of the video frame in DecodedStream.

Review commit: https://reviewboard.mozilla.org/r/31389/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31389/

Comment 29

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - MediaStreamVideoSink for ImageCapture case. r?roc

Make CaptureTask to inherite from MediaStreamVideoSink. The main change is to move the logic of |NotifyQueuedTrackChanges| to |SetCurrentFrames|.

Review commit: https://reviewboard.mozilla.org/r/31391/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31391/

Comment 30

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - MediaStreamVideoSink for MediaRecorder case. r?roc

Add MediaStreamVideoRecorderSink into MediaEncorder. In this patch, I still keep use duration to pass to TrackEncoders. Don't want to make this bug too big and out of control. We can file a new bug to change TrackEncoders use TimeStamp only.

Review commit: https://reviewboard.mozilla.org/r/31393/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31393/

Comment 31

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - MediaStreamVideoSink for MediaPipelineTransmit case. r?roc

Replace |MediaPipelineTransmit::PipelineListener::NotifyQueuedTrackChanges| with |MediaPipelineTransmit::PipelineVideoSink::SetCurrentFrames|. We only need to deal with the video case since audio will be routed to |NotifyQueuedAudioData|.

Review commit: https://reviewboard.mozilla.org/r/31395/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31395/

Comment 32

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - Do not copy video segment to StreamTracks in TrackUnionStream. r?roc

Now everything is ready. We can make NotifyQueuedTrackChanges only triggered by TRACK_EVENT_CREATED and TRACK_EVENT_ENDED without breaking anything. Also we make TrackUnionStream no longer copying data in video case.

Review commit: https://reviewboard.mozilla.org/r/31397/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31397/

Comment 33

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

The patchs structure is designed to keep each changeset passes all mochitests in dom/media/test and dom/media/tests/mochitest. That will be easier to find a defect between changesets. :)

Comment 34

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66656f0845aa

Comment 35

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://reviewboard.mozilla.org/r/31389/#review28259

::: dom/media/MediaStreamGraph.cpp:285
(Diff revision 1)
> +    }

Forgot to remove above lines. Will removed in next patch.

::: dom/media/MediaStreamGraph.cpp:285
(Diff revision 1)
> +    }

Forgot to remove above lines. Will removed in next patch.

Comment 36

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5837d36db9cf

Comment 37

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b4b0e24253a

Comment 38

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99acb8e39fd1

Comment 39

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5e0fb557452

Comment 40

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9c3bd9f32a6

Comment 41

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=759e78cc08e0

Comment 42

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f3419720ad9

Comment 43

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709281 [details]
MozReview Request: Bug 1201363 - Rename StreamBuffer to StreamTracks. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31377/diff/1-2/

Comment 44

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709282 [details]
MozReview Request: Bug 1201363 - Add ImageSizeChangedListener to notify image size changed. r?jesup,r?pehrsons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31379/diff/1-2/

Comment 45

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709283 [details]
MozReview Request: Bug 1201363 - Add |MediaStreamListener::NotifyQueuedAudioData| and separate non TRACK_CREATE and TRACK_END part to NotifyQueuedAudioData. r?jesup,r?pehrsons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31381/diff/1-2/

Comment 46

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709284 [details]
Bug 1201363 - Adding base class MediaStreamVideoSink and changing VideoFrameContainer to be inherited from MediaStreamVideoSink.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31383/diff/1-2/

Comment 47

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709285 [details]
MozReview Request: Bug 1201363 - Replace VideoFrameContainer with MediaStreamVideoSink in MSG. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31385/diff/1-2/

Comment 48

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709286 [details]
MozReview Request: Bug 1201363 - Update set of VideoFrameSink in SourceMediaStream when the stream order is dirty. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31387/diff/1-2/

Comment 49

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709287 [details]
MozReview Request: Bug 1201363 - Call MediaStreamVideoSink::setCurrentFrames in SourceMediaStream::AppendToTrack. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31389/diff/1-2/

Comment 50

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709288 [details]
MozReview Request: Bug 1201363 - MediaStreamVideoSink for ImageCapture case. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31391/diff/1-2/

Comment 51

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709289 [details]
MozReview Request: Bug 1201363 - MediaStreamVideoSink for MediaRecorder case. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31393/diff/1-2/

Comment 52

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709290 [details]
MozReview Request: Bug 1201363 - MediaStreamVideoSink for MediaPipelineTransmit case. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31395/diff/1-2/

Comment 53

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709291 [details]
MozReview Request: Bug 1201363 - Do not copy video segment to StreamTracks in TrackUnionStream. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31397/diff/1-2/

Comment 54

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://reviewboard.mozilla.org/r/31389/#review28623

::: dom/media/MediaStreamGraph.cpp:285
(Diff revision 1)
> +    }

Fixed in Revision 2.

Comment 55

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Update the patches to fix some issues which found in try server.

Comment 56

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

Comment on attachment 8709281 [details]
MozReview Request: Bug 1201363 - Rename StreamBuffer to StreamTracks. r?roc

https://reviewboard.mozilla.org/r/31377/#review28985

Use "hg rename" to preserve the history of StreamBuffer -> StreamTrack.h/cpp

Comment 57

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

Comment on attachment 8709282 [details]
MozReview Request: Bug 1201363 - Add ImageSizeChangedListener to notify image size changed. r?jesup,r?pehrsons

https://reviewboard.mozilla.org/r/31379/#review28987

Comment 58

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

Comment on attachment 8709283 [details]
MozReview Request: Bug 1201363 - Add |MediaStreamListener::NotifyQueuedAudioData| and separate non TRACK_CREATE and TRACK_END part to NotifyQueuedAudioData. r?jesup,r?pehrsons

https://reviewboard.mozilla.org/r/31381/#review29003

Comment 59

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

Comment on attachment 8709284 [details]
Bug 1201363 - Adding base class MediaStreamVideoSink and changing VideoFrameContainer to be inherited from MediaStreamVideoSink.

https://reviewboard.mozilla.org/r/31383/#review29011

Good!

Comment 60

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

Comment on attachment 8709285 [details]
MozReview Request: Bug 1201363 - Replace VideoFrameContainer with MediaStreamVideoSink in MSG. r?roc

https://reviewboard.mozilla.org/r/31385/#review29015

Comment 61

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

https://reviewboard.mozilla.org/r/31383/#review29017

::: dom/media/MediaStreamVideoSink.h:27
(Diff revision 2)
> +  virtual void SetCurrentFrames(const VideoSegment& aSegment) = 0;

At some point we need to change VideoSegment so that it doesn't require frame durations. I.e. we should just have a list of video frames with their timestamps. But you've done the right thing by not making that change yet.

Comment 62

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

Comment on attachment 8709286 [details]
MozReview Request: Bug 1201363 - Update set of VideoFrameSink in SourceMediaStream when the stream order is dirty. r?roc

https://reviewboard.mozilla.org/r/31387/#review29019

::: dom/media/MediaStreamGraph.cpp:30
(Diff revision 2)
> +#include <stack>

use nsTArray instead

::: dom/media/MediaStreamGraph.cpp:347
(Diff revision 2)
> +MediaStreamGraphImpl::UpdateMediaStreamVideoSinkSet(SourceMediaStream* aStream)

In theory, this approach would lead to O(N^2) behavior in the MSG, where there are N sources connected to N sinks via a single intermediate node. I.e. with 2N + 1 nodes and 2N edges, we'd have time quadratic in the size of the graph. This could be avoided by computing video sink sets for each node in the graph all at once and it might not be more code.

Think about that and tell me what you think.

Comment 63

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

https://reviewboard.mozilla.org/r/31387/#review29019

> In theory, this approach would lead to O(N^2) behavior in the MSG, where there are N sources connected to N sinks via a single intermediate node. I.e. with 2N + 1 nodes and 2N edges, we'd have time quadratic in the size of the graph. This could be avoided by computing video sink sets for each node in the graph all at once and it might not be more code.
> 
> Think about that and tell me what you think.

I guess that doesn't really make sense because a node isn't going to have multiple video inputs except in very rare cases. So never mind.

Comment 64

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

Comment on attachment 8709287 [details]
MozReview Request: Bug 1201363 - Call MediaStreamVideoSink::setCurrentFrames in SourceMediaStream::AppendToTrack. r?roc

https://reviewboard.mozilla.org/r/31389/#review29027

::: dom/media/MediaStreamGraph.h:706
(Diff revision 2)
> +    mStreamTracksStartTimeStamp(TimeStamp::Now()),

Actually I think we probably should fix VideoSegment now (or really, introduce a new type for MediaStreamVideoSink::SetFrames), one that includes a TimeStamp for each frame but no duration. Then the clients will all be responsible for setting an actual TimeStamp on each frame. I think that's better than having this default behavior. I don't think the creation time of the SourceMediaStream object should affect frame timing.

Comment 65

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

Let's deal with those comments before I do the rest of the reviews.

Comment 66

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Sorry for the late reply. I found that I didn't deal with multiple MediaStreamTrack well in previous patches. After bug 1103188 landed, the DOMMediaStream can contain multiple VideoStreamTracks. It makes the logic of attach MediaStreamVideoSink to the SourceMediaStream more complicated. There should not be happened that two SourceMediaStreams try to call SetCurrentFrames to the same MediaStreamVideoSink at the same time. So the MediaStreamVideoSink should bind with one MediaStreamTrack. But the MediaStreamTrack in addTrack case means the new added MediaStreamTrack owned by another DOMMediaStream. This made the implementation more complicated. The question will become that which TrackID should the MediaStreamTrack bind with? After some experiments, the solution I found for the implementation is the MediaStreamVideoSink should be bound with the track id of playback stream in the DOMMediaStream.
But how to do that? Right now, the MediaStreamTrack only own a track id for owned stream. One of the solution is to keep an array of playback id in MediaStreamTrack. But that seems not a good idea. My solution is add the playback id into TrackPort. The TrackPort is used to keep track the relationship of MediaInputPort and MediaStreamTrack of the playback stream in DOMMediaStream. In another word, if we add one video stream track into a two tracks(audio and video) DOMMediaStream, the mTracks(array of TrackPort) will show 3 TrackPorts. The mOwnedTracks will contain 2 TrackPorts. Another issue is threading problem. The playback track id is assigned by MSG(TrackUnionStream::AddTrack) and the TrackPort is in main thread. I use NotifiyQueuedTrackChanged to update the playback track id in TrackPort. And the implementation works fine now. I will fix remain minor issues.

Comment 67

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

I found one issue about multiple video stream track in the same MediaStream. I would like to hear your advices.

Example:
There are two VideoElements, video1 and video2. The src of video2 is from video1.mozCaptureStream called mdeiaStream1. The mdeiaStream1 in video2 contains only 1 VideoStreamTrack in this time. The developer can use gUM to get a new MediaStream and add it to the mdeiaStream1. Now the mdeiaStream1 contains two VideoStreamTracks. So the problem will be which VideoStreamTrack should be displayed in the video2?
According to the MediaElement spec, "A media resource can have multiple audio and video tracks. For the purposes of a media element, the video data of the media resource is only that of the currently selected track (if any) given by the element's videoTracks attribute, and the audio data of the media resource is the result of mixing all the currently enabled tracks (if any) given by the element's audioTracks attribute."

But Gecko doesn't support selected in VideoTracks now.

Current behavior is show the video1 first. Once the new VideoStreamTrack is added, show the new added VideoStreamTrack(in this case => gUM).
 
Per talk with the roc and pehrsons in IRC, we think the behavior might be changed to show the video1 even the new VideoStreamTrack is added. But when track of video1 is ended, show the new added VideoStreamTrack.

But when I tried to implement this, I faced an issue. The issue will be happened in a case(2 video track in one MediaStream) that the end user try to click the seek bar. Current Gecko implementation will destroy the original SourceMediaStream and create a new SourceMediaStream and then also remove the original MediaStreamTrack and add a new MediaStreamTrack. The removal MediaStreamTrack behavior will cause the implementation of showing the new added MediaStreamTrack when the old one is ended difficult. That will cause the situation, when the end user click the seek bar in the video1, if the video2 contains two video tracks, the video2 will show the new added video track and won't show video1 again. I don't know this is a corner case or not.

There are several options.
1. Take the problem as a corner case and show the new added video track when the video1 is ended.
2. Change the seek implementation in MozCaptureStream case to not destroy the old SourceMediaStream.
3. Do 1 and file a new bug for 2 and fix it.
4. Happy to see more advices.

Thanks.

 


 




[1]:https://www.w3.org/TR/html5/embedded-content-0.html#media-elements

Comment 68

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - Rename StreamBuffer to StreamTracks. r?jesup,r?pehrsons

Rename StreamBuffer to StreamTracks. We still need a place to keep the track information in every MediaStream, even the StreamBuffer::Track::mSegment is empty.

Review commit: https://reviewboard.mozilla.org/r/42457/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42457/

Comment 69

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709282 [details]
MozReview Request: Bug 1201363 - Add ImageSizeChangedListener to notify image size changed. r?jesup,r?pehrsons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31379/diff/2-3/

Comment 70

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709283 [details]
MozReview Request: Bug 1201363 - Add |MediaStreamListener::NotifyQueuedAudioData| and separate non TRACK_CREATE and TRACK_END part to NotifyQueuedAudioData. r?jesup,r?pehrsons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31381/diff/2-3/

Comment 71

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709284 [details]
Bug 1201363 - Adding base class MediaStreamVideoSink and changing VideoFrameContainer to be inherited from MediaStreamVideoSink.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31383/diff/2-3/

Comment 72

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Bug 1201363 - Replace VideoFrameContainer with MediaStreamVideoSink in MSG.

Replace the pointer of VideoFrameContainer with the pointer of MediaStreamVideoSink.

Review commit: https://reviewboard.mozilla.org/r/42459/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42459/

Comment 73

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - Let MediaStreamVideoSink bind with particular video track. r?jesup,r?pehrsons

In some cases, MSG need to deal with the same MediaStreamVideoSink will be exposed to different SourceMediaStream. For example, if MediaStream.addTrack() is called, the MSG should take care this case. Or you will see flicker due to multiple SourceMediaStream trying to call |SetCurrentFrames| in the same time.

Review commit: https://reviewboard.mozilla.org/r/42461/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42461/

Comment 74

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - Update set of VideoFrameSink in SourceMediaStream when the stream order is dirty. r?jesup,r?pehrsons

The MediaStreamVideoSink could be added in any thread and any MediaStream. The MSG will traverse from each SourceMediaStream to each own children and collect the mVideoOutput to the set of MediaStreamVideoSink in |UpdateStreamOrder|. The MSG also calculate the new added and removed sink in |UpdateMediaStreamVideoSinkSet|.

Review commit: https://reviewboard.mozilla.org/r/42463/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42463/

Comment 75

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - Call MediaStreamVideoSink::setCurrentFrames in SourceMediaStream::AppendToTrack. r?jesup,r?pehrsons

In this patch, we first deal with the case of MediaElement. Now we replace |PlayVideo| with |VideoFrameContainer::SetCurrentFrames| in |SourceMediaStream::AppendToTrack|. The MSG use TimeStamp::Now() for the TimeStamp of each video frame in most of case except MediaElement case. Becasue the MediaElement has its own VideoQueue, we need to calucalte the correct Timestamp based on the StartTimeStamp of this MediaStream and the elpased time of the video frame in DecodedStream.

Review commit: https://reviewboard.mozilla.org/r/42465/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42465/

Comment 76

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - MediaStreamVideoSink for ImageCapture case. r?jesup,r?pehrsons

Make CaptureTask to inherite from MediaStreamVideoSink. The main change is to move the logic of |NotifyQueuedTrackChanges| to |SetCurrentFrames|.
The original image capture is not modified for support multiple video MediaStreamTracks. The design still used the track id in owned media stream. The should be fixed in the following bug if we still want to support ImageCapture in multiple video tracks case.

Review commit: https://reviewboard.mozilla.org/r/42467/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42467/

Comment 77

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - MediaStreamVideoSink for MediaRecorder case. r?jesup,r?pehrsons

Add MediaStreamVideoRecorderSink into MediaEncorder. In this patch, I still keep use duration to pass to TrackEncoders. Don't want to make this bug too big and out of control. We can file a new bug to change TrackEncoders use TimeStamp only.

Review commit: https://reviewboard.mozilla.org/r/42469/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42469/

Comment 78

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - MediaStreamVideoSink for MediaPipelineTransmit case. r?jesup,r?pehrsons

Replace |MediaPipelineTransmit::PipelineListener::NotifyQueuedTrackChanges| with |MediaPipelineTransmit::PipelineVideoSink::SetCurrentFrames|. We only need to deal with the video case since audio will be routed to |NotifyQueuedAudioData|.

Review commit: https://reviewboard.mozilla.org/r/42471/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42471/

Comment 79

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1201363 - Do not copy video segment to StreamTracks in TrackUnionStream. r?jesup,r?pehrsons

Now everything is ready. We can make NotifyQueuedTrackChanges only triggered by TRACK_EVENT_CREATED and TRACK_EVENT_ENDED without breaking anything. Also we make TrackUnionStream no longer copying data in video case.

Review commit: https://reviewboard.mozilla.org/r/42473/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42473/

Comment 80

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Done!
Change reviewer to jesup and pehrsons.

(In reply to Robert O'Callahan (:roc) (Exited; email my personal email if necessary) from comment #56)
> Comment on attachment 8709281 [details]
> MozReview Request: Bug 1201363 - Rename StreamBuffer to StreamTracks. r?roc
> 
> https://reviewboard.mozilla.org/r/31377/#review28985
> 
> Use "hg rename" to preserve the history of StreamBuffer -> StreamTrack.h/cpp

Comment 81

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Change to nsTArray is done!
Per talk with roc in irc, I will keep the original design. Although MediaStreamGraphImpl keeps mStreams for all streams, but iterate those streams are not enough. Still need O(N^2) time in some cases. 


(In reply to Robert O'Callahan (:roc) (Exited; email my personal email if necessary) from comment #62)
> Comment on attachment 8709286 [details]
> MozReview Request: Bug 1201363 - Update set of VideoFrameSink in
> SourceMediaStream when the stream order is dirty. r?roc
> 
> https://reviewboard.mozilla.org/r/31387/#review29019
> 
> ::: dom/media/MediaStreamGraph.cpp:30
> (Diff revision 2)
> > +#include <stack>
> 
> use nsTArray instead
> 
> ::: dom/media/MediaStreamGraph.cpp:347
> (Diff revision 2)
> > +MediaStreamGraphImpl::UpdateMediaStreamVideoSinkSet(SourceMediaStream* aStream)
> 
> In theory, this approach would lead to O(N^2) behavior in the MSG, where
> there are N sources connected to N sinks via a single intermediate node.
> I.e. with 2N + 1 nodes and 2N edges, we'd have time quadratic in the size of
> the graph. This could be avoided by computing video sink sets for each node
> in the graph all at once and it might not be more code.
> 
> Think about that and tell me what you think.

Comment 82

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

All of suggestions are done. But in last patches, I didn't consider the multiple VideoStreamTracks. I fix it in this time.

(In reply to Robert O'Callahan (:roc) (Exited; email my personal email if necessary) from comment #65)
> Let's deal with those comments before I do the rest of the reviews.

Comment 83

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

The structure of the latest patches are followed the comment #15 but add the logic of binding VideoFrameSink with TrackID part for multiple VideoStreamTracks case.

(In reply to Robert O'Callahan (:roc) (Exited; email my personal email if necessary) from comment #16)
> (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #15)
> > Now I have a WIP changesets which pass all dom/media/test and
> > dom/media/tests/mochitest/. And the logic is below.
> > 
> > 1. A base class called VideoFrameSink which contains |SetCurrentFrame(s)|
> > and ClearFrames. The function names might be discussable since the class
> > will be used more than VideoFrameContainer only. But the functionality
> > should cover send video frames with timestamp to the sub-class of
> > VideoFrameSink and clear the frames.
> 
> Probably should call it MediaStreamVideoSink to clearly distinguish it from
> VideoSink.
> 
> > 2. A ImageSizeChangedListener for HTMLMediaElement::StreamSizeListener. Then
> > we can make NotifyQueuedTrackChanges do less job.
> > 
> > 3. Add NotifyQueuedAudioData into MediaStreamListener and make
> > |MediaStreamListener::NotifyQueuedTrackChanges| only for TRACK_CREATE and
> > TRACK_END
> > 
> > 4. Rename StreamBuffer to StreamTracks for notify owned media stream in
> > DOMMediaStream for the DOMMediaStreamTrack creation.
> > 
> > 5. Add mVideoSinks(an array of VideoFrameSink) into SourceMediaStream.Every
> > MediaStream still keep the VideoOutput as original, only change to an array
> > of VideoFrameSink. MSG will walk through the graph(DFS) in |UpdateGraph|
> > when the stream order is dirty and collect all VideoFrameSinks of the
> > children of the SourceMediaStream. 
> > 
> > 6. In original design, the |PlayVideo| will be merged to AppendToTrack. I
> > change a little bit. I Add a function called |BroadcastToVideoFrameSink| to
> > replace |PlayVideo|. It will be called in |ExtractPendingInput|. And the
> > TimeStamp is calculated as mTracksStartTime plus accumulated durations and
> > the value is the frame time in graph time. So I can use this value to
> > transfer to the target time stamp based on current time stamp. Thanks to bug
> > 1189506(removing blocking from MSG), it is much simpler than my imagination.
> >
> > Above is current status of the WIP patches.
> > Some questions would like to raise.
> > 1. Should I call |BroadcastToVideoFrameSink| in |AppendToTrack| in stead of
> > |ExtractPendingInput|? The reason I move to |ExtractPendingInput| is the
> > |AppendToTrack| will be called before the VideoFrameSink set of
> > SourceMediaStream is updated. So the MSG need to copy the VideoSegments to
> > UpdateTracks and calls |BroadcastToVideoFrameSink| in |ExtractPendingInput|
> > in next MSG tick.
> 
> It would be very good to avoid waiting for an MSG iteration to broadcast
> video frames to the sinks. If we get rid of that, we reduce video latency
> for WebRTC because the latest video frame for a call can be sent to the
> compositor as soon as it's decoded. So it would be good to call
> BroadcastToVideoFrameSink from the MSG only when a new sink is added to the
> set.
> 
> > 2. I will fix the cases of MediaEncoder, CaptureTask and WebRTC
> > MediaPipelineTransimt::PipelineListener non-direct case. But I think the
> > removing direct listener could be done in another follow-up bug.
> 
> I think we'll still need the direct listener for WebRTC audio.
> 
> This basically sounds very good. Thanks!

Comment 84

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Hi, jesup, pehrsons
You guys might still want to take a look with those patches which r+ by roc already.
Thanks!

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #69)
> Comment on attachment 8709282 [details]
> MozReview Request: Bug 1201363 - Add ImageSizeChangedListener to notify
> image size changed. r?jesup,r?pehrsons
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/31379/diff/2-3/

Comment 85

[Andreas Pehrson [:pehrsons]]

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #67)
> There are several options.
> 1. Take the problem as a corner case and show the new added video track when
> the video1 is ended.
> 2. Change the seek implementation in MozCaptureStream case to not destroy
> the old SourceMediaStream.
> 3. Do 1 and file a new bug for 2 and fix it.
> 4. Happy to see more advices.

We're not spec compliant on HTMLMediaElement.captureStream at the moment. An important change that we have to do to become spec compliant is to not destroy captured tracks when seeking. There's bug 1172394 for that.

However, given that HTMLMediaElement.mozCaptureStream is prefixed, multiple same-type tracks in one stream is rather new functionality, and there's a simple workaround available (only do one track of each type in the stream), I'm fine with treating it as a corner case per your (1.).

I'll let jesup have final say.

Comment 86

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8709282 [details]
MozReview Request: Bug 1201363 - Add ImageSizeChangedListener to notify image size changed. r?jesup,r?pehrsons

https://reviewboard.mozilla.org/r/31379/#review39425

::: dom/html/HTMLMediaElement.cpp:3056
(Diff revision 3)
>   * This listener observes the first video frame to arrive with a non-empty size,
>   * and calls HTMLMediaElement::ReceivedMediaStreamInitialSize() with that size.
>   */
> -class HTMLMediaElement::StreamSizeListener : public MediaStreamListener {
> +class HTMLMediaElement::StreamSizeListener : public ImageSizeChangedListener {
>  public:
> -  explicit StreamSizeListener(HTMLMediaElement* aElement) :
> +  explicit StreamSizeListener(HTMLMediaElement* aElement, MediaStreamGraph* aGraph) :

No need for `explicit` with two args.

::: dom/media/VideoFrameContainer.h:32
(Diff revision 3)
> +  // Protected destructor, to discourage deletion outside of Release():
> +  virtual ~ImageSizeChangedListener() {}
> +
> +public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ImageSizeChangedListener)
> +  ImageSizeChangedListener(MediaStreamGraph* aGraph) : mGraph(aGraph) {}

`explicit` with one arg.

::: dom/media/VideoFrameContainer.h:35
(Diff revision 3)
> +public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ImageSizeChangedListener)
> +  ImageSizeChangedListener(MediaStreamGraph* aGraph) : mGraph(aGraph) {}
> +  virtual void NotifyImageSizeChanged(MediaStreamGraph* aGraph,
> +                                      const gfx::IntSize& aIntrinsicSize) = 0;
> +  MediaStreamGraph* mGraph;

How do you know `mGraph` is kept alive?

::: dom/media/VideoFrameContainer.cpp:84
(Diff revision 3)
>    }
>    gfx::IntSize newFrameSize = mImageContainer->GetCurrentSize();
>    if (oldFrameSize != newFrameSize) {
>      mImageSizeChanged = true;
> +    for (ImageSizeChangedListener* l : mListeners) {
> +      l->NotifyImageSizeChanged(l->mGraph, newFrameSize);

`ImageSizeChangedListener::NotifyImageSizeChanged` has an arg `aIntrinsicSize` but here you're checking the non-intrinsic size?

Comment 87

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8734723 [details]
MozReview Request: Bug 1201363 - Rename StreamBuffer to StreamTracks. r?jesup,r?pehrsons

https://reviewboard.mozilla.org/r/42457/#review39427

Comment 88

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8709283 [details]
MozReview Request: Bug 1201363 - Add |MediaStreamListener::NotifyQueuedAudioData| and separate non TRACK_CREATE and TRACK_END part to NotifyQueuedAudioData. r?jesup,r?pehrsons

https://reviewboard.mozilla.org/r/31381/#review39429

::: dom/media/MediaStreamGraph.cpp:174
(Diff revision 3)
> +      if ((data->mCommands & SourceMediaStream::TRACK_CREATE) ||
> +          (data->mCommands & SourceMediaStream::TRACK_END)) {

`if (data->mCommands)` should be enough.

::: dom/media/MediaStreamGraph.cpp:180
(Diff revision 3)
> +      } else if (data->mData->GetType() == MediaSegment::AUDIO) {
> +        for (MediaStreamListener* l : aStream->mListeners) {
> +            l->NotifyQueuedAudioData(this, data->mID,
> +                                     offset, *(static_cast<AudioSegment*>(data->mData.get())));
> +        }

Don't you have to call `NotifyQueuedAudioData` also when there's a command?

::: dom/media/encoder/MediaEncoder.h:92
(Diff revision 3)
>    /**
> +   * Notifed by the control loop of MediaStreamGraph; aQueueMedia is the raw
> +   * track data in form of AudioSegment.
> +   */

"in the form of an AudioSegment"

::: dom/media/encoder/MediaEncoder.h:96
(Diff revision 3)
> +  virtual void NotifyQueuedAudioData(MediaStreamGraph* aGraph, TrackID aID,
> +                                     StreamTime aTrackOffset,
> +                                     const AudioSegment& aQueuedMedia,
> +                                     MediaStream* aInputStream = nullptr,
> +                                     TrackID aInputTrackID = TRACK_INVALID) override;

No `virtual` when you have `override`.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:488
(Diff revision 3)
> +    virtual void NotifyQueuedAudioData(MediaStreamGraph* graph, TrackID tid,
> +                                       StreamTime offset,
> +                                       const AudioSegment& queued_media,
> +                                       MediaStream* input_stream,
> +                                       TrackID input_tid) override;

Remove `virtual`.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:643
(Diff revision 3)
> -
> +    virtual void NotifyQueuedAudioData(mozilla::MediaStreamGraph* aGraph, mozilla::TrackID aID,
> +                                       mozilla::StreamTime aTrackOffset,
> +                                       const mozilla::AudioSegment& aQueuedMedia,
> +                                       MediaStream* aInputStream,
> +                                       mozilla::TrackID aInputTrackID) override {}

Remove `virtual`.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:758
(Diff revision 3)
> +    virtual void NotifyQueuedAudioData(mozilla::MediaStreamGraph* aGraph, mozilla::TrackID aID,
> +                                       mozilla::StreamTime aTrackOffset,
> +                                       const mozilla::AudioSegment& aQueuedMedia,
> +                                       MediaStream* aInputStream,
> +                                       mozilla::TrackID aInputTrackID) override {}

Remove `virtual`.

Comment 89

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8709284 [details]
Bug 1201363 - Adding base class MediaStreamVideoSink and changing VideoFrameContainer to be inherited from MediaStreamVideoSink.

https://reviewboard.mozilla.org/r/31383/#review39441

::: dom/media/MediaStreamVideoSink.h:30
(Diff revision 3)
> +
> +  // Call on any thread
> +  virtual void SetCurrentFrames(const VideoSegment& aSegment) = 0;
> +  virtual void ClearFrames() = 0;
> +
> +  virtual VideoFrameContainer* AsVideoFrameContainer() { return nullptr; }

Hmm, is this really needed? We don't want one for each impl.

Perhaps the user of VideoFrameContainer that could need this could just use a VideoFrameContainer directly instead?

::: dom/media/VideoFrameContainer.h:69
(Diff revision 3)
> +  virtual void SetCurrentFrames(const VideoSegment& aSegment) override;
> +  virtual void ClearFrames() override;
>    B2G_ACL_EXPORT void SetCurrentFrame(const gfx::IntSize& aIntrinsicSize, Image* aImage,
>                                        const TimeStamp& aTargetTime);
>    void SetCurrentFrames(const gfx::IntSize& aIntrinsicSize,
>                          const nsTArray<ImageContainer::NonOwningImage>& aImages);
>    void ClearCurrentFrame(const gfx::IntSize& aIntrinsicSize)
>    {
>      SetCurrentFrames(aIntrinsicSize, nsTArray<ImageContainer::NonOwningImage>());
>    }
> +  virtual VideoFrameContainer* AsVideoFrameContainer() override { return this; }

No `virtual` here.

::: dom/media/VideoSegment.h:148
(Diff revision 3)
> +  virtual bool IsEmpty() const
> +  {
> +    return mChunks.IsEmpty();
> +  }

Why virtual?

Comment 90

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/31383/#review39441

> Hmm, is this really needed? We don't want one for each impl.
> 
> Perhaps the user of VideoFrameContainer that could need this could just use a VideoFrameContainer directly instead?

Looking at a future commit I understand better. I think the API would be cleaner if you have MediaStreamVideoSink::Invalidate which defaults to {} but is implemented by VideoFrameContainer.

Comment 91

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8734724 [details]
Bug 1201363 - Replace VideoFrameContainer with MediaStreamVideoSink in MSG.

https://reviewboard.mozilla.org/r/42459/#review39499

::: dom/media/MediaStreamGraph.h:20
(Diff revision 1)
>  #include "AudioSegment.h"
>  #include "AudioStream.h"
>  #include "nsTArray.h"
>  #include "nsIRunnable.h"
>  #include "StreamTracks.h"
> -#include "VideoFrameContainer.h"
> +#include "MediaStreamVideoSink.h"

It'd be great if you can move this to the cpp and forward declare here.

Comment 92

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8734725 [details]
MozReview Request: Bug 1201363 - Let MediaStreamVideoSink bind with particular video track. r?jesup,r?pehrsons

https://reviewboard.mozilla.org/r/42461/#review39501

With this approach there will be a gap in video data sent to a HTMLMediaElement when a track ends and it switches to a new one (on main thread). To get rid of this gap you have to move logic to MediaStream in MediaStreamGraph. It has to know the order of video tracks so it can pick the first one (per the spec) whenever the selected one fails. If StreamTracks keeps the same order as DOMMediaStream.getTracks() that'd be fine, but I'm not sure that's always the case.

I commented in my review on wrapping stuff in MediaStreamTrack. That'd be a cleaner way of your current approach, but there are clear benefits (getting rid of the gap, per above) to putting the logic in MSG instead. I think it's worth doing it the hard way.

::: dom/html/HTMLMediaElement.h:752
(Diff revision 1)
>  
> +  void AddVideoOutput();
> +

This needs documentation.

::: dom/html/HTMLMediaElement.h:1531
(Diff revision 1)
> +  // The selected video track id of playback stream in the mSrcStream.
> +  TrackID mSelectedVideoTrackID;

I think it'd be simpler if this is a pointer to a VideoStreamTrack.

::: dom/html/HTMLMediaElement.h:1534
(Diff revision 1)
> +  // Defer the AddVideoOutput until the track is added into StreamTracks.
> +  bool bDeferAddVideoOutput;

You shouldn't need this. Add it to the MediaStreamTrack (main thread) instead and either defer there or in the MSG itself.

If you rebase on top of bug 1208371 you'll see that I'm already doing that for some listener.

::: dom/html/HTMLMediaElement.cpp:3423
(Diff revision 1)
> +  DOMMediaStream::TrackPort* trackPort = mSrcStream->FindPlaybackTrackPort(*aTrack);
> +  TrackID playbackID = trackPort->GetPlaybackTrackID();
> +  if (playbackID == mSelectedVideoTrackID) {
> +    MediaStream* stream = GetSrcMediaStream();
> +    if (stream) {
> +      stream->RemoveVideoOutput(GetVideoFrameContainer(), mSelectedVideoTrackID);
> +    }

I think a comment to what is happening here (and below) is necessary.

::: dom/media/DOMMediaStream.h:277
(Diff revision 1)
> +    TrackID GetPlaybackTrackID() const { return mPlaybackTrackID; }
> +
> +    void SetPlaybackTrackID(TrackID aTrackID) { mPlaybackTrackID = aTrackID;}
> +
>    private:
>      RefPtr<MediaInputPort> mInputPort;
>      RefPtr<MediaStreamTrack> mTrack;
>  
> +    TrackID mPlaybackTrackID;
> +

What are these for?

::: dom/media/DOMMediaStream.h:555
(Diff revision 1)
>  
>    // Dispatches NotifyTrackAdded() to all registered track listeners.
>    void NotifyTrackAdded(const RefPtr<MediaStreamTrack>& aTrack);
>  
>    // Dispatches NotifyTrackRemoved() to all registered track listeners.
> -  void NotifyTrackRemoved(const RefPtr<MediaStreamTrack>& aTrack);
> +  void NotifyTrackRemoved(const RefPtr<MediaStreamTrack>& aTrack, TrackID aID);

I've been trying to remove TrackID from APIs where we have access to MediaStreamTrack to not expose internal structures. This is why you need to create the APIs you need on MediaStreamTrack itself and use TrackID there.

::: dom/media/MediaStreamGraph.h:568
(Diff revision 1)
>    {
>      NS_ASSERTION(mSuspendedCount > 0, "Suspend count underrun");
>      --mSuspendedCount;
>    }
>  
> +  typedef Pair<RefPtr<MediaStreamVideoSink>, TrackID> VideoOutputElement;

Bug 1208371 comes with a struct that lets you pair a TrackID to any listener T. Use that so you can explicitly call out mListener and mTrackID.

Comment 93

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8734726 [details]
MozReview Request: Bug 1201363 - Update set of VideoFrameSink in SourceMediaStream when the stream order is dirty. r?jesup,r?pehrsons

https://reviewboard.mozilla.org/r/42463/#review39767

This is basically what I have done in bug 1208371 for direct track listeners, but I only traverse the graph when a listener is removed or added. I personally think you'll be better off using the same mechanism, unless we can say for certain that there will be many updates happening during single iterations, for optimization reasons. It'll also reduce our number of code paths.

Comment 94

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8734727 [details]
MozReview Request: Bug 1201363 - Call MediaStreamVideoSink::setCurrentFrames in SourceMediaStream::AppendToTrack. r?jesup,r?pehrsons

https://reviewboard.mozilla.org/r/42465/#review39803

::: dom/media/MediaStreamGraph.h:924
(Diff revision 1)
> +  // This time stamp will be updated in adding and blocked SourceMediaStream,
> +  // |AddStreamGraphThread| and |AdvanceTimeVaryingValuesToCurrentTime| in
> +  // particularly.
> +  TimeStamp mStreamTracksStartTimeStamp;

I don't understand why this is necessary.

Is it because DecodedStream needs to know how much the stream has been blocked but can't access that because it's not on the MSG thread?

Comment 95

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8734728 [details]
MozReview Request: Bug 1201363 - MediaStreamVideoSink for ImageCapture case. r?jesup,r?pehrsons

https://reviewboard.mozilla.org/r/42467/#review39805

::: dom/media/imagecapture/CaptureTask.h:11
(Diff revision 1)
>  
>  #ifndef CAPTURETASK_H
>  #define CAPTURETASK_H
>  
>  #include "DOMMediaStream.h"
>  #include "MediaStreamGraph.h"

Seeing that you removed the CaptureTask constructor impl, can you remove the MSG include as well? Would be nice.

::: dom/media/imagecapture/CaptureTask.h:38
(Diff revision 1)
> -                   MediaStreamGraphEvent aEvent) override;
> +  virtual void SetCurrentFrames(const VideoSegment& aSegment) override;
> +  virtual void ClearFrames() override {}

No `virtual` for overrides.

::: dom/media/imagecapture/CaptureTask.h:76
(Diff revision 1)
>    // The ImageCapture associates with this task. This reference count should not
>    // change in this class unless it clears this reference after a blob or error
>    // event to script.
>    RefPtr<dom::ImageCapture> mImageCapture;
>  
> +  RefPtr<MediaStreamEventListener> mEventListener;

This needs documentation.

::: dom/media/imagecapture/CaptureTask.cpp:40
(Diff revision 1)
> +    if (mCaptureTask->mImageGrabbedOrTrackEnd) {
> +      return;
> +    }
> +
> +    if (aTrackEvents == MediaStreamListener::TRACK_EVENT_ENDED) {
> +      mCaptureTask->PostTrackEndEvent();
> +    }
> +    return;

Collapse this to an if without returns.

::: dom/media/imagecapture/CaptureTask.cpp:51
(Diff revision 1)
> +    }
> +    return;
> +  }
> +
> +private:
> +  CaptureTask* mCaptureTask;

How can you be sure the CaptureTask is still alive when you need it? Try to avoid rawptrs.

::: dom/media/imagecapture/CaptureTask.cpp:97
(Diff revision 1)
>  CaptureTask::AttachStream()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    RefPtr<dom::VideoStreamTrack> track = mImageCapture->GetVideoStreamTrack();
>  
>    RefPtr<DOMMediaStream> domStream = track->GetStream();
>    domStream->AddPrincipalChangeObserver(this);
>  
>    RefPtr<MediaStream> stream = domStream->GetPlaybackStream();
> -  stream->AddListener(this);
> +  stream->AddListener(mEventListener.get());
> +  stream->AddVideoOutput(this, mTrackID);
>  }

Since ImageCapture takes a VideoStreamTrack this'll be cleaner when you can pass a VideoOutput through a VideoStreamTrack directly.

Comment 96

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8734729 [details]
MozReview Request: Bug 1201363 - MediaStreamVideoSink for MediaRecorder case. r?jesup,r?pehrsons

https://reviewboard.mozilla.org/r/42469/#review39811

::: dom/media/encoder/TrackEncoder.cpp:187
(Diff revision 1)
> -VideoTrackEncoder::NotifyQueuedTrackChanges(MediaStreamGraph* aGraph,
> +VideoTrackEncoder::Init(const VideoSegment& aSegment)
> -                                            TrackID aID,
> -                                            StreamTime aTrackOffset,
> -                                            uint32_t aTrackEvents,
> -                                            const MediaSegment& aQueuedMedia)
>  {
> -  if (mCanceled) {
> +  if (mInitialized) {
>      return;
>    }

For semantics I think you should put the `mInitialized` check outside `Init()` so that `Init()` always initializes the encoder. Assert that `mInitialized` is false if you want.

::: dom/media/encoder/TrackEncoder.cpp:230
(Diff revision 1)
> +  if (!(aTrackEvents == MediaStreamListener::TRACK_EVENT_CREATED ||
> +       aTrackEvents == MediaStreamListener::TRACK_EVENT_ENDED)) {
> +    return;
>    }

Is this needed now? Assert the assumption that you can only get those events to protect from future changes instead?

::: dom/media/encoder/TrackEncoder.cpp:273
(Diff revision 1)
> +      MOZ_ASSERT(chunk.mTimeStamp >= mLastFrameTimeStamp);
> +      TimeDuration timeDuration = chunk.mTimeStamp - mLastFrameTimeStamp;
> +      duration = SecondsToMediaTime(timeDuration.ToSeconds());
> +    }
>      mRawSegment.AppendFrame(image.forget(),
> -                            chunk.GetDuration(),
> +                            duration,

Why can't we keep using `chunk.GetDuration()`?

Can we abstract away this logic from the sink instance into the base class somehow? It seems to me that each instance will inevitably need it.

Perhaps a VideoSegment is not the appropriate class to pass to a sink.

::: dom/media/gtest/TestVideoTrackEncoder.cpp:182
(Diff revision 1)
> +  explicit TestVP8TrackEncoder(TrackRate aTrackRate = 90000)
> +    : VP8TrackEncoder(aTrackRate) {}

You need to also test the failure cases for track rate, like we did before.

Comment 97

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8734730 [details]
MozReview Request: Bug 1201363 - MediaStreamVideoSink for MediaPipelineTransmit case. r?jesup,r?pehrsons

https://reviewboard.mozilla.org/r/42471/#review39815

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:484
(Diff revision 1)
>      virtual void NotifyQueuedTrackChanges(MediaStreamGraph* graph, TrackID tid,
>                                            StreamTime offset,
>                                            uint32_t events,
>                                            const MediaSegment& queued_media,
>                                            MediaStream* input_stream,
> -                                          TrackID input_tid) override;
> +                                          TrackID input_tid) override {}

This can be removed now.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:646
(Diff revision 1)
> +#if !defined(MOZILLA_EXTERNAL_LINKAGE)
> +  // Find the first video track id
> +  nsTArray<RefPtr<mozilla::dom::VideoStreamTrack>> videoTracks;
> +  if (domstream_) {
> +    domstream_->GetVideoTracks(videoTracks);
> +    if (!videoTracks.IsEmpty()) {
> +      selected_video_track_id_ = videoTracks[0]->GetTrackID();
> +      // Right now, the MediaRecorder hasn't dealt with multiple video track
> +      // issues. So we just bind with the first video track.
> +      stream_->AddVideoOutput(video_sink_, selected_video_track_id_);
> +    }
> +  }
> +#endif

You'll need to rebase this on top of bug 1208371. Even without it we can have a DOMMediaStream with more than one video track.

Comment 98

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8734731 [details]
MozReview Request: Bug 1201363 - Do not copy video segment to StreamTracks in TrackUnionStream. r?jesup,r?pehrsons

https://reviewboard.mozilla.org/r/42473/#review39819

Comment 99

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://reviewboard.mozilla.org/r/42463/#review39767

I think the way you bind the VideoOutput to VideoStreamTrack make sense. But the patch in bug 1208371 used the pair of track id and VideoFrameContainer might not be unique in some cases. For example, in the VideoMonitor/VideoProcessor case, the MediaStreamTrack will expose an API for addVideoMonitor to the MediaStreamTrack. If we clone a MST1 to MST2, we can add the same VideoMonitor to the MST1 and MST2. That will cause the SourceMediaStream keep only one record, because the track id and pointer of VideoMonitor are the same. So I propose to replace the TrackID to the MediaStreamTrack::ID(uuid used in JS). Does that make sense?

Comment 100

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/42465/#review40007

::: dom/media/MediaStreamGraph.cpp:445
(Diff revision 1)
> +    // Re-send missed VideoSegment to new added video sinks. Sometimes the
> +    // |SourceMediaStream::AppendToTrack| will be called before the
> +    // MediaStreamVideoSink actually added to the set of MediaStreamVideoSink in
> +    // SourceMediaStream. So we need to re-send the missed video frames.
> +    for (auto newAdded : newAddedVideoSinks) {
> +      StreamTracks::Track* streamTrack = aStream->mTracks.FindTrack(newAdded.second());
> +      if (streamTrack && streamTrack->GetType() == MediaSegment::VIDEO) {
> +        newAdded.first()->SetCurrentFrames(*(static_cast<VideoSegment*>(streamTrack->GetSegment())));
> +      }
> +
> +    }

If you move to use direct track listeners for your video sinks, you'll have to add this behavior to them as we currently don't push any data as you add a direct listener (stream or track).

This would happen on the MSG thread so we'd have to make sure no consumer does anything blocking - encoding, converting, similar things.

You'd also have to make sure there's no race between the MSG and the producer here. So perhaps call the listener on MSG thread first, and add it to the list of listeners after. Or use the SourceMediaStream Mutex like you do here.

Comment 101

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Per talk with pehrsons today(thanks your time :-) ), there are two major issues according to his review. Others should be controllable.
1. Leverage addDirectTrackListener - I think the way to add listener to MediaStreamTrack is clever. But I am not sure the way to take track id as the index is suitable for VideoProcessor/VideoMonitor or not. I might write some patches to evaluate it.

2. The way to fix the gap between the first track end and second track start (see comment 92) might not be easy. Need more time to figure it out.

Thanks for pehrsons' detailed review.

Comment 102

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Per talk with pehrsons today(thanks your time :-) ), there are two major issues according to his review. Others should be controllable.
1. Leverage addDirectTrackListener - I think the way to add listener to MediaStreamTrack is clever. But I am not sure the way to take track id as the index is suitable for VideoProcessor/VideoMonitor or not. I might write some patches to evaluate it.

2. The way to fix the gap between the first track end and second track start (see comment 92) might not be easy. Need more time to figure it out.

Thanks for pehrsons' detailed review.

Comment 103

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://reviewboard.mozilla.org/r/31379/#review39425

This patch will cause Bug 1240478 again....Because the size event only fired when the VideoFrameContainer is set. Get to find a new way for it.

Comment 104

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8709282 [details]
MozReview Request: Bug 1201363 - Add ImageSizeChangedListener to notify image size changed. r?jesup,r?pehrsons

Clearing reviews on all these patches; per conversation with ctai he's in the process of rebasing on top of pehrsons' landing.

Comment 105

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Hi, jesup, karl,
I would like to hear your input about below problem.

Problem statements:
1. There is an interface called VideoTrackList containing selectedIndex in HTMLMediaElement spec. Basically that is for the support of multiple video tracks in HTMLMediaElement. In general, there is only one video track selected and controlled by the attribute selectedIndex. The end user will only see the selected video track.
2. Gecko implemented partial of support for multiple video tracks and this feature is behind the preference "media.track.enabled". The reason of behind pref is the lacking of support in playback side for multiple tracks. See Bug 744896 for more information.
3. Right now, the MediaStream already supports multiple tracks. But the support of multiple tracks in HTMLMediaElement is not doing well which limits the usages of multiple tracks in MediaStream. For example:
Sceneraio:
Taking the media stream 1(captured from other media element v1) as src of another media element v2. 
Current behaviors:
When adding a new track into media stream 1, the UA will displayed the new added track, not original track. This kind of behavior is not follow the spec. The expected behavior is show the original track until the selected track is changed to the new added track. But roc suggest another behavior. Once the original track is ended, display the new added track. This might be better UX.

4. MediaStream is widely adopted in different spec, like PeerConnection, MediaRecorder, ImageCapture, HTMLMediaElement, VideoProcessor/Monitor. The needs of behaviors are diverse in different specs. And the behavior of multiple tracks is not even well defined in some of specs(ex: MediaRecorder).

5. Due to 4, I choose to implement default logic of selected index inside HTMLMediaElement not in MSG. But that will lead to a no video gap while doing track change. That is what Andreas pointed out. So he suggested to implement the selected index inside MSG.

Here comes the questions.
1. Not every clients of MSG need the selected index logic. Bring it to MSG will add the complexity to MSG. What if other client don't want the selected index? For example, VideoMonitor will support receive video frame from different tracks in the same time.
2. How serious of the no video gap problem? Is that necessary to fix it right now?

So my thought is keeping doing the selected index inside HTMLMediaElement. And let's see how bad of the no video gap and file a bug for it. Then we can fix it until there is the need from other spec for selected index or we think that is serious.

HDYT? Any other suggestions?

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #92)
> Comment on attachment 8734725 [details]
> MozReview Request: Bug 1201363 - Let MediaStreamVideoSink bind with
> particular video track. r?jesup,r?pehrsons
> 
> https://reviewboard.mozilla.org/r/42461/#review39501
> 
> With this approach there will be a gap in video data sent to a
> HTMLMediaElement when a track ends and it switches to a new one (on main
> thread). To get rid of this gap you have to move logic to MediaStream in
> MediaStreamGraph. It has to know the order of video tracks so it can pick
> the first one (per the spec) whenever the selected one fails. If
> StreamTracks keeps the same order as DOMMediaStream.getTracks() that'd be
> fine, but I'm not sure that's always the case.
> 
> I commented in my review on wrapping stuff in MediaStreamTrack. That'd be a
> cleaner way of your current approach, but there are clear benefits (getting
> rid of the gap, per above) to putting the logic in MSG instead. I think it's
> worth doing it the hard way.
> 
> ::: dom/html/HTMLMediaElement.h:752
> (Diff revision 1)
> >  
> > +  void AddVideoOutput();
> > +
> 
> This needs documentation.
> 
> ::: dom/html/HTMLMediaElement.h:1531
> (Diff revision 1)
> > +  // The selected video track id of playback stream in the mSrcStream.
> > +  TrackID mSelectedVideoTrackID;
> 
> I think it'd be simpler if this is a pointer to a VideoStreamTrack.
> 
> ::: dom/html/HTMLMediaElement.h:1534
> (Diff revision 1)
> > +  // Defer the AddVideoOutput until the track is added into StreamTracks.
> > +  bool bDeferAddVideoOutput;
> 
> You shouldn't need this. Add it to the MediaStreamTrack (main thread)
> instead and either defer there or in the MSG itself.
> 
> If you rebase on top of bug 1208371 you'll see that I'm already doing that
> for some listener.
> 
> ::: dom/html/HTMLMediaElement.cpp:3423
> (Diff revision 1)
> > +  DOMMediaStream::TrackPort* trackPort = mSrcStream->FindPlaybackTrackPort(*aTrack);
> > +  TrackID playbackID = trackPort->GetPlaybackTrackID();
> > +  if (playbackID == mSelectedVideoTrackID) {
> > +    MediaStream* stream = GetSrcMediaStream();
> > +    if (stream) {
> > +      stream->RemoveVideoOutput(GetVideoFrameContainer(), mSelectedVideoTrackID);
> > +    }
> 
> I think a comment to what is happening here (and below) is necessary.
> 
> ::: dom/media/DOMMediaStream.h:277
> (Diff revision 1)
> > +    TrackID GetPlaybackTrackID() const { return mPlaybackTrackID; }
> > +
> > +    void SetPlaybackTrackID(TrackID aTrackID) { mPlaybackTrackID = aTrackID;}
> > +
> >    private:
> >      RefPtr<MediaInputPort> mInputPort;
> >      RefPtr<MediaStreamTrack> mTrack;
> >  
> > +    TrackID mPlaybackTrackID;
> > +
> 
> What are these for?
> 
> ::: dom/media/DOMMediaStream.h:555
> (Diff revision 1)
> >  
> >    // Dispatches NotifyTrackAdded() to all registered track listeners.
> >    void NotifyTrackAdded(const RefPtr<MediaStreamTrack>& aTrack);
> >  
> >    // Dispatches NotifyTrackRemoved() to all registered track listeners.
> > -  void NotifyTrackRemoved(const RefPtr<MediaStreamTrack>& aTrack);
> > +  void NotifyTrackRemoved(const RefPtr<MediaStreamTrack>& aTrack, TrackID aID);
> 
> I've been trying to remove TrackID from APIs where we have access to
> MediaStreamTrack to not expose internal structures. This is why you need to
> create the APIs you need on MediaStreamTrack itself and use TrackID there.
> 
> ::: dom/media/MediaStreamGraph.h:568
> (Diff revision 1)
> >    {
> >      NS_ASSERTION(mSuspendedCount > 0, "Suspend count underrun");
> >      --mSuspendedCount;
> >    }
> >  
> > +  typedef Pair<RefPtr<MediaStreamVideoSink>, TrackID> VideoOutputElement;
> 
> Bug 1208371 comes with a struct that lets you pair a TrackID to any listener
> T. Use that so you can explicitly call out mListener and mTrackID.

Comment 106

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #85)
> (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #67)
> > There are several options.
> > 1. Take the problem as a corner case and show the new added video track when
> > the video1 is ended.
> > 2. Change the seek implementation in MozCaptureStream case to not destroy
> > the old SourceMediaStream.
> > 3. Do 1 and file a new bug for 2 and fix it.
> > 4. Happy to see more advices.
> 
> We're not spec compliant on HTMLMediaElement.captureStream at the moment. An
> important change that we have to do to become spec compliant is to not
> destroy captured tracks when seeking. There's bug 1172394 for that.
> 
> However, given that HTMLMediaElement.mozCaptureStream is prefixed, multiple
> same-type tracks in one stream is rather new functionality, and there's a
> simple workaround available (only do one track of each type in the stream),
> I'm fine with treating it as a corner case per your (1.).
> 
> I'll let jesup have final say.

I think #3 - we could probably land this (given the state of existing uses of multiple video tracks in a stream), but I don't like leaving it that way forever.

Comment 107

[Randell Jesup [:jesup] (needinfo me)]

> 5. Due to 4, I choose to implement default logic of selected index inside
> HTMLMediaElement not in MSG. But that will lead to a no video gap while
> doing track change. That is what Andreas pointed out. So he suggested to
> implement the selected index inside MSG.
> 
> Here comes the questions.
> 1. Not every clients of MSG need the selected index logic. Bring it to MSG
> will add the complexity to MSG. What if other client don't want the selected
> index? For example, VideoMonitor will support receive video frame from
> different tracks in the same time.
> 2. How serious of the no video gap problem? Is that necessary to fix it
> right now?
> 
> So my thought is keeping doing the selected index inside HTMLMediaElement.
> And let's see how bad of the no video gap and file a bug for it. Then we can
> fix it until there is the need from other spec for selected index or we
> think that is serious.
> 
> HDYT? Any other suggestions?

How much complexity does moving the selected logic to MSG cause?  What is VideoMonitor?  (Though I can guess)
I imagine you can emulate this behavior for videoMonitor if needed.

Seamless clip-switching seems like it would have uses - but that's a vague comment, I'm not sure if one could handle any usecases that wanted that in a different manner.

Per my comment above, one could start with a simpler method (with gaps), and file a followup (and see how bad it is, and if anyone cares) before doing a more complicated fix.

Comment 108

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8734725 [details]
MozReview Request: Bug 1201363 - Let MediaStreamVideoSink bind with particular video track. r?jesup,r?pehrsons

Finishing clearing r? in prep for ctai's rebased/updated patches

Comment 109

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

For moving the selected logic to MSG, I think we can use Strategy Pattern to implement a class handling different behaviors. And I agree with to implement the simple version first and see how it goes since the multiple video tracks case not spread yet.

VdieoMonitor: see Bug 1108950.

Thanks for the feedback. :)

(In reply to Randell Jesup [:jesup] from comment #107)
> > 5. Due to 4, I choose to implement default logic of selected index inside
> > HTMLMediaElement not in MSG. But that will lead to a no video gap while
> > doing track change. That is what Andreas pointed out. So he suggested to
> > implement the selected index inside MSG.
> > 
> > Here comes the questions.
> > 1. Not every clients of MSG need the selected index logic. Bring it to MSG
> > will add the complexity to MSG. What if other client don't want the selected
> > index? For example, VideoMonitor will support receive video frame from
> > different tracks in the same time.
> > 2. How serious of the no video gap problem? Is that necessary to fix it
> > right now?
> > 
> > So my thought is keeping doing the selected index inside HTMLMediaElement.
> > And let's see how bad of the no video gap and file a bug for it. Then we can
> > fix it until there is the need from other spec for selected index or we
> > think that is serious.
> > 
> > HDYT? Any other suggestions?
> 
> How much complexity does moving the selected logic to MSG cause?  What is
> VideoMonitor?  (Though I can guess)
> I imagine you can emulate this behavior for videoMonitor if needed.
> 
> Seamless clip-switching seems like it would have uses - but that's a vague
> comment, I'm not sure if one could handle any usecases that wanted that in a
> different manner.
> 
> Per my comment above, one could start with a simpler method (with gaps), and
> file a followup (and see how bad it is, and if anyone cares) before doing a
> more complicated fix.

Comment 110

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

For moving the selected logic to MSG, I think we can use Strategy Pattern to implement a class handling different behaviors. And I agree with to implement the simple version first and see how it goes since the multiple video tracks case not spread yet.

VdieoMonitor: see Bug 1108950.

Thanks for the feedback. :)

(In reply to Randell Jesup [:jesup] from comment #107)
> > 5. Due to 4, I choose to implement default logic of selected index inside
> > HTMLMediaElement not in MSG. But that will lead to a no video gap while
> > doing track change. That is what Andreas pointed out. So he suggested to
> > implement the selected index inside MSG.
> > 
> > Here comes the questions.
> > 1. Not every clients of MSG need the selected index logic. Bring it to MSG
> > will add the complexity to MSG. What if other client don't want the selected
> > index? For example, VideoMonitor will support receive video frame from
> > different tracks in the same time.
> > 2. How serious of the no video gap problem? Is that necessary to fix it
> > right now?
> > 
> > So my thought is keeping doing the selected index inside HTMLMediaElement.
> > And let's see how bad of the no video gap and file a bug for it. Then we can
> > fix it until there is the need from other spec for selected index or we
> > think that is serious.
> > 
> > HDYT? Any other suggestions?
> 
> How much complexity does moving the selected logic to MSG cause?  What is
> VideoMonitor?  (Though I can guess)
> I imagine you can emulate this behavior for videoMonitor if needed.
> 
> Seamless clip-switching seems like it would have uses - but that's a vague
> comment, I'm not sure if one could handle any usecases that wanted that in a
> different manner.
> 
> Per my comment above, one could start with a simpler method (with gaps), and
> file a followup (and see how bad it is, and if anyone cares) before doing a
> more complicated fix.

Comment 111

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://reviewboard.mozilla.org/r/42459/#review39499

> It'd be great if you can move this to the cpp and forward declare here.

We can't use forward declartion due to "nsTArray<RefPtr<MediaStreamVideoSink>> mVideoOutputs;" in MediaStream.

Comment 112

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/42459/#review39499

> We can't use forward declartion due to "nsTArray<RefPtr<MediaStreamVideoSink>> mVideoOutputs;" in MediaStream.

A forward declaration is OK for an array of RefPtr. Can't you forward declare in DOMMediaStream.h and include in DOMMediaStream.cpp as well?

Comment 113

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/42459/#review39499

> A forward declaration is OK for an array of RefPtr. Can't you forward declare in DOMMediaStream.h and include in DOMMediaStream.cpp as well?

The MediaSream destructor would need to move to the .cpp file, and maybe the constructor also.
If that is sufficient, it would be nice to avoid bringing in unnecessary includes.

Comment 114

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://reviewboard.mozilla.org/r/42459/#review39499

> The MediaSream destructor would need to move to the .cpp file, and maybe the constructor also.
> If that is sufficient, it would be nice to avoid bringing in unnecessary includes.

hi, karlt,
Thanks. I think that is the missing part. I will figure it out. :)

Comment 115

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://reviewboard.mozilla.org/r/42459/#review39499

> hi, karlt,
> Thanks. I think that is the missing part. I will figure it out. :)

Moving MediaSream destructor is enough! Thanks, karlt. :)

Comment 116

[Randell Jesup [:jesup] (needinfo me)]

Are there new patches to review?

Comment 117

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Bug 1201363 - Let MediaStreamVideoSink bind with particular video track.

Review commit: https://reviewboard.mozilla.org/r/66490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66490/

Comment 118

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Bug 1201363 - Register MediaStreamVideoSink into SourceMediaStream.

Review commit: https://reviewboard.mozilla.org/r/66492/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66492/

Comment 119

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Bug 1201363 - Adding Add/RemoveVideoOutput into VideoStreamTrack.

Review commit: https://reviewboard.mozilla.org/r/66494/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66494/

Comment 120

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Bug 1201363 - Call MediaStreamVideoSink::setCurrentFrames in SourceMediaStream::AppendToTrack.

In this patch, we first deal with the case of MediaElement. Now we replace |PlayVideo| with |VideoFrameContainer::SetCurrentFrames| in |SourceMediaStream::AppendToTrack|. The MSG use TimeStamp::Now() for the TimeStamp of each video frame in most of case except MediaElement case. Becasue the MediaElement has its own VideoQueue, we need to calucalte the correct Timestamp based on the StartTimeStamp of this MediaStream and the elpased time of the video frame in DecodedStream.

Review commit: https://reviewboard.mozilla.org/r/66496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66496/

Comment 121

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Bug 1201363 - MediaStreamVideoSink for ImageCapture case.

Make CaptureTask to inherite from MediaStreamVideoSink. The main change is to move the logic of |NotifyQueuedTrackChanges| to |SetCurrentFrames|.
The original image capture is not modified for support multiple video MediaStreamTracks. The design still used the track id in owned media stream. The should be fixed in the following bug if we still want to support ImageCapture in multiple video tracks case.

Review commit: https://reviewboard.mozilla.org/r/66498/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66498/

Comment 122

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Bug 1201363 - MediaStreamVideoSink for MediaRecorder case.

Add MediaStreamVideoRecorderSink into MediaEncorder. In this patch, I still keep use duration to pass to TrackEncoders. Don't want to make this bug too big and out of control. We can file a new bug to change TrackEncoders use TimeStamp only.

Review commit: https://reviewboard.mozilla.org/r/66500/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66500/

Comment 123

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Bug 1201363 - MediaStreamVideoSink for MediaPipelineTransmit case.

Replace |MediaPipelineTransmit::PipelineListener::NotifyQueuedTrackChanges| with |MediaPipelineTransmit::PipelineVideoSink::SetCurrentFrames|. We only need to deal with the video case since audio will be routed to |NotifyQueuedAudioData|.

Review commit: https://reviewboard.mozilla.org/r/66502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66502/

Comment 124

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Bug 1201363 - Do not copy video segment to StreamTracks in TrackUnionStream.

Now everything is ready. We can make NotifyQueuedTrackChanges only triggered by TRACK_EVENT_CREATED and TRACK_EVENT_ENDED without breaking anything. Also we make TrackUnionStream no longer copying data in video case.

Review commit: https://reviewboard.mozilla.org/r/66504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66504/

Comment 125

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709284 [details]
Bug 1201363 - Adding base class MediaStreamVideoSink and changing VideoFrameContainer to be inherited from MediaStreamVideoSink.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31383/diff/3-4/

Comment 126

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8734724 [details]
Bug 1201363 - Replace VideoFrameContainer with MediaStreamVideoSink in MSG.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42459/diff/1-2/

Comment 127

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Hi, Randell,
I found a Intermittent yesterday and fixing it. This is still a wip patch. There are some nits in the patch. You might want to see the logic first if you have time. Thanks.

Comment 128

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Hi, Randell,
I found a Intermittent yesterday and fixing it. This is still a wip patch. There are some nits in the patch. You might want to see the logic first if you have time. Thanks.

Comment 129

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8709284 [details]
Bug 1201363 - Adding base class MediaStreamVideoSink and changing VideoFrameContainer to be inherited from MediaStreamVideoSink.

https://reviewboard.mozilla.org/r/31383/#review63250

Comment 130

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8734724 [details]
Bug 1201363 - Replace VideoFrameContainer with MediaStreamVideoSink in MSG.

https://reviewboard.mozilla.org/r/42459/#review63252

Comment 131

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8773828 [details]
Bug 1201363 - Let MediaStreamVideoSink bind with particular video track.

https://reviewboard.mozilla.org/r/66490/#review63254

Comment 132

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8773829 [details]
Bug 1201363 - Register MediaStreamVideoSink into SourceMediaStream.

https://reviewboard.mozilla.org/r/66492/#review63732

::: dom/media/MediaStreamListener.h:249
(Diff revision 1)
>     *    didn't exist. This should only happen if you try to install the listener
>     *    directly to a SourceMediaStream that doesn't contain the given TrackID.
>     * STREAM_NOT_SUPPORTED
>     *    While looking for the data source of this track, we found a MediaStream
>     *    that is not a SourceMediaStream or a TrackUnionStream.
> +   * ALREADY_EXIST

Small preference for "ALREADY_EXISTS" (S on the end).  Not critial, just reads a little better.

Comment 133

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8773830 [details]
Bug 1201363 - Adding Add/RemoveVideoOutput into VideoStreamTrack.

https://reviewboard.mozilla.org/r/66494/#review63734

Comment 134

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8773831 [details]
Bug 1201363 - Call MediaStreamVideoSink::setCurrentFrames in SourceMediaStream::AppendToTrack.

https://reviewboard.mozilla.org/r/66496/#review63750

Comment 135

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8773832 [details]
Bug 1201363 - MediaStreamVideoSink for ImageCapture case.

https://reviewboard.mozilla.org/r/66498/#review63792

Comment 136

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8773833 [details]
Bug 1201363 - MediaStreamVideoSink for MediaRecorder case.

https://reviewboard.mozilla.org/r/66500/#review63796

::: dom/media/MediaRecorder.cpp:791
(Diff revision 1)
> +    // fixme: what a mess!
> +    nsTArray<RefPtr<mozilla::dom::VideoStreamTrack>> videoTracks;
>      DOMMediaStream* domStream = mRecorder->Stream();
> +    if (domStream) {
> +      domStream->GetVideoTracks(videoTracks);
> +      if (!videoTracks.IsEmpty()) {
> +        // Right now, the MediaRecorder hasn't dealt with multiple video track
> +        // issues. So we just bind with the first video track.
> +        videoTracks[0]->AddDirectListener(mEncoder->GetVideoSink());

File a bug for multiple track support (P4, Rank 45) and reference it here.

::: dom/media/encoder/TrackEncoder.cpp:286
(Diff revision 1)
> +      // fixme: what a mess :(
> +      StreamTime duration;
> +      if (mFirstFrame)
> +      {
> +        duration = chunk.GetDuration();
> +        mFirstFrame = false;
> +      } else {
> +        MOZ_ASSERT(chunk.mTimeStamp >= mLastFrameTimeStamp);
> +        TimeDuration timeDuration = chunk.mTimeStamp - mLastFrameTimeStamp;
> +        duration = SecondsToMediaTime(timeDuration.ToSeconds());
> +      }

So this sets the duration to the duration of the *previous* frame - which may well be what the current code is doing.  It's not correct (though it's convenient).  Either fix it, or if that cause problems file as a followup (and likely it will cause problems, since to fix it you have to buffer one frame of video (and encode it on any sort of "end" of the recording, using a made-up duration).

Mark any followup bug # here, and comment what's going on

Comment 137

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8773834 [details]
Bug 1201363 - MediaStreamVideoSink for MediaPipelineTransmit case.

https://reviewboard.mozilla.org/r/66502/#review63804

Comment 138

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8773835 [details]
Bug 1201363 - Do not copy video segment to StreamTracks in TrackUnionStream.

https://reviewboard.mozilla.org/r/66504/#review63806

Comment 139

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709284 [details]
Bug 1201363 - Adding base class MediaStreamVideoSink and changing VideoFrameContainer to be inherited from MediaStreamVideoSink.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31383/diff/4-5/

Comment 140

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8734724 [details]
Bug 1201363 - Replace VideoFrameContainer with MediaStreamVideoSink in MSG.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42459/diff/2-3/

Comment 141

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773828 [details]
Bug 1201363 - Let MediaStreamVideoSink bind with particular video track.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66490/diff/1-2/

Comment 142

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773829 [details]
Bug 1201363 - Register MediaStreamVideoSink into SourceMediaStream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66492/diff/1-2/

Comment 143

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773830 [details]
Bug 1201363 - Adding Add/RemoveVideoOutput into VideoStreamTrack.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66494/diff/1-2/

Comment 144

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773831 [details]
Bug 1201363 - Call MediaStreamVideoSink::setCurrentFrames in SourceMediaStream::AppendToTrack.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66496/diff/1-2/

Comment 145

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773832 [details]
Bug 1201363 - MediaStreamVideoSink for ImageCapture case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66498/diff/1-2/

Comment 146

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773833 [details]
Bug 1201363 - MediaStreamVideoSink for MediaRecorder case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66500/diff/1-2/

Comment 147

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773834 [details]
Bug 1201363 - MediaStreamVideoSink for MediaPipelineTransmit case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66502/diff/1-2/

Comment 148

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773835 [details]
Bug 1201363 - Do not copy video segment to StreamTracks in TrackUnionStream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66504/diff/1-2/

Comment 149

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4b154ebab9f

Comment 150

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/726ee2bcfdef
Adding base class MediaStreamVideoSink and changing VideoFrameContainer to be inherited from MediaStreamVideoSink. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/b018ce9d1484
Replace VideoFrameContainer with MediaStreamVideoSink in MSG. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/488e4a5e859b
Let MediaStreamVideoSink bind with particular video track. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc59ebb35b26
Register MediaStreamVideoSink into SourceMediaStream. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6bafaf6bbe6
Adding Add/RemoveVideoOutput into VideoStreamTrack. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/06c0a7eb7bc6
Call MediaStreamVideoSink::setCurrentFrames in SourceMediaStream::AppendToTrack. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c8af95cd440
MediaStreamVideoSink for ImageCapture case. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b926a6a6e4e
MediaStreamVideoSink for MediaRecorder case. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/f239b2ba9c46
MediaStreamVideoSink for MediaPipelineTransmit case. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/e350d8c25d2e
Do not copy video segment to StreamTracks in TrackUnionStream. r=jesup

Comment 151

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=33074814&repo=mozilla-inbound#L11920

Comment 152

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f02b3b69812c
Backed out changeset e350d8c25d2e 
https://hg.mozilla.org/integration/mozilla-inbound/rev/e30ccea2a4c3
Backed out changeset f239b2ba9c46 
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9df948898fb
Backed out changeset 5b926a6a6e4e 
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7406361245e
Backed out changeset 1c8af95cd440 
https://hg.mozilla.org/integration/mozilla-inbound/rev/a88a42b482e5
Backed out changeset 06c0a7eb7bc6 
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6681a5accdc
Backed out changeset a6bafaf6bbe6 
https://hg.mozilla.org/integration/mozilla-inbound/rev/44340f69bd3e
Backed out changeset cc59ebb35b26 
https://hg.mozilla.org/integration/mozilla-inbound/rev/84c75d35b12e
Backed out changeset 488e4a5e859b 
https://hg.mozilla.org/integration/mozilla-inbound/rev/09d825a0f11a
Backed out changeset b018ce9d1484 
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cf2433f2dad
Backed out changeset 726ee2bcfdef for static build bustage

Comment 153

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

(In reply to Carsten Book [:Tomcat] from comment #151)
> backed out for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=33074814&repo=mozilla-
> inbound#L11920

Sorry for that. I will fix it. Thanks.

Comment 154

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773832 [details]
Bug 1201363 - MediaStreamVideoSink for ImageCapture case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66498/diff/2-3/

Comment 155

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773833 [details]
Bug 1201363 - MediaStreamVideoSink for MediaRecorder case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66500/diff/2-3/

Comment 156

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773834 [details]
Bug 1201363 - MediaStreamVideoSink for MediaPipelineTransmit case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66502/diff/2-3/

Comment 157

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773835 [details]
Bug 1201363 - Do not copy video segment to StreamTracks in TrackUnionStream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66504/diff/2-3/

Comment 158

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Fixed busted. The latest try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdfacbbede3a&selectedJob=25014280

Comment 159

[Carsten Book [:Tomcat]]

Hi, just wondering when i import this patches from reviewboard just the 4 patches you changed get imported to commit is this expected ?

Comment 160

[Carsten Book [:Tomcat]]

all good found a different way of importing

Comment 161

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b531c8bff6e9
Adding base class MediaStreamVideoSink and changing VideoFrameContainer to be inherited from MediaStreamVideoSink. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/3851902daa94
Replace VideoFrameContainer with MediaStreamVideoSink in MSG. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/263cc3ad83c2
Let MediaStreamVideoSink bind with particular video track. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/91e5ccbeef19
Register MediaStreamVideoSink into SourceMediaStream. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/0744bfe6854f
Adding Add/RemoveVideoOutput into VideoStreamTrack. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/4111e388bd90
Call MediaStreamVideoSink::setCurrentFrames in SourceMediaStream::AppendToTrack. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c07c4aa3569
MediaStreamVideoSink for ImageCapture case. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/64b256cf2807
MediaStreamVideoSink for MediaRecorder case. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d2c395386ba
MediaStreamVideoSink for MediaPipelineTransmit case. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecee67ed1ddf
Do not copy video segment to StreamTracks in TrackUnionStream. r=jesup

Comment 162

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

(In reply to Carsten Book [:Tomcat] from comment #160)
> all good found a different way of importing

Thanks. :)

Comment 163

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for timeout in GTest GTest VP8VideoTrackEncoder.FrameEncode:

https://hg.mozilla.org/integration/mozilla-inbound/rev/89c54b6f4ed51fdf510f49b0dbd3060462011181
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea07e54bae12cba798fd51fab742af6fe4136bf6
https://hg.mozilla.org/integration/mozilla-inbound/rev/388625618f48e4ea8350c1fe15b2b9af1798ee7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/01fb24e26c762131bfb77dd1b9b7c7a9f93ed7d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/de996e6fce4fbd2aea68b32cead03424475f0da3
https://hg.mozilla.org/integration/mozilla-inbound/rev/74562d5542282a379830bd0f75ca246d7c3b72f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/e136f51425edced466815be42ad91f75f66d7de4
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f3fed235d0a9728de389d8d8a10135ce12e75ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea1d73ff5344cae48aca051521cb21a0b5b96d42
https://hg.mozilla.org/integration/mozilla-inbound/rev/3997ac8715f36e85b02476083029ea98549f2e94

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ecee67ed1ddf9169f89980b97f123566897fcef4
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=33214975&repo=mozilla-inbound

Comment 164

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8709284 [details]
Bug 1201363 - Adding base class MediaStreamVideoSink and changing VideoFrameContainer to be inherited from MediaStreamVideoSink.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31383/diff/5-6/

Comment 165

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8734724 [details]
Bug 1201363 - Replace VideoFrameContainer with MediaStreamVideoSink in MSG.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42459/diff/3-4/

Comment 166

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773828 [details]
Bug 1201363 - Let MediaStreamVideoSink bind with particular video track.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66490/diff/2-3/

Comment 167

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773829 [details]
Bug 1201363 - Register MediaStreamVideoSink into SourceMediaStream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66492/diff/2-3/

Comment 168

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773830 [details]
Bug 1201363 - Adding Add/RemoveVideoOutput into VideoStreamTrack.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66494/diff/2-3/

Comment 169

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773831 [details]
Bug 1201363 - Call MediaStreamVideoSink::setCurrentFrames in SourceMediaStream::AppendToTrack.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66496/diff/2-3/

Comment 170

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773832 [details]
Bug 1201363 - MediaStreamVideoSink for ImageCapture case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66498/diff/3-4/

Comment 171

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773833 [details]
Bug 1201363 - MediaStreamVideoSink for MediaRecorder case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66500/diff/3-4/

Comment 172

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773834 [details]
Bug 1201363 - MediaStreamVideoSink for MediaPipelineTransmit case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66502/diff/3-4/

Comment 173

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773835 [details]
Bug 1201363 - Do not copy video segment to StreamTracks in TrackUnionStream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66504/diff/3-4/

Comment 174

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773833 [details]
Bug 1201363 - MediaStreamVideoSink for MediaRecorder case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66500/diff/4-5/

Comment 175

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773834 [details]
Bug 1201363 - MediaStreamVideoSink for MediaPipelineTransmit case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66502/diff/4-5/

Comment 176

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8773835 [details]
Bug 1201363 - Do not copy video segment to StreamTracks in TrackUnionStream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66504/diff/4-5/

Comment 177

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #163)
> Backed out for timeout in GTest GTest VP8VideoTrackEncoder.FrameEncode:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 89c54b6f4ed51fdf510f49b0dbd3060462011181
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> ea07e54bae12cba798fd51fab742af6fe4136bf6
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 388625618f48e4ea8350c1fe15b2b9af1798ee7c
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 01fb24e26c762131bfb77dd1b9b7c7a9f93ed7d4
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> de996e6fce4fbd2aea68b32cead03424475f0da3
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 74562d5542282a379830bd0f75ca246d7c3b72f3
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> e136f51425edced466815be42ad91f75f66d7de4
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 4f3fed235d0a9728de389d8d8a10135ce12e75ff
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> ea1d73ff5344cae48aca051521cb21a0b5b96d42
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 3997ac8715f36e85b02476083029ea98549f2e94
> 
> Push with failures:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=ecee67ed1ddf9169f89980b97f123566897fcef4
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=33214975&repo=mozilla-
> inbound

Gtest fixed. The latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed22de378f20

Comment 178

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3295f400cd3
Adding base class MediaStreamVideoSink and changing VideoFrameContainer to be inherited from MediaStreamVideoSink. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/02fc34b73508
Replace VideoFrameContainer with MediaStreamVideoSink in MSG. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/94a4304e69df
Let MediaStreamVideoSink bind with particular video track. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/8248c1bc1b17
Register MediaStreamVideoSink into SourceMediaStream. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ec3f1abad0a
Adding Add/RemoveVideoOutput into VideoStreamTrack. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/647046fe760c
Call MediaStreamVideoSink::setCurrentFrames in SourceMediaStream::AppendToTrack. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/36328de44ea7
MediaStreamVideoSink for ImageCapture case. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/d151c9b4885a
MediaStreamVideoSink for MediaRecorder case. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd0556b9b370
MediaStreamVideoSink for MediaPipelineTransmit case. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/a02925fe4ded
Do not copy video segment to StreamTracks in TrackUnionStream. r=jesup

Comment 179

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d3295f400cd3
https://hg.mozilla.org/mozilla-central/rev/02fc34b73508
https://hg.mozilla.org/mozilla-central/rev/94a4304e69df
https://hg.mozilla.org/mozilla-central/rev/8248c1bc1b17
https://hg.mozilla.org/mozilla-central/rev/8ec3f1abad0a
https://hg.mozilla.org/mozilla-central/rev/647046fe760c
https://hg.mozilla.org/mozilla-central/rev/36328de44ea7
https://hg.mozilla.org/mozilla-central/rev/d151c9b4885a
https://hg.mozilla.org/mozilla-central/rev/cd0556b9b370
https://hg.mozilla.org/mozilla-central/rev/a02925fe4ded