Stop buffering video in the MediaStreamGraph

RESOLVED FIXED in Firefox 51

Status

()

defect
P1
normal
Rank:
18
RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: roc, Assigned: ctai)

Tracking

(Depends on 1 bug, Blocks 4 bugs)

Trunk
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox51 fixed)

Details

Attachments

(10 attachments, 18 obsolete attachments)

58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
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.
Assignee: nobody → ctai
Sounds like this will let us throw out CameraPreviewMediaStream.
Blocks: 1124630
Component: Audio/Video → Audio/Video: MSG/cubeb/GMP
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.
Flags: needinfo?(roc)
(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.
Flags: needinfo?(roc)
(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).
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
There may be another patch I was thinking of; but that seems to be in the area
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
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.
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.
Flags: needinfo?(roc)
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.
Flags: needinfo?(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.
Flags: needinfo?(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.
...
> 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.
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
(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.
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
(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!
Flags: needinfo?(roc)
(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. :)
(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).
Rank: 18
Priority: -- → P1
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.
>
Flags: needinfo?(jwwang)
Flags: needinfo?(roc)
(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.
Flags: needinfo?(roc)
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?
Flags: needinfo?(jwwang)
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/
Attachment #8709281 - Flags: review?(roc)
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/
Attachment #8709282 - Flags: review?(roc)
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/
Attachment #8709283 - Flags: review?(roc)
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/
Attachment #8709284 - Flags: review?(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/
Attachment #8709285 - Flags: review?(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/
Attachment #8709286 - Flags: review?(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/
Attachment #8709287 - Flags: review?(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/
Attachment #8709288 - Flags: review?(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/
Attachment #8709289 - Flags: review?(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/
Attachment #8709290 - Flags: review?(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/
Attachment #8709291 - Flags: review?(roc)
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. :)
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 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 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 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 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 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 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 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 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 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 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 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/
https://reviewboard.mozilla.org/r/31389/#review28623

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

Fixed in Revision 2.
Update the patches to fix some issues which found in try server.
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
Attachment #8709281 - Flags: review?(roc)
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
Attachment #8709282 - Flags: review?(roc) → review+
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
Attachment #8709283 - Flags: review?(roc) → review+
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!
Attachment #8709284 - Flags: review?(roc) → review+
Attachment #8709285 - Flags: review?(roc) → review+
Comment on attachment 8709285 [details]
MozReview Request: Bug 1201363 - Replace VideoFrameContainer with MediaStreamVideoSink in MSG. r?roc

https://reviewboard.mozilla.org/r/31385/#review29015
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 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.
Attachment #8709286 - Flags: review?(roc)
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 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.
Let's deal with those comments before I do the rest of the reviews.
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.
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
Flags: needinfo?(rjesup)
Flags: needinfo?(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/
Attachment #8734723 - Flags: review?(rjesup)
Attachment #8734723 - Flags: review?(pehrsons)
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/
Attachment #8709282 - Attachment description: MozReview Request: Bug 1201363 - Add ImageSizeChangedListener to notify image size changed. r?roc → MozReview Request: Bug 1201363 - Add ImageSizeChangedListener to notify image size changed. r?jesup,r?pehrsons
Attachment #8709282 - Flags: review?(rjesup)
Attachment #8709282 - Flags: review?(pehrsons)
Attachment #8709283 - Attachment description: MozReview Request: Bug 1201363 - Add |MediaStreamListener::NotifyQueuedAudioData| and separate non TRACK_CREATE and TRACK_END part to NotifyQueuedAudioData. r?roc → MozReview Request: Bug 1201363 - Add |MediaStreamListener::NotifyQueuedAudioData| and separate non TRACK_CREATE and TRACK_END part to NotifyQueuedAudioData. r?jesup,r?pehrsons
Attachment #8709283 - Flags: review?(rjesup)
Attachment #8709283 - Flags: review?(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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31381/diff/2-3/
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/
Attachment #8709284 - Attachment description: MozReview Request: Bug 1201363 - Adding base class MediaStreamVideoSink and changing VideoFrameContainer to be inherited from MediaStreamVideoSink. r?roc → MozReview Request: Bug 1201363 - Adding base class MediaStreamVideoSink and changing VideoFrameContainer to be inherited from MediaStreamVideoSink. r?jesup,r?pehrsons
Attachment #8709284 - Flags: review?(rjesup)
Attachment #8709284 - Flags: review?(pehrsons)
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/
Attachment #8734724 - Flags: review?(rjesup)
Attachment #8734724 - Flags: review?(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/
Attachment #8734725 - Flags: review?(rjesup)
Attachment #8734725 - Flags: review?(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/
Attachment #8734726 - Flags: review?(rjesup)
Attachment #8734726 - Flags: review?(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/
Attachment #8734727 - Flags: review?(rjesup)
Attachment #8734727 - Flags: review?(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/
Attachment #8734728 - Flags: review?(rjesup)
Attachment #8734728 - Flags: review?(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/
Attachment #8734729 - Flags: review?(rjesup)
Attachment #8734729 - Flags: review?(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/
Attachment #8734730 - Flags: review?(rjesup)
Attachment #8734730 - Flags: review?(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/
Attachment #8734731 - Flags: review?(rjesup)
Attachment #8734731 - Flags: review?(pehrsons)
Attachment #8709281 - Attachment is obsolete: true
Attachment #8709285 - Attachment is obsolete: true
Attachment #8709286 - Attachment is obsolete: true
Attachment #8709287 - Attachment is obsolete: true
Attachment #8709288 - Attachment is obsolete: true
Attachment #8709289 - Attachment is obsolete: true
Attachment #8709290 - Attachment is obsolete: true
Attachment #8709291 - Attachment is obsolete: true
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
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.
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.
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!
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/
(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.
Flags: needinfo?(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?
Attachment #8709282 - Flags: review?(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
Attachment #8734723 - Flags: review?(pehrsons) → review+
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`.
Attachment #8709283 - Flags: review?(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?
Attachment #8709284 - Flags: review?(pehrsons) → review+
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 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.
Attachment #8734724 - Flags: review?(pehrsons) → review+
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.
Attachment #8734725 - Flags: review?(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.
Attachment #8734726 - Flags: review?(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?
Attachment #8734727 - Flags: review?(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.
Attachment #8734728 - Flags: review?(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.
Attachment #8734729 - Flags: review?(pehrsons)
Attachment #8734730 - Flags: review?(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 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
Attachment #8734731 - Flags: review?(pehrsons) → review+
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?
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.
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.
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.
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.
Depends on: 1208371
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.
Flags: needinfo?(rjesup)
Attachment #8709282 - Flags: review?(rjesup)
Attachment #8709283 - Flags: review?(rjesup)
Attachment #8709284 - Flags: review?(rjesup)
Attachment #8734723 - Flags: review?(rjesup)
Attachment #8734724 - Flags: review?(rjesup)
Depends on: 1266644
Depends on: 1266646
Depends on: 1266647
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.
Flags: needinfo?(rjesup)
Flags: needinfo?(karlt)
(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.
Flags: needinfo?(rjesup)
> 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 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
Attachment #8734725 - Flags: review?(rjesup)
Attachment #8734726 - Flags: review?(rjesup)
Attachment #8734727 - Flags: review?(rjesup)
Attachment #8734728 - Flags: review?(rjesup)
Attachment #8734729 - Flags: review?(rjesup)
Attachment #8734730 - Flags: review?(rjesup)
Attachment #8734731 - Flags: review?(rjesup)
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.
Flags: needinfo?(karlt)
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.
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.
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?
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.
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. :)
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. :)
Are there new patches to review?
Flags: needinfo?(ctai)
Review commit: https://reviewboard.mozilla.org/r/66490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66490/
Attachment #8709284 - Attachment description: MozReview Request: Bug 1201363 - Adding base class MediaStreamVideoSink and changing VideoFrameContainer to be inherited from MediaStreamVideoSink. r?jesup,r?pehrsons → Bug 1201363 - Adding base class MediaStreamVideoSink and changing VideoFrameContainer to be inherited from MediaStreamVideoSink.
Attachment #8734724 - Attachment description: MozReview Request: Bug 1201363 - Replace VideoFrameContainer with MediaStreamVideoSink in MSG. r?jesup,r?pehrsons → Bug 1201363 - Replace VideoFrameContainer with MediaStreamVideoSink in MSG.
Attachment #8773828 - Flags: review?(rjesup)
Attachment #8773829 - Flags: review?(rjesup)
Attachment #8773830 - Flags: review?(rjesup)
Attachment #8773831 - Flags: review?(rjesup)
Attachment #8773832 - Flags: review?(rjesup)
Attachment #8773833 - Flags: review?(rjesup)
Attachment #8773834 - Flags: review?(rjesup)
Attachment #8773835 - Flags: review?(rjesup)
Attachment #8709284 - Flags: review?(rjesup)
Attachment #8709284 - Flags: review+
Attachment #8734724 - Flags: review+ → review?(rjesup)
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/
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/
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/
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/
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 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 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/
Attachment #8734723 - Attachment is obsolete: true
Attachment #8709282 - Attachment is obsolete: true
Attachment #8709283 - Attachment is obsolete: true
Attachment #8734725 - Attachment is obsolete: true
Attachment #8734726 - Attachment is obsolete: true
Attachment #8734727 - Attachment is obsolete: true
Attachment #8734728 - Attachment is obsolete: true
Attachment #8734729 - Attachment is obsolete: true
Attachment #8734730 - Attachment is obsolete: true
Attachment #8734731 - Attachment is obsolete: true
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.
Flags: needinfo?(ctai)
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 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
Attachment #8709284 - Flags: review?(rjesup) → review+
Comment on attachment 8734724 [details]
Bug 1201363 - Replace VideoFrameContainer with MediaStreamVideoSink in MSG.

https://reviewboard.mozilla.org/r/42459/#review63252
Attachment #8734724 - Flags: review?(rjesup) → review+
Comment on attachment 8773828 [details]
Bug 1201363 - Let MediaStreamVideoSink bind with particular video track.

https://reviewboard.mozilla.org/r/66490/#review63254
Attachment #8773828 - Flags: review?(rjesup) → review+
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.
Attachment #8773829 - Flags: review?(rjesup) → review+
Comment on attachment 8773830 [details]
Bug 1201363 - Adding Add/RemoveVideoOutput into VideoStreamTrack.

https://reviewboard.mozilla.org/r/66494/#review63734
Attachment #8773830 - Flags: review?(rjesup) → review+
Comment on attachment 8773831 [details]
Bug 1201363 - Call MediaStreamVideoSink::setCurrentFrames in SourceMediaStream::AppendToTrack.

https://reviewboard.mozilla.org/r/66496/#review63750
Attachment #8773831 - Flags: review?(rjesup) → review+
Attachment #8773832 - Flags: review?(rjesup) → review+
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
Attachment #8773833 - Flags: review?(rjesup) → review+
Comment on attachment 8773834 [details]
Bug 1201363 - MediaStreamVideoSink for MediaPipelineTransmit case.

https://reviewboard.mozilla.org/r/66502/#review63804
Attachment #8773834 - Flags: review?(rjesup) → review+
Comment on attachment 8773835 [details]
Bug 1201363 - Do not copy video segment to StreamTracks in TrackUnionStream.

https://reviewboard.mozilla.org/r/66504/#review63806
Attachment #8773835 - Flags: review?(rjesup) → review+
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/
Attachment #8773829 - Attachment description: Bug 1201363 - Register MediaStreamVideoSink into SourceMediaStream. . → Bug 1201363 - Register MediaStreamVideoSink into SourceMediaStream.
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 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 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 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 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 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 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 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 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 150

3 years ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
(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.
Flags: needinfo?(ctai)
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 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 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 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/
Hi, just wondering when i import this patches from reviewboard just the 4 patches you changed get imported to commit is this expected ?
Flags: needinfo?(ctai)
all good found a different way of importing
Flags: needinfo?(ctai)

Comment 161

3 years ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #160)
> all good found a different way of importing

Thanks. :)
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 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 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 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 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 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 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 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 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 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 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 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 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/
(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
Flags: needinfo?(ctai)
Keywords: checkin-needed

Comment 178

3 years ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
No longer blocks: 1291951
No longer blocks: 1291953
No longer blocks: 1291946
Depends on: 1293015
Depends on: 1292335
Depends on: 1198168
No longer depends on: 1198168
Depends on: 1298428
Depends on: 1299714
Depends on: 1298305

Updated

3 years ago
Depends on: 1300871
No longer depends on: 1299860
Depends on: 1348381
Depends on: 1354457
Depends on: 1506093
You need to log in before you can comment on or make changes to this bug.