1172394 - Don't recreate the source stream on seek when HTMLMediaElement is captured to a stream

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, task, P3)

Description

[Andreas Pehrson [:pehrsons]]

An apparent issue is this (from bug 1171907):

Seeking in the original HTMLMediaElement ONCE makes the remote video pause. Seeking again makes it play again.

- This is because on seek, MediaDecoder replaces the SourceMediaStream piped into the TrackUnionStream currently backed by the DOMMediaStream we are accessing in JS code. When that happens, the tracks in the underlying StreamBuffer of said TrackUnionStream live on a little longer so the tracks coming from the new SourceMediaStream get assigned different TrackIDs.

- The optimal fix would be to have the MediaDecoder feed us data without recreating the source stream on seek.

Comment 1

[Andreas Pehrson [:pehrsons]]

This is also blocking my DOMMediaStream refactor in case we want to continue with that.

Comment 2

[Andreas Pehrson [:pehrsons]]

JW told me he wanted to take this in a weak moment, thanks JW! ;-)

Comment 3

[Andreas Pehrson [:pehrsons]]

OK, I have finished off most of my other bugs and I want to clear this blocker of bug 1170958, so I'll take it back.

The plan is to make MediaDecoderStateMachine's feeding of the output stream pull based.

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1172394 - Make MediaDecoder's captureStream fully pull based. r=jwwang,kinetik,padenot

Bug 1172394 - Make MediaDecoder's captureStream fully pull based. r=jwwang,kinetik,padenot

Makes the following changes:
* Feeding of DecodedStream through its own MediaQueues.
* Output MediaStream is fed purely through pulling the exact number of
  ticks needed.
* Seeking does not recreate the DecodedStream

Known issues:
* If some but not all tracks end and we seek, the ended tracks will not
  be recreated - they will be hidden.
* WebAudio's MediaElementAudioSourceNode breaks. It want volume and
  muting to propagate to the output stream.

Comment 5

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8631046 [details]
MozReview Request: Bug 1172394 - Make MediaDecoder's captureStream fully pull based. r=jwwang,kinetik,padenot

This is what I propose for resolving my blocker on MediaStream::AddTrack/RemoveTrack, padenot's blocker on audio capture, and to get us closer to the new spec proposal for media element's captureStream (http://w3c.github.io/mediacapture-fromelement).

The big changes are:
- DecodedStream pull based and has its own separate MediaQueues.
-- Can handle any amount of tracks to make way for the new spec proposal (url above) where mutliple audio tracks are possible.
-- No source stream recreation on seek, simple enough now that it's pull based.
- AudioSink does not get shut down
-- There's no stream clock anymore
-- Output still going to speakers (breaks WebAudio MediaElementSourceAudioNode, we'll have to discuss how to solve this)

JW, I included you as DecodedStream peer.
kinetik because I heard you've had thoughts on making DecodedStream pull based.

Any thoughts on the approach taken and ways of solving the remaining issues are welcome. It's not necessary to go too deep as I still have some refactoring to do, especially in making all the time conversion stuff cleaner.

Comment 6

[Andreas Pehrson [:pehrsons]]

I put together some cases and solution ideas in [0] Maire, do you want to check it out before inviting people?

[0] https://etherpad.mozilla.org/capturestream-cases

Comment 7

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8631046 [details]
MozReview Request: Bug 1172394 - Make MediaDecoder's captureStream fully pull based. r=jwwang,kinetik,padenot

Bug 1172394 - Make MediaDecoder's captureStream fully pull based. r=jwwang,kinetik,padenot
Makes the following changes:
* Feeding of DecodedStream through its own MediaQueues.
* Output MediaStream is fed purely through pulling the exact number of
  ticks needed.
* Seeking does not recreate the DecodedStream

Known issues:
* If some but not all tracks end and we seek, the ended tracks will not
  be recreated - they will be hidden.
* WebAudio's MediaElementAudioSourceNode breaks. It want volume and
  muting to propagate to the output stream.

Comment 8

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8631046 [details]
MozReview Request: Bug 1172394 - Make MediaDecoder's captureStream fully pull based. r=jwwang,kinetik,padenot

That was a rebase on top of JW's recent code shuffle patch. Phew!

Comment 9

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #7)
> Known issues:
> * If some but not all tracks end and we seek, the ended tracks will not
>   be recreated - they will be hidden.

This was actually fixed in this patch.

Comment 10

[Maire Reavy [:mreavy]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #6)
> I put together some cases and solution ideas in [0] Maire, do you want to
> check it out before inviting people?
> 
> [0] https://etherpad.mozilla.org/capturestream-cases

Thanks, Andreas.  Your writeup is clear and concise and much appreciated.  

Paul - can you read over Andreas's writeup, in particular his proposed solutions and weigh in on which one you prefer.

NeedInfoing roc and jesup for their awareness and suggestions.  Thanks

Comment 11

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

Seems reasonable, thanks.  I'm tempted by the c++ 'polyfill' idea

Comment 12

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

https://reviewboard.mozilla.org/r/12825/#review11551

::: dom/media/MediaDecoderStateMachine.h:159
(Diff revision 2)
> +  void RecreateDecodedStream(MediaStreamGraph* aGraph);

Not used.

::: dom/media/DecodedStream.h:11
(Diff revision 2)
> +#include "MediaQueue.h"

You don't need to include "MediaQueue.h". Forward declaration will do the job.

::: dom/media/DecodedStream.cpp:40
(Diff revision 2)
> -      mLastOutputTime = mStream->StreamTimeToMicroseconds(mStream->GraphTimeToStreamTime(aCurrentTime));
> +      mDecodedStream->NotifyPull(aGraph, aDesiredTime);

Call back to DecodedStream with lock held is a good candicate for deadlock. Please get rid of bi-directional dependency if you can or ensure the locking order.

::: dom/media/DecodedStream.cpp:84
(Diff revision 2)
> +  aStream->SetPullEnabled(!aBlocking);

Why changing pull-enabled? We should be able to get rid of UpdateStreamBlocking() since the stream  will be blocked automatically if you don't provide enough data in NotifyPull().

::: dom/media/DecodedStream.cpp:98
(Diff revision 2)
> -  mListener = new DecodedStreamGraphListener(mStream);
> +  mListener = new DecodedStreamGraphListener(this);

Move this to initialization list.

::: dom/media/DecodedStream.cpp:176
(Diff revision 2)
> +  return HasTrackData<AudioData>(mAudioTrackBuffers, aID) ||

HasTrackData is able to deduce the parameter type from mAudioTrackBuffers or mVideoTrackBuffers. You don't have to supply <AudioData>

::: dom/media/DecodedStream.cpp:164
(Diff revision 2)
> -  return mListener->GetLastOutputTime();
> +  for (DecodedTrack<Data> data : aTracks) {

Use auto&& data to avoid copy.

::: dom/media/DecodedStream.cpp:247
(Diff revision 2)
> +      allTracksEnded = false;

Break for early return.

::: dom/media/DecodedStream.cpp:245
(Diff revision 2)
> +  for (DecodedTrack<Data> data : aTracks) {

Ditto.

::: dom/media/DecodedStream.cpp:605
(Diff revision 2)
> -      // Remove the finished stream so it won't block the decoded stream.
> +    // Remove the finished stream so it won't block the decoded stream.

Where is DecodedStream::Remove()?

::: dom/media/DecodedStream.cpp:714
(Diff revision 2)
> +    MutexAutoUnlock unlock(mMutex);

This breaks the atomicity of RecreateData(). Have something like DestroyDataInLock().

::: dom/media/MediaDecoderStateMachine.h:412
(Diff revision 2)
> +  MediaQueue<AudioData>& AudioPushToStreamQueue() { return mDecodedStreamAudioQueue; }

I would prefer to move these queues into DecodedStream which is more relevant.

::: dom/media/MediaDecoderStateMachine.cpp:380
(Diff revision 2)
> -  return !mAudioCaptured || mDecodedStream->HaveEnoughAudio(mInfo);
> +  return true;

Why is it OK to just return true?

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 2)
> -      if (mAudioCaptured || VideoQueue().GetSize() == 1) {

Why removing the code?

::: dom/media/MediaDecoderStateMachine.cpp:2187
(Diff revision 2)
> +  AudioPushToStreamQueue().ClearListeners();

Pop listeners are not added at all.

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 2)
> -      self->StopAudioThread();

This is a spec. requirement and should belong to its own bug. Small patches are easier to review.

::: dom/media/DecodedStream.cpp:823
(Diff revision 2)
> -    int64_t silentFrames = frameOffset.value() - audioWrittenOffset.value();
> +    RecreateData();

I think this breaks the intention of this bug since it would like to re-create the source media stream under no condition.

Comment 13

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

Comment on attachment 8631046 [details]
MozReview Request: Bug 1172394 - Make MediaDecoder's captureStream fully pull based. r=jwwang,kinetik,padenot

feedback- for critical issues.

Comment 14

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/12825/#review11665

::: dom/media/DecodedStream.cpp:823
(Diff revision 2)
> -    int64_t silentFrames = frameOffset.value() - audioWrittenOffset.value();
> +    RecreateData();

This is not a regular seek. It's a seek after the media has finished, aka a restart of playback. In that case the tracks have ended or will end soon, and we want to have new tracks added when playback finishes again. The current solution for that is to recreate the source stream.

::: dom/media/DecodedStream.cpp:40
(Diff revision 2)
> -      mLastOutputTime = mStream->StreamTimeToMicroseconds(mStream->GraphTimeToStreamTime(aCurrentTime));
> +      mDecodedStream->NotifyPull(aGraph, aDesiredTime);

It has to be bidirectional because we need to call Forget() on it when shutting down so we don't get a UAF in NotifyPull(). That's also the only call into the listener by DecodedStreamData.

::: dom/media/DecodedStream.cpp:84
(Diff revision 2)
> +  aStream->SetPullEnabled(!aBlocking);

The stream will auto-block when it underruns and pulling is off, but not when it's on. When we take blocking away we can simplify this code but that will also result in less accurate tests (we can't be sure of the stream duration then, but on the other hand it conforms better to the spec) so I want to keep it for now. We can probably remove the explicit blocking calls though.

::: dom/media/DecodedStream.cpp:176
(Diff revision 2)
> +  return HasTrackData<AudioData>(mAudioTrackBuffers, aID) ||

I thought I tried and it didn't, but I'll have a go again. Maybe it was the NotifyPull with two template classes (only one was in a parameter) that failed.

::: dom/media/DecodedStream.cpp:605
(Diff revision 2)
> -      // Remove the finished stream so it won't block the decoded stream.
> +    // Remove the finished stream so it won't block the decoded stream.

This was the only user so I inlined it.

::: dom/media/DecodedStream.cpp:714
(Diff revision 2)
> +    MutexAutoUnlock unlock(mMutex);

Yeah, I have to fix this.

::: dom/media/MediaDecoderStateMachine.h:412
(Diff revision 2)
> +  MediaQueue<AudioData>& AudioPushToStreamQueue() { return mDecodedStreamAudioQueue; }

I don't agree. The state machine is handling pushing to the queues and the lifetime of the decoded stream so it makes sense that it manages these queues as well. If we remove recreations altogether I expect ownership to be transferred to DecodedStream but for now this way is much simpler. When the state machine is made able to handle multiple tracks I expect that this code might change as well.

::: dom/media/MediaDecoderStateMachine.cpp:380
(Diff revision 2)
> -  return !mAudioCaptured || mDecodedStream->HaveEnoughAudio(mInfo);
> +  return true;

Because we're not killing the audio sink when starting capture, and the decoded stream is consuming data at the exact same rate as the audio sink.

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 2)
> -      if (mAudioCaptured || VideoQueue().GetSize() == 1) {

Because the purpose of this code was "Schedule the state machine to send stream data as soon as possible or the VideoQueue() is empty before the Push()."

::: dom/media/MediaDecoderStateMachine.cpp:2187
(Diff revision 2)
> +  AudioPushToStreamQueue().ClearListeners();

Still, shouldn't hurt?

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 2)
> -      self->StopAudioThread();

The goal is to still fulfill the spec (WebAudio MediaElementAudioSourceNode). I can probably do this change and the spec-fix in another patch though.

Comment 15

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

https://reviewboard.mozilla.org/r/12825/#review11669

::: dom/media/DecodedStream.cpp:823
(Diff revision 2)
> -    int64_t silentFrames = frameOffset.value() - audioWrittenOffset.value();
> +    RecreateData();

Play again after reaching the end is indeed a regular seek. So you should never finish a track since it breaks the purpose of this bug.

::: dom/media/DecodedStream.cpp:84
(Diff revision 2)
> +  aStream->SetPullEnabled(!aBlocking);

Can you show me the code where it doesn't block on underrun when pull is enabled?

::: dom/media/DecodedStream.cpp:605
(Diff revision 2)
> -      // Remove the finished stream so it won't block the decoded stream.
> +    // Remove the finished stream so it won't block the decoded stream.

DecodedStream::Remove() is to encapsulate the detail so we can remove the monitor usued in DecodedStream in the future. This inlining breaks the purpose. See bug 1179665 comment 2

::: dom/media/MediaDecoderStateMachine.cpp:380
(Diff revision 2)
> -  return !mAudioCaptured || mDecodedStream->HaveEnoughAudio(mInfo);
> +  return true;

Please add a comment for the reason you mention.

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 2)
> -      if (mAudioCaptured || VideoQueue().GetSize() == 1) {

But you still don't explain why removing the code.

::: dom/media/MediaDecoderStateMachine.cpp:2187
(Diff revision 2)
> +  AudioPushToStreamQueue().ClearListeners();

No. But I don't like unsed code to cause confusion.

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 2)
> -      self->StopAudioThread();

Please do so. Each patch should focus on one issue whenever possible. It is also easier to trace why and how this change is made in the future.

Comment 16

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/12825/#review11675

::: dom/media/DecodedStream.cpp:84
(Diff revision 2)
> +  aStream->SetPullEnabled(!aBlocking);

I don't know where, it's based on an observation of a TrackUnionStream failing because there wasn't enough data provided by its producer.

I can try to see if there's maybe a bug causing this.

::: dom/media/DecodedStream.cpp:823
(Diff revision 2)
> -    int64_t silentFrames = frameOffset.value() - audioWrittenOffset.value();
> +    RecreateData();

Sure, it's a regular seek for the MediaDecoder, but not for the DecodedStream.

Here all tracks have ended (and because of that, the source stream has also finished) and we need to create new ones.

The spec is here: http://w3c.github.io/mediacapture-fromelement/#methods

"A captured MediaStreamTrack ends when the track that it captures ends."

::: dom/media/MediaDecoderStateMachine.cpp:380
(Diff revision 2)
> -  return !mAudioCaptured || mDecodedStream->HaveEnoughAudio(mInfo);
> +  return true;

I'll put in a general comment to how the DecodedStream works now, covering this case.

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 2)
> -      if (mAudioCaptured || VideoQueue().GetSize() == 1) {

I'm not sure I follow what you're looking for here, but simply put:

The reason for having this code is gone, so this code is not needed anymore.

::: dom/media/MediaDecoderStateMachine.cpp:2187
(Diff revision 2)
> +  AudioPushToStreamQueue().ClearListeners();

Fair enough.

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 2)
> -      self->StopAudioThread();

This comment is just to open an issue in reviewboard so I don't forget.

::: dom/media/DecodedStream.cpp:605
(Diff revision 2)
> -      // Remove the finished stream so it won't block the decoded stream.
> +    // Remove the finished stream so it won't block the decoded stream.

Why does it break? I have removed the dependency on the monitor in this patch, and the output streams array is only modified on the main thread.

Comment 17

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

https://reviewboard.mozilla.org/r/12825/#review11695

::: dom/media/DecodedStream.cpp:605
(Diff revision 2)
> -      // Remove the finished stream so it won't block the decoded stream.
> +    // Remove the finished stream so it won't block the decoded stream.

No, you just replace the monitor with a Mutex which doesn't really make a difference. Yes, this code is safe without any lock on the main thread. However, it doesn't still justify expanding DecodedStream::Remove(). We should encapsulate the details whenever possible so it is easier for  DecodedStream to mutate without breaking its clients. This is the principle of least privilege.

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 2)
> -      if (mAudioCaptured || VideoQueue().GetSize() == 1) {

I think you should still keep |VideoQueue().GetSize() == 1| which handles the case where we are slowing in decoding video and running out of video frames. We would like to render the incoming video frame as soon as possible when it comes. The case is not about audio-capture.

::: dom/media/DecodedStream.cpp:823
(Diff revision 2)
> -    int64_t silentFrames = frameOffset.value() - audioWrittenOffset.value();
> +    RecreateData();

This is interesting. If we seek before a track finishes, the track will remain the same in the capture stream. However, if we seek after a track finishes, the capture stream will have to remove the old track since it is finished and create a new one, right? Things would become unpredictable if we seek near the end of the media resource since chances are we are seeking before or after the track finishes.

It is even more confusing with tracks with different duration. When we seek in the middle of playback, chances are some tracks are already finished while others not which makes the tracks in the capture stream unpredictable after seeking.

Comment 18

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/12825/#review11695

> I think you should still keep |VideoQueue().GetSize() == 1| which handles the case where we are slowing in decoding video and running out of video frames. We would like to render the incoming video frame as soon as possible when it comes. The case is not about audio-capture.

Yeah, sorry, I read it as `!mAudioCaptured` but that's clearly wrong.

> No, you just replace the monitor with a Mutex which doesn't really make a difference. Yes, this code is safe without any lock on the main thread. However, it doesn't still justify expanding DecodedStream::Remove(). We should encapsulate the details whenever possible so it is easier for  DecodedStream to mutate without breaking its clients. This is the principle of least privilege.

I thought the monitor came from the state machine, I see now that has recently changed.

I'm not sure I follow how the principle of least privilege applies here when what I do is actually shrinking the API surface. But sure, I'll put `Remove()` back.

Comment 19

[Andreas Pehrson [:pehrsons]]

(In reply to JW Wang [:jwwang] from comment #17)
> https://reviewboard.mozilla.org/r/12825/#review11695
> 
> This is interesting. If we seek before a track finishes, the track will
> remain the same in the capture stream. However, if we seek after a track
> finishes, the capture stream will have to remove the old track since it is
> finished and create a new one, right? Things would become unpredictable if
> we seek near the end of the media resource since chances are we are seeking
> before or after the track finishes.
> 
> It is even more confusing with tracks with different duration. When we seek
> in the middle of playback, chances are some tracks are already finished
> while others not which makes the tracks in the capture stream unpredictable
> after seeking.

Martin, I think JW is right in that this is a confusing part of the spec. Is this a case you have considered?

On its edge I think this example explains the issue at hand quite well:
> var video = document.createElement("video");
> var video2 = document.createElement("video");
> video.src = "some-media-with-10s-audio-and-20s-video.webm";
> video.play();
> video2.srcObject = video.captureStreamUntilEnded();
> video2.play();
> video2.onloadedmetadata = () =>
>   video2.getAudioTracks()[0].onended = () => video.currentTime = 0;

What does `video2` play after the seek? Is it what the user expected?

Comment 20

[Martin Thomson [:mt:]]

I think that we have to regard a seek as being equivalent to a new source[1].  In the example here, we have to consider the loss of audio as unimportant.  I'm sorry if that completely changes the dynamic here.

[1] https://github.com/w3c/mediacapture-fromelement/issues/10

Comment 21

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

Per comment 20, this bug will become invalid since creating a new source media stream is the right way to handle seeking though.

Comment 22

[Andreas Pehrson [:pehrsons]]

Perhaps we can use some bits from the patch here for other bugs though.

Comment 23

[Andreas Pehrson [:pehrsons]]

(In reply to Martin Thomson [:mt:] from comment #20)
> I think that we have to regard a seek as being equivalent to a new
> source[1].  In the example here, we have to consider the loss of audio as
> unimportant.  I'm sorry if that completely changes the dynamic here.
> 
> [1] https://github.com/w3c/mediacapture-fromelement/issues/10

I commented on that issue a while back. Martin, have you seen my comment, and do you have any comments to it?

Comment 24

[Martin Thomson [:mt:]]

Ahh, yes.  That's a difficult one.  I wanted to prepare a pull request for the change, to try it out.  I will do that.  Also, we have a face-to-face next week.  I'll see if there is time on the agenda.

Comment 25

[Andreas Pehrson [:pehrsons]]

With the latest spec update at [1] we need to revive this.

[1] https://w3c.github.io/mediacapture-fromelement/index.html

Comment 26

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Mass change P2 -> P3

Comment 27

[Blake Wu [:bwu][:blakewu]]

Ctai is the best guy to work on this bug! So put it on his plate. :-)

Comment 28

[Andreas Pehrson [:pehrsons]]

This is getting more and more important because
1) We need this to get up to spec and remove the prefix on HTMLMediaElement.mozCaptureStream. Chrome has implemented this api and we are already falling behind.
2) This is the only MediaStream producer we have left that relies on the MediaStreamGraph to create MediaStreamTracks for it. Getting this fixed would mean that we can start cleaning things up and simplify the code.

Comment 29

[Andreas Pehrson [:pehrsons]]

I think it's time to tackle this anew.

Comment 30

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1172394 - Perform some forgotten Stream -> Track renaming in DecodedStream. r?padenot

Comment 31

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1172394 - Don't remove dom::MediaTracks on chained metadata updates. r?padenot

HTMLMediaElement avoid creating new tracks in MetadataLoaded when it has already
created some, so there should be no side effect to this patch.

Depends on D52036

Comment 32

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1172394 - Make dom::MediaTrack lifetime spec compliant. r?bryce

This makes us forget tracks at the right times. The spec also says no
removetrack events should be fired because of this, yet it seems to be something
other user agents do:
https://wpt.fyi/results/media-source/mediasource-avtracks.html

This is of low importance however, since MediaTracks are prefed off by default.

Depends on D52037

Comment 33

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1172394 - Merge MediaStream and MediaDecoder track sources. r?padenot

This reworks how media element captureStream works by removing the differences
between MediaStream and MediaDecoder capture. MediaDecoder capture will be
refactored so that ownership of MediaStreamTracks lies with the media element
instead of the OutputStreamManager. The internal MediaDecoder parts happen in a
later patch.

The new API for capturing a MediaDecoder involves a boolean on/off toggle, the
output tracks the decoder pipes data to, and the principal that data is tagged
with. If capturing is on but there are no output tracks, playback will not
happen, to ensure that no data gets accidentally skipped in the output tracks
while captured.

This also changes the logic for setting up MediaElementTrackSources in
HTMLMediaElement so it's triggered by the WatchManager and thus run in tail
dispatched runnables.

Depends on D52038

Comment 34

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1172394 - Refactor how DecodedStream is set up. r?padenot

This patch removes the responsibility of js-facing MediaStreamTracks from the
MediaDecoder stack, and moves the machinery for setting up DecodedStream to
higher order functions like state mirroring and watchables.

OutputStreamManager is completely gone, since it was designed to manage
MediaStreamTracks across multiple output streams for a single decoder,
on main thread. HTMLMediaElement took over its task in the previous patch.

The MediaDecoderStateMachine now has three control points for capturing:

• mOutputCaptured, which, if true, will capture all decoded data into
  mOutputTracks. If this is set, but mOutputTracks is empty, we are still
  waiting for tracks, and DecodedStream will not play any data. When tracks are
  set, a new DecodedStream is created that will play data through
  SourceMediaTracks piped into mOutputTracks.
• mOutputTracks, which is the set of tracks data is captured into, for
  forwarding to all the output tracks the media element is managing. This set of
  tracks is managed by the MediaDecoder owner, and must contain one audio track
  if the decoder is decoding audio, and one video track if the decoder is
  decoding video. It may be empty since output can be captured before metadata
  is loaded, or playback has ended.
• mOutputPrincipal, which is the principal of the decoded data. All data sent
  into SourceMediaTracks is tagged with this principal.

Depends on D52040

Comment 35

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1172394 - Simplify MediaSink somewhat. r?padenot

This patches does several minor things:

• Moves SetSink (from setSinkid) to automatic coalescing of multiple calls
  through a Canonical/Mirror setup instead of a manual atomic counter.
• Simplifies the logic for when to update the sink in SetSink.
• Removes PlaybackParams as a general MediaSink property, as it only contains
  audio params.
• Makes PlaybackParams an internal AudioSink concept, that AudioSinkWrapper
  knows about.
• Ensures mMediaSink is only accessed on the decoder TaskQueue, to allow
  accessing mirrored members when creating it.

Depends on D52042

Comment 36

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1172394 - Modernize test_streams_element_capture_reset.html. r?jib

Depends on D52043

Comment 37

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1172394 - Update test_streams_element_capture_reset.html per new seeking behavior. r?jib

Depends on D52044

Comment 38

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1172394 - Use tail dispatching instead of mSrcStreamTracksAvailable. r?padenot

Depends on D52045

Comment 39

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1172394 - Hinge UpdateReadyStateInternal off watchables instead of direct updates. r?bryce

Depends on D52046

Comment 40

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1172394 - Ignore video tracks in autoplay checks in MediaStreamAudioSourceNode. r?alwu

Depends on D52047

Comment 41

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1172394 - Always mark a MediaStreamAudioSourceNode attached to a live track as active. r?padenot

Depends on D52048

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/afb4b226ff04
Perform some forgotten Stream -> Track renaming in DecodedStream. r=padenot
https://hg.mozilla.org/integration/autoland/rev/744fb77a5833
Don't remove dom::MediaTracks on chained metadata updates. r=padenot
https://hg.mozilla.org/integration/autoland/rev/89ad0b553b0f
Make dom::MediaTrack lifetime spec compliant. r=bryce
https://hg.mozilla.org/integration/autoland/rev/a796541fe5ef
Merge MediaStream and MediaDecoder track sources. r=padenot
https://hg.mozilla.org/integration/autoland/rev/c57c41e8c39e
Refactor how DecodedStream is set up. r=padenot
https://hg.mozilla.org/integration/autoland/rev/1c45b135318d
Simplify MediaSink somewhat. r=padenot
https://hg.mozilla.org/integration/autoland/rev/c3bd415507e8
Modernize test_streams_element_capture_reset.html. r=jib
https://hg.mozilla.org/integration/autoland/rev/7e7baa73e1ef
Update test_streams_element_capture_reset.html per new seeking behavior. r=jib
https://hg.mozilla.org/integration/autoland/rev/94bd21d9b396
Use tail dispatching instead of mSrcStreamTracksAvailable. r=padenot
https://hg.mozilla.org/integration/autoland/rev/edff95b6f724
Hinge UpdateReadyStateInternal off watchables instead of direct updates. r=bryce
https://hg.mozilla.org/integration/autoland/rev/d0aa43657e8c
Ignore video tracks in autoplay checks in MediaStreamAudioSourceNode. r=alwu
https://hg.mozilla.org/integration/autoland/rev/a4f256e68cef
Always mark a MediaStreamAudioSourceNode attached to a live track as active. r=padenot

Comment 43

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on browser_disabledForMediaStreamVideos.js (+ Bug 1546756 - Bug 1302379 - Bug 1500049)

backout: https://hg.mozilla.org/integration/autoland/rev/7272d77d4e808dcbbd1f4f50210786dd326b218a

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=browser-chrome&revision=355f090421a6b38618b3f2e2e0c08d9166a78e72

• permafails on subsequent pushes too, but it's moving chunks

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=276082854&repo=autoland&lineNumber=5211

[task 2019-11-13T22:24:00.091Z] 22:24:00 INFO - TEST-START | toolkit/components/pictureinpicture/tests/browser_disabledForMediaStreamVideos.js
[task 2019-11-13T22:25:30.112Z] 22:25:30 INFO - TEST-INFO | started process screenshot
[task 2019-11-13T22:25:30.393Z] 22:25:30 INFO - TEST-INFO | screenshot: exit 0
[task 2019-11-13T22:25:30.393Z] 22:25:30 INFO - Buffered messages logged at 22:24:00
[task 2019-11-13T22:25:30.393Z] 22:25:30 INFO - Entering test bound test_disabledForMediaStreamVideos
[task 2019-11-13T22:25:30.393Z] 22:25:30 INFO - Buffered messages finished
[task 2019-11-13T22:25:30.394Z] 22:25:30 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/pictureinpicture/tests/browser_disabledForMediaStreamVideos.js | Test timed out -
[task 2019-11-13T22:25:30.394Z] 22:25:30 INFO - GECKO(3016) | MEMORY STAT | vsize 19406245MB | vsizeMaxContiguous 64994388MB | residentFast 992MB
[task 2019-11-13T22:25:30.394Z] 22:25:30 INFO - TEST-OK | toolkit/components/pictureinpicture/tests/browser_disabledForMediaStreamVideos.js | took 90127ms
[task 2019-11-13T22:25:30.394Z] 22:25:30 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-11-13T22:25:30.394Z] 22:25:30 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/pictureinpicture/tests/browser_disabledForMediaStreamVideos.js | Found a tab after previous test timed out: http://example.com/browser/toolkit/components/pictureinpicture/tests/test-page.html -
[task 2019-11-13T22:25:30.394Z] 22:25:30 INFO - checking window state

Comment 44

[Andreas Pehrson [:pehrsons]]

Thanks. I have a pernosco recording of this timeout at https://pernos.co/debug/u5SU1FR4G6Qb4PJeXE7UeA/index.html.

Comment 45

[Andreas Pehrson [:pehrsons]]

Ah, captureStream is busted for looping media elements. Even for seeking ones actually. Shortly after seeking back to the beginning, capture stops.

Track lifetimes are ok (existing dom/media tests), so a media element playing a stream from another media element that seeked still end as expected. But it loses the direct video listener between the SourceMediaTrack and the ForwardedInputTrack, so video stops flowing for the media element with the MediaStream. In this particular test case, we do a mozCaptureStream() typically in the second loop, so that video track will never see a first frame, and the media element doesn't resolve its play promises.

The culprit is here. It worked before, when we didn't support consecutively switching the inputs of a live ForwardedInputTrack.

Comment 46

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1172394 - Forward direct listeners to all inputs over a ForwardedInputTrack's lifetime. r?padenot

This was not needed when inputs to a ForwardedInputTrack could not come and go,
but they can now. This keeps direct listeners for a ForwardedInputTrack around
until it ends, as that's the terminal state where we know we can no longer
process another input.

Depends on D52049

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b466cf155f9
Perform some forgotten Stream -> Track renaming in DecodedStream. r=padenot
https://hg.mozilla.org/integration/autoland/rev/e20ee1ce7ce5
Don't remove dom::MediaTracks on chained metadata updates. r=padenot
https://hg.mozilla.org/integration/autoland/rev/8e45c51fc6cf
Make dom::MediaTrack lifetime spec compliant. r=bryce
https://hg.mozilla.org/integration/autoland/rev/9114318b6493
Merge MediaStream and MediaDecoder track sources. r=padenot
https://hg.mozilla.org/integration/autoland/rev/fdf5dd8ff807
Refactor how DecodedStream is set up. r=padenot
https://hg.mozilla.org/integration/autoland/rev/b91bd828c657
Simplify MediaSink somewhat. r=padenot
https://hg.mozilla.org/integration/autoland/rev/bff56fd97a88
Modernize test_streams_element_capture_reset.html. r=jib
https://hg.mozilla.org/integration/autoland/rev/9d3f25494445
Update test_streams_element_capture_reset.html per new seeking behavior. r=jib
https://hg.mozilla.org/integration/autoland/rev/4f27806289e2
Use tail dispatching instead of mSrcStreamTracksAvailable. r=padenot
https://hg.mozilla.org/integration/autoland/rev/6bcd10fe43b9
Hinge UpdateReadyStateInternal off watchables instead of direct updates. r=bryce
https://hg.mozilla.org/integration/autoland/rev/2d4bb8556f08
Ignore video tracks in autoplay checks in MediaStreamAudioSourceNode. r=alwu
https://hg.mozilla.org/integration/autoland/rev/ae3cd36f38f6
Always mark a MediaStreamAudioSourceNode attached to a live track as active. r=padenot
https://hg.mozilla.org/integration/autoland/rev/2516219862e8
Forward direct listeners to all inputs over a ForwardedInputTrack's lifetime. r=padenot

Comment 48

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/5b466cf155f9
https://hg.mozilla.org/mozilla-central/rev/e20ee1ce7ce5
https://hg.mozilla.org/mozilla-central/rev/8e45c51fc6cf
https://hg.mozilla.org/mozilla-central/rev/9114318b6493
https://hg.mozilla.org/mozilla-central/rev/fdf5dd8ff807
https://hg.mozilla.org/mozilla-central/rev/b91bd828c657
https://hg.mozilla.org/mozilla-central/rev/bff56fd97a88
https://hg.mozilla.org/mozilla-central/rev/9d3f25494445
https://hg.mozilla.org/mozilla-central/rev/4f27806289e2
https://hg.mozilla.org/mozilla-central/rev/6bcd10fe43b9
https://hg.mozilla.org/mozilla-central/rev/2d4bb8556f08
https://hg.mozilla.org/mozilla-central/rev/ae3cd36f38f6
https://hg.mozilla.org/mozilla-central/rev/2516219862e8