Last Comment Bug 779715 - Create ProcessedMediaStream infrastructure and use it to fix media element mozCaptureStream
: Create ProcessedMediaStream infrastructure and use it to fix media element mo...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Web Audio (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla17
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on: 779721 786081 898967 966800
Blocks: webaudio
  Show dependency treegraph
 
Reported: 2012-08-01 19:48 PDT by Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
Modified: 2014-02-02 12:56 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Add basic support for ProcessedMediaStreams (54.82 KB, patch)
2012-08-03 16:56 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
rjesup: review+
Details | Diff | Splinter Review
Part 2: Create TrackUnionStream (13.37 KB, patch)
2012-08-03 16:59 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
rjesup: review+
Details | Diff | Splinter Review
Part 3: Add an API to get notifications of changes to the main-thread-visible state of a MediaStream (6.31 KB, patch)
2012-08-03 17:00 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
rjesup: review+
Details | Diff | Splinter Review
Part 4: Play all tracks of a media stream with multiple tracks (21.11 KB, text/plain)
2012-08-03 17:00 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
rjesup: review+
Details
Part 5: Rename identifiers related to media element 'src' MediaStreams to be more specifically about 'src' (21.65 KB, patch)
2012-08-03 17:02 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
cpearce: review+
Details | Diff | Splinter Review
Part 6: Rework capturing MediaStreams from media elements to use TrackUnionStreams (50.56 KB, patch)
2012-08-03 17:04 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
cpearce: review+
rjesup: review+
Details | Diff | Splinter Review
Part 7: Update test_streams_element_capture_reset.html to test new functionality (4.77 KB, patch)
2012-08-03 17:16 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
cpearce: review+
Details | Diff | Splinter Review
Part 8: Make it safe to call MediaInputPort::Destroy after streams a t both ends of the port have been destroyed (3.54 KB, patch)
2012-08-21 02:17 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
rjesup: review+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-01 19:48:37 PDT
Currently the media element methods mozCaptureStream and mozCaptureStreamUntilEnded are quite broken when the media element seeks or restarts playback after ending.

I'm fixing this by introducing stream processing and creating a TrackUnionStream that aggregates the tracks of its input streams. Then I make nsBuiltinDecoder create a new SourceMediaStream each time we play back the resource, i.e., after each seek or end it creates a new SourceMediaStream. mozCaptureStream creates a TrackUnionStream and each SourceMediaStream created for the element is wired up as the sole input to all the element's TrackUnionStreams. mozCaptureStreamUntilEnded behaves the same way, except that when resource playback actually ends we forget about the TrackUnionStream so that no future SourceMediaStream is added as an input.

TrackUnionStream will also be useful for WebRTC. The ProcessedMediaStream superclass will be useful for Web Audio.
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-02 07:02:26 PDT
https://tbpl.mozilla.org/?tree=Try&rev=deeca015a677
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-02 20:05:21 PDT
Fixed some bugs.
https://tbpl.mozilla.org/?tree=Try&rev=1ac2482674f2
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-02 20:21:58 PDT
Fixed warnings-as-errors bustage.
https://tbpl.mozilla.org/?tree=Try&rev=8cc459506ae3
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-03 16:56:24 PDT
Created attachment 648902 [details] [diff] [review]
Part 1: Add basic support for ProcessedMediaStreams
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-03 16:59:09 PDT
Created attachment 648904 [details] [diff] [review]
Part 2: Create TrackUnionStream

I have put the entire implementation in the header file since it's only included from MediaStreamGraph.cpp and we don't have to separate definitions from declaratoins that way. I could break a lot of it out into TrackUnionStream.cpp if you think that's better.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-03 17:00:03 PDT
Created attachment 648905 [details] [diff] [review]
Part 3: Add an API to get notifications of changes to the main-thread-visible state of a MediaStream
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-03 17:00:36 PDT
Created attachment 648906 [details]
Part 4: Play all tracks of a media stream with multiple tracks
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-03 17:02:44 PDT
Created attachment 648908 [details] [diff] [review]
Part 5: Rename identifiers related to media element 'src' MediaStreams to be more specifically about 'src'
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-03 17:04:47 PDT
Created attachment 648909 [details] [diff] [review]
Part 6: Rework capturing MediaStreams from media elements to use TrackUnionStreams

This is the biggest patch of the lot.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-03 17:16:33 PDT
Created attachment 648912 [details] [diff] [review]
Part 7: Update test_streams_element_capture_reset.html to test new functionality
Comment 11 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-08-05 14:55:43 PDT
Comment on attachment 648909 [details] [diff] [review]
Part 6: Rework capturing MediaStreams from media elements to use TrackUnionStreams

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

::: content/media/nsBuiltinDecoder.cpp
@@ +124,5 @@
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
> +  mReentrantMonitor.AssertCurrentThreadIn();
> +  LOG(PR_LOG_DEBUG, ("nsBuiltinDecoder::RecreateDecodedStream this=%p aStartTimeUSecs=%lld!",
> +                     this, (long long)aStartTimeUSecs));

You shouldn't need the (long long) cast?

@@ +689,5 @@
> +      }
> +    }
> +  }
> +
> +  printf("nsBuiltinDecoder::PlaybackEnded this=%p mPlayState=%d\n", this, mPlayState);

This should use LOG, not printf.

@@ +908,5 @@
>        ChangeState(PLAY_STATE_SEEKING);
>        seekWasAborted = true;
>      } else {
>        UnpinForSeek();
> +      printf("nsBuiltinDecoder::SeekingStopped, this=%p next state=%d\n", this, mNextState);

Use LOG not printf.

@@ +992,5 @@
> +      mDecodedStream->mStream->ChangeExplicitBlockerCount(blockForPlayState ? 1 : -1);
> +      mDecodedStream->mHaveBlockedForPlayState = blockForPlayState;
> +    }
> +  }
> +  printf("nsBuiltinDecoder::ChangeState this=%p prev=%d new=%d\n", this, mPlayState, aState);

LOG not printf.

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +571,2 @@
>  {
>    mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();

It would be good if you could assert what thread this runs on; it's a good way to document what's going on.
Comment 12 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-08-05 15:09:27 PDT
Comment on attachment 648912 [details] [diff] [review]
Part 7: Update test_streams_element_capture_reset.html to test new functionality

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

::: content/media/test/test_streams_element_capture_reset.html
@@ +45,5 @@
>    };
> +  function endedAfterSeek() {
> +    isWithinEps(v.currentTime, test.duration, "checking v.currentTime at second 'ended' event");
> +    isWithinEps(vout.currentTime, (v.currentTime - seekTime) + test.duration,
> +	            "checking vout.currentTime after seeking and playing through again");

Looks like there's tabs messing up indentation here.

@@ +58,3 @@
>      v.removeEventListener("seeked", seeked, false);
> +    v.addEventListener("ended", endedAfterSeek, false);
> +	v.play();

So I'm guessing vout_untilended should *not* play again now, since the captured stream ended? Maybe you should add checks to ensure that?

Tabs here too.

@@ +61,5 @@
>    };
> +  function ended() {
> +    isWithinEps(vout.currentTime, test.duration, "checking vout.currentTime at first 'ended' event");
> +    isWithinEps(v.currentTime, test.duration, "checking v.currentTime at first 'ended' event");
> +	is(vout.ended, false, "checking vout has not ended");

Indentation seems off here? Are there tabs instead of spaces inside this function?
Comment 13 Randell Jesup [:jesup] 2012-08-09 15:14:27 PDT
Comment on attachment 648902 [details] [diff] [review]
Part 1: Add basic support for ProcessedMediaStreams

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

::: content/media/MediaStreamGraph.h
@@ +625,5 @@
> +  }
> +
> +  // Called on graph manager thread
> +  // Do not call these from outside MediaStreamGraph.cpp!
> +  void Init();

Is there a convenient way to enforce this or flag it if done from elsewhere?
Comment 14 Randell Jesup [:jesup] 2012-08-09 16:14:12 PDT
Comment on attachment 648904 [details] [diff] [review]
Part 2: Create TrackUnionStream

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

All looks good to me as best I can tell.

::: content/media/TrackUnionStream.h
@@ +15,5 @@
> +#else
> +#define LOG(type, msg)
> +#endif
> +
> +class TrackUnionStream : public ProcessedMediaStream {

Not that it's an issue, but is there a reason the entire TrackUnionStream impl is in the .h file?  If there is a specific reason, perhaps that should be noted; if it's just convenience then I don't think any note is needed.
Comment 15 Randell Jesup [:jesup] 2012-08-10 00:30:00 PDT
Comment on attachment 648906 [details]
Part 4: Play all tracks of a media stream with multiple tracks

"For video, we pick the last track that has a video frame for the current time, and display that."

+  for (StreamBuffer::TrackIter tracks(aStream->GetStreamBuffer(), MediaSegment::VIDEO);
+       !tracks.IsEnded(); tracks.Next()) {
+    VideoSegment* segment = tracks->Get<VideoSegment>();
+    TrackTicks thisStart;
+    const VideoFrame* thisFrame =
+      segment->GetFrameAt(tracks->TimeToTicksRoundDown(frameBufferTime), &thisStart);
+    if (thisFrame && thisFrame->GetImage()) {
+      start = thisStart;
+      frame = thisFrame;
+      track = tracks.get();
+    }
   }

I don't think it matter a whole lot, but if there are multiple video tracks, then this will get and throw away frames from all but the last, which seems... inefficient.  Also, isn't normally priority given to the first track not the last, up to now?

Does this mean it's impossible to process all the video tracks in a MediaStream from MediaStream Processing?  Can we take the individual tracks and break them into separate MediaStreams?  Can we process all of them, and then break them out?  Can we write a JS process filter that swizzles the video to show the track that has the most motion?  (Think security system with 8 cameras, or conference system with 4 cameras.)  Etc....

All that said, r+ on the code itself; the above are design questions.
Comment 16 Randell Jesup [:jesup] 2012-08-12 21:18:24 PDT
Comment on attachment 648909 [details] [diff] [review]
Part 6: Rework capturing MediaStreams from media elements to use TrackUnionStreams

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

Great comments.  All code should have ones like these.

::: content/media/nsBuiltinDecoder.cpp
@@ +108,5 @@
> +    // During cycle collection, nsDOMMediaStream can be destroyed and send
> +    // its Destroy message before this decoder is destroyed. So we have to
> +    // be careful not to send any messages after the Destroy().
> +    if (!os.mStream->IsDestroyed()) {
> +      os.mStream->ChangeExplicitBlockerCount(1);

I see the reentrantMonitor is held here.  Is that sufficient to avoid race conditions with the Cycle Collector destroying nsDOMMediaStream?  I.e. does nsDOMMediaStream hold mReentrantMonitor?

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +571,2 @@
>  {
>    mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();

Chris suggested asserting the thread this runs on, but I think the point of holding the ReentrantMonitor is that this can be called from more than one thread.

@@ +1493,1 @@
>    ScheduleStateMachine();

Vague concern: what if there are multiple Seek() calls before the state machine runs?  I.e. multiple RecreateDecodedStreams(), with (I assume) only the last one actually happening.  It may well be fine.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-19 20:57:31 PDT
(In reply to Chris Pearce (:cpearce) from comment #11)
> ::: content/media/nsBuiltinDecoder.cpp
> @@ +124,5 @@
> > +{
> > +  NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
> > +  mReentrantMonitor.AssertCurrentThreadIn();
> > +  LOG(PR_LOG_DEBUG, ("nsBuiltinDecoder::RecreateDecodedStream this=%p aStartTimeUSecs=%lld!",
> > +                     this, (long long)aStartTimeUSecs));
> 
> You shouldn't need the (long long) cast?

I don't know that we're allowed to assume PRInt64 == long long. This is safer.

> > +  printf("nsBuiltinDecoder::PlaybackEnded this=%p mPlayState=%d\n", this, mPlayState);
> 
> This should use LOG, not printf.

I just removed it.

> @@ +908,5 @@
> > +      printf("nsBuiltinDecoder::SeekingStopped, this=%p next state=%d\n", this, mNextState);
> 
> Use LOG not printf.

I just removed it.

> @@ +992,5 @@
> > +  printf("nsBuiltinDecoder::ChangeState this=%p prev=%d new=%d\n", this, mPlayState, aState);
> 
> LOG not printf.

I just removed it.

> ::: content/media/nsBuiltinDecoderStateMachine.cpp
> @@ +571,2 @@
> >  {
> >    mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();
> 
> It would be good if you could assert what thread this runs on; it's a good
> way to document what's going on.

Done.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-19 21:16:01 PDT
(In reply to Chris Pearce (:cpearce) from comment #12)
> So I'm guessing vout_untilended should *not* play again now, since the
> captured stream ended? Maybe you should add checks to ensure that?

Done. Everything else fixed.
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-19 21:18:20 PDT
(In reply to Randell Jesup [:jesup] from comment #13)
> > +  // Called on graph manager thread
> > +  // Do not call these from outside MediaStreamGraph.cpp!
> > +  void Init();
> 
> Is there a convenient way to enforce this or flag it if done from elsewhere?

It's a bit of a pain, because I call this from a class that's local to a function so there's no wait to friend it.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-19 21:26:37 PDT
(In reply to Randell Jesup [:jesup] from comment #14)
> Not that it's an issue, but is there a reason the entire TrackUnionStream
> impl is in the .h file?  If there is a specific reason, perhaps that should
> be noted; if it's just convenience then I don't think any note is needed.

Added a comment. It's only included from MediaStreamGraph.cpp so it's a bit simpler and less code to put it all in the .h file.

(In reply to Randell Jesup [:jesup] from comment #15)
> I don't think it matter a whole lot, but if there are multiple video tracks,
> then this will get and throw away frames from all but the last, which
> seems... inefficient.

Adding the ability to iterate tracks in reverse order just for this case seems not worthwhile.

> Also, isn't normally priority given to the first
> track not the last, up to now?

I think the behavior we actually want is to composite the tracks in track order so later tracks are rendered on top of earlier tracks. Since currently all our images are opaque, that means choosing the last one (modulo some issues around image sizes).

> Does this mean it's impossible to process all the video tracks in a
> MediaStream from MediaStream Processing?

No, this has nothing to do with processing. This only affects what gets rendered when a MediaStream is used as the source for a <video>.

> Can we take the individual tracks
> and break them into separate MediaStreams?  Can we process all of them, and
> then break them out?  Can we write a JS process filter that swizzles the
> video to show the track that has the most motion?  (Think security system
> with 8 cameras, or conference system with 4 cameras.)  Etc....

We can still do all of those things.
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-19 21:57:15 PDT
(In reply to Randell Jesup [:jesup] from comment #16)
> ::: content/media/nsBuiltinDecoder.cpp
> @@ +108,5 @@
> > +    // During cycle collection, nsDOMMediaStream can be destroyed and send
> > +    // its Destroy message before this decoder is destroyed. So we have to
> > +    // be careful not to send any messages after the Destroy().
> > +    if (!os.mStream->IsDestroyed()) {
> > +      os.mStream->ChangeExplicitBlockerCount(1);
> 
> I see the reentrantMonitor is held here.  Is that sufficient to avoid race
> conditions with the Cycle Collector destroying nsDOMMediaStream?  I.e. does
> nsDOMMediaStream hold mReentrantMonitor?

It doesn't. I'm not sure what race conditions you're worried about, since this code and the cycle collector both run on the main thread.

> ::: content/media/nsBuiltinDecoderStateMachine.cpp
> @@ +571,2 @@
> >  {
> >    mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();
> 
> Chris suggested asserting the thread this runs on, but I think the point of
> holding the ReentrantMonitor is that this can be called from more than one
> thread.

No, in quite a few places we hold the monitor and ensure we're only running on the main thread. We need to exclude other threads (decoder and state machine threads) from touching the decoder while we do our work.

> @@ +1493,1 @@
> >    ScheduleStateMachine();
> 
> Vague concern: what if there are multiple Seek() calls before the state
> machine runs?  I.e. multiple RecreateDecodedStreams(), with (I assume) only
> the last one actually happening.  It may well be fine.

It is fine. ScheduleStateMachine is idempotent.
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-19 22:06:53 PDT
https://tbpl.mozilla.org/?tree=Try&rev=33f238b77d48
Comment 23 Randell Jesup [:jesup] 2012-08-19 22:29:37 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> (In reply to Randell Jesup [:jesup] from comment #15)
> > I don't think it matter a whole lot, but if there are multiple video tracks,
> > then this will get and throw away frames from all but the last, which
> > seems... inefficient.
> 
> Adding the ability to iterate tracks in reverse order just for this case
> seems not worthwhile.

Given I think this returns a single frame, from the last track that provided one, wouldn't it makes sense to instead go backwards and return the first track that gives you one?  That said: multiple video tracks (especially >2) are very rare and I doubt ever will be common, though some specific usercases will use them), and if GetFrameAt() just returns a reference to it and doesn't actually copy data, then this is not worth worrying about.

> > Also, isn't normally priority given to the first
> > track not the last, up to now?
> 
> I think the behavior we actually want is to composite the tracks in track
> order so later tracks are rendered on top of earlier tracks. Since currently
> all our images are opaque, that means choosing the last one (modulo some
> issues around image sizes).

If we're compositing by default, the <video> tag spec needs a LOT of updating.  It currently provides several mechanisms for selecting a single track (of video), and none that appear to allow selecting multiple tracks at once.  If it did allow that, you'd really want the ability to place them independently, support for (partial) transparency, swizzling the order, etc, directly in the <video> tag (and spec).

I could be wrong; you've lived this *far* more than I, but that's what I'd understood, and what reading the spec now to check appears to uphold.
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-20 04:45:33 PDT
(In reply to Randell Jesup [:jesup] from comment #23)
> Given I think this returns a single frame, from the last track that provided
> one, wouldn't it makes sense to instead go backwards and return the first
> track that gives you one?

Yes, that code would be slightly simpler than what's here, but it would require us to add an API to iterate through tracks in reverse order. Currently we have no other use for that API and maybe we'll never have another use for it. So I don't think it's worth it at this time.

> if GetFrameAt() just returns a reference to it
> and doesn't actually copy data, then this is not worth worrying about.

Yes, we never copy Images. There isn't even an API to do that.

> > I think the behavior we actually want is to composite the tracks in track
> > order so later tracks are rendered on top of earlier tracks. Since currently
> > all our images are opaque, that means choosing the last one (modulo some
> > issues around image sizes).
> 
> If we're compositing by default, the <video> tag spec needs a LOT of
> updating.  It currently provides several mechanisms for selecting a single
> track (of video), and none that appear to allow selecting multiple tracks at
> once.

True.

> If it did allow that, you'd really want the ability to place them
> independently, support for (partial) transparency, swizzling the order, etc,
> directly in the <video> tag (and spec).

I don't want to go down that path. The main use for compositing video tracks that I've thought of is that we could have <canvas> elements produce a video track of their contents, and then you could use composite video to create "burned in" captions, special effects, etc.

But maybe we shouldn't bother with that. We could just render one video track and support the compositing use-cases with a special built-in ProcessedMediaStream subclass that composites video tracks together. Or, we could support it later with real-time JS video stream processing.

I can post a followup patch to select the first video track instead of the last, if you think that's the right way to go. I don't really mind.
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-20 14:55:14 PDT
Checked in parts 1-5:
http://hg.mozilla.org/integration/mozilla-inbound/rev/74e761adfc42
http://hg.mozilla.org/integration/mozilla-inbound/rev/965f5e18ee05
http://hg.mozilla.org/integration/mozilla-inbound/rev/c6fd7f4798c9
http://hg.mozilla.org/integration/mozilla-inbound/rev/86b2cd5958f3
http://hg.mozilla.org/integration/mozilla-inbound/rev/7ef22534a6cd

Part 6 caused crashtest failures (despite having passed on try earlier in the day!), so I backed parts 6-7 out. I'm working on those failures now.
Comment 27 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-21 02:17:36 PDT
Created attachment 653691 [details] [diff] [review]
Part 8: Make it safe to call MediaInputPort::Destroy after streams a t both ends of the port have been destroyed
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-21 02:18:01 PDT
https://tbpl.mozilla.org/?tree=Try&rev=c979e2573b99
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-22 19:30:09 PDT
I forgot to add [leave open].
Comment 33 :Ehsan Akhgari 2013-06-05 07:11:36 PDT
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.

Note You need to log in before you can comment on or make changes to this bug.