Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Description • 8 years ago

STR:
1. Open URL and hit [Record!]

Expected result:
  recording for 3 seconds...
  Successfully recorded 857025 byte blob
  Playing video/webm recording.

Actual result:
  recording for 3 seconds...
  Successfully recorded 0 byte blob
  Playing recording.
  MEDIA_ERR_SRC_NOT_SUPPORTED

Workaround:
1. Uncomment line 2 (interim recording step, means src is a blob, not a direct gum stream. Works).

	Andreas Pehrson [:pehrsons] Assignee
	Comment 1 • 8 years ago

I did some debugging with bug 1208371 applied. Same issue.

It must be that the TrackUnionStream has all its tracks added in the MSG before the DOMMediaStream gets its PlaybackStreamListener added to the TrackUnionStream on the main thread.

I.e., we never get here: https://dxr.mozilla.org/mozilla-central/rev/63be002b4a803df1122823841ef7633b7561d873/dom/media/TrackUnionStream.cpp#121

	Andreas Pehrson [:pehrsons] Assignee
	Comment 2 • 8 years ago

In the short term we can make the MSG call back as soon as a listener is added and there are already tracks in the StreamBuffer.

In the long term we should be able to kill the OnTracksAvailableCallback. Main thread state of tracks should be enough.

	Chia-hung Tai [:ctai :ctai_mozilla :cht]
	Comment 3 • 8 years ago

What pehrson means is:
1. MediaRecorder::Session uses TracksAvailableCallback which inherited OnTracksAvailableCallback in |Start|
See https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaRecorder.cpp#294 and
https://dxr.mozilla.org/mozilla-central/source/dom/media/DOMMediaStream.h#64
2. |TrackUnionStream::ProcessInput| call |MediaStreamListener::NotifyFinishedTrackCreation| after the track added into this TrackUnionStream. But the MediaStreamListener is not added in this iteration. In another word, the DOMMediaStream::PlaybackStreamListener is added later then the track added into TrackUnionStream.
3. So the |DOMMediaStream::NotifyTracksCreated| will not be called. It means |MediaRecorder::Session::TracksAvailableCallback::NotifyTracksAvailable| will not be executed. So the MediaRecorder thinks there is no track in this MediaStream and error happened.

	Chia-hung Tai [:ctai :ctai_mozilla :cht]
	Comment 4 • 8 years ago

The short term solution is adding MediaStreamListener::NotifyFinishedTrackCreation in |MediaStream::AddListenerImpl| when there is a MediaStream::mBuffer is not empty.
See https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaStreamGraph.cpp#2311

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #2)
> In the short term we can make the MSG call back as soon as a listener is
> added and there are already tracks in the StreamBuffer.
> 
> In the long term we should be able to kill the OnTracksAvailableCallback.
> Main thread state of tracks should be enough.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 5 • 8 years ago

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #3)
> What pehrson means is:
> 1. MediaRecorder::Session uses TracksAvailableCallback which inherited
> OnTracksAvailableCallback in |Start|
> See
> https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaRecorder.
> cpp#294 and
> https://dxr.mozilla.org/mozilla-central/source/dom/media/DOMMediaStream.h#64
> 2. |TrackUnionStream::ProcessInput| call
> |MediaStreamListener::NotifyFinishedTrackCreation| after the track added
> into this TrackUnionStream. But the MediaStreamListener is not added in this
> iteration. In another word, the DOMMediaStream::PlaybackStreamListener is
> added later then the track added into TrackUnionStream.
> 3. So the |DOMMediaStream::NotifyTracksCreated| will not be called. It means
> |MediaRecorder::Session::TracksAvailableCallback::NotifyTracksAvailable|
> will not be executed. So the MediaRecorder thinks there is no track in this
> MediaStream and error happened.

Exactly. And I think that MediaRecorder doesn't need TracksAvailableCallback to work. We should:
1. Clarify in the spec how multiple tracks and changes to the track set should be handled. Should we use the tracks available on Start() (and throw if there are none), or should we allow dynamic changes to the track set, have MediaRecorder accept tracks instead of a stream, or something else?

2. Remove TracksAvailableCallback. Just set up the listeners on main thread state directly. Simplest would be to have a MediaStreamTrackListener on each track, but if we allow dynamic changes to the track set we should perhaps go for a MediaStreamListener instead.

	Chia-hung Tai [:ctai :ctai_mozilla :cht]
	Comment 6 • 8 years ago

YES, MediaRecorder doesn't need TracksAvailableCallback.
But that should be a long term solution and it related to the confirmation of the spec.

We should take the short term solution first and file another bug for the long term solution.
jesup, HDYT?

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #5)
> Exactly. And I think that MediaRecorder doesn't need TracksAvailableCallback
> to work. We should:
> 1. Clarify in the spec how multiple tracks and changes to the track set
> should be handled. Should we use the tracks available on Start() (and throw
> if there are none), or should we allow dynamic changes to the track set,
> have MediaRecorder accept tracks instead of a stream, or something else?
> 
> 2. Remove TracksAvailableCallback. Just set up the listeners on main thread
> state directly. Simplest would be to have a MediaStreamTrackListener on each
> track, but if we allow dynamic changes to the track set we should perhaps go
> for a MediaStreamListener instead.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 7 • 8 years ago

Having looked at this code recently I think this is because we don't really support video.mozCaptureStream when the src is a MediaStream.

We'll return a stream but it'll be empty, see [1]. In the fiddle we should be able to see that no MediaStreamTracks are available.

The obvious work around is to grab the MediaStream that is acting src directly. It's problematic if its wrapped in a URL but from an application's point of view it is possible.


[1] https://dxr.mozilla.org/mozilla-central/rev/8d0aadfe7da782d415363880008b4ca027686137/dom/html/HTMLMediaElement.cpp#2041

	Andreas Pehrson [:pehrsons] Assignee
	Comment 8 • 8 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 9 • 8 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 10 • 8 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 11 • 8 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 12 • 8 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 13 • 8 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 14 • 8 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 15 • 8 years ago

This prepares HTMLMediaElement for having a separate MediaStreamTrackSource for
MediaStreams, StreamCaptureTrackSource.

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 16 • 8 years ago

This adds support for HTMLMediaElement.mozCaptureStream() and
mozCaptureStreamUntilEnded() for a HTMLMediaElement playing a MediaStream.

This is up to spec, while capturing a HTMLMediaElement playing a file is not.
This incompatibility means we cannot mix sources for the returned MediaStream.

As such, a MediaStream returned while the HTMLMediaElement was playing a file
will only have content while the element is playing files. If the src changes
to a MediaStream, the stream will be empty.

It works the same way if a MediaStream was captured while the HTMLMediaElement
was playing another MediaStream.

This is due to TrackID management - MediaDecoder doesn't care, and creates new
tracks when you seek, so users are unable to keep track, while for MediaStream
we control everything from main thread and keep track of the TrackIDs used
previously.

This also adds a separate path from MediaElementAudioSourceNode so that we don't
forward video tracks when the returned MediaStream is only used internally for
WebAudio. We should in that case not require a DOMMediaStream but just forwarding
tracks to a TrackUnionStream should be enough, and will save us some cpu cycles.
This is however fine for now as it's simpler.

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 17 • 8 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 18 • 8 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 19 • 8 years ago

Comment on attachment 8778258 [details]
Bug 1259788 - Add audio content test for captureStream of MediaElement playing a MediaStream.

https://reviewboard.mozilla.org/r/69572/#review66760

Nice test.

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_audio.html:30
(Diff revision 1)
> +    captureStream = gUMAudioElement.mozCaptureStream();
> +
> +    analyser = new AudioStreamAnalyser(audioContext, captureStream);

Nit: captureStream doesn't need to be global, or exist.

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_audio.html:59
(Diff revision 1)
> +    gUMAudioElement.srcObject = null;
> +
> +    return analyser.waitForAnalysisSuccess(array =>
> +      array[analyser.binIndexForFrequency(50)]    < 50 &&
> +      array[analyser.binIndexForFrequency(TEST_AUDIO_FREQ)]  < 50 &&
> +      array[analyser.binIndexForFrequency(2500)]  < 50);
> +  })
> +  .then(() => getUserMedia({audio: true}))

This calls gUM a second time without having stopped the previous gUM track (gUM concurrency). Is that on purpose?

If it's important that it be a new source, then you may want to stop the old track first (or you could reuse the track even).

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_audio.html:71
(Diff revision 1)
> +  .then(() => new Promise(r => gUMAudioElement.onloadedmetadata = r))
> +  .then(() => {
> +    gUMAudioElement.play();

Here you wait for loadedmetadata before calling play(), whereas in the test above you didn't. Is this different on purpose? Why not: gUMAudioElement.autoplay = true; ?

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_audio.html:87
(Diff revision 1)
> +    let oscOut = audioContext.createMediaStreamDestination();
> +    oscillator.connect(oscOut);
> +
> +    gUMAudioElement.srcObject.addTrack(oscOut.stream.getTracks()[0]);

Before combining the tracks, would it be interesting to also test setting

    gUMAudioElement.srcObject = oscOut.stream;

?

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 20 • 8 years ago

https://reviewboard.mozilla.org/r/69572/#review66786

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_audio.html:19
(Diff revision 1)
> +  title: "Test CaptureStream audio content on HTMLMediaElement playing a gUM MediaStream",
> +  visible: true
> +});
> +
> +var audioContext;
> +var gUMAudioElement;

gUMAudioElement could be initialized here, and made const.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 21 • 8 years ago

https://reviewboard.mozilla.org/r/69574/#review66784

Looks good, just misc nits.

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_video.html:19
(Diff revision 1)
> +var gUMAudioElement;
> +var captureStream;
> +var captureStreamElement;

gUMAudioElement and captureStreamElement could be initialized here, and made const.

It should also be gUMVideoElement...

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_video.html:29
(Diff revision 1)
> +var h;
> +
> +var checkVideoPlaying = video =>
> +  h.waitForPixel(video, offsetX, offsetY,

Nit: could h be initialized here (and made const), so readers don't have to guess what h is based on use?

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_video.html:31
(Diff revision 1)
> +var checkVideoPlaying = video =>
> +  h.waitForPixel(video, offsetX, offsetY,
> +                 px => (px[3] = 255, h.isPixelNot(px, h.blackTransparent, threshold)))
> +    .then(() => {

Nit: (Promoting my indent-style in lieu of official code style guidance:) What if we indent line overflow by 4 and brackets by 2?

    var checkVideoPlaying = video =>
        h.waitForPixel(video, offsetX, offsetY,
                       px => (px[3] = 255, h.isPixelNot(px, h.blackTransparent, threshold)))
      .then(() => {
        // ...
      })

Produces less "bracket-gap" (indentation-creep on following lines), and seems more consistent with your indent-to-open-paren use below.

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_video.html:35
(Diff revision 1)
> +      let startPixel = { data: h.getPixel(video, offsetX, offsetY)
> +                       , name: "startcolor"
> +                       };

Then I'll overlook this... ;)

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_video.html:38
(Diff revision 1)
> +      return h.waitForPixel(video, offsetX, offsetY,
> +                            //px => h.isPixelNot(px, startPixel, threshold));
> +                            px => {
> +                              let result = h.isPixelNot(px, startPixel, threshold);
> +                              info("Checking playing, " + Array.slice(px) + " vs " +
> +                                   Array.slice(startPixel.data) +
> +                                   ". Pass=" + result);
> +                              return result;
> +                            });

Remove either commented-out code or info debug cruft.

If the former, why not:

    return h.waitForPixel(video, offsetX, offsetY, px => {
      info("Checking playing, " + Array.slice(px) + " vs " +
           Array.slice(startPixel.data) + ". Pass=" + result);
      return h.isPixelNot(px, startPixel, threshold);
    });

?

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_video.html:49
(Diff revision 1)
> +var checkVideoPaused =
> +  // First check that we have a non-black frame. We should freeze on last frame
> +  // on pause.
> +  video => h.waitForPixel(video, offsetX, offsetY,

Style thought: you're already passing in one global here, why not two?

    var checkVideoPaused = (video, ms) => ...

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_video.html:77
(Diff revision 1)
> +    captureStream = gUMVideoElement.mozCaptureStream();
> +    captureStreamElement = createMediaElement("video", "captureStream");
> +    captureStreamElement.srcObject = captureStream;

captureStream doesn't need to be global, or exist.

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_video.html:82
(Diff revision 1)
> +    captureStream = gUMVideoElement.mozCaptureStream();
> +    captureStreamElement = createMediaElement("video", "captureStream");
> +    captureStreamElement.srcObject = captureStream;
> +    captureStreamElement.play();
> +
> +    h = new CaptureStreamTestHelper2D(50, 50);

How about adding:

    h.isOpaquePixelNot = (px, col, thr) => (px[3] = 255, h.isPixelNot(px, col, thr));

?

Some re-use value, and would save a comment or two.

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_video.html:104
(Diff revision 1)
> +  .then(() => getUserMedia({video: true, fake: true}))
> +  .then(stream => {

Again maybe stop previous track unless gUM concurrency is desired.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 22 • 8 years ago

Comment on attachment 8778259 [details]
Bug 1259788 - Add video content test for captureStream of MediaElement playing a MediaStream.

https://reviewboard.mozilla.org/r/69574/#review66894

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 23 • 8 years ago

https://reviewboard.mozilla.org/r/69572/#review66896

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_audio.html:4
(Diff revision 1)
> +  <title>Test media element CaptureStream audio contents with MediaStream</title>
> +  <script type="application/javascript" src="mediaStreamPlayback.js"></script>
> +  <script type="application/javascript" src="head.js"></script>
> +</head>
> +<body>
> +<pre id="test">
> +<script>
> +
> +createHTML({
> +  bug: "1259788",
> +  title: "Test CaptureStream audio content on HTMLMediaElement playing a gUM MediaStream",

Don't need both <title> and createHTML().

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 24 • 8 years ago

https://reviewboard.mozilla.org/r/69574/#review66898

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_video.html:4
(Diff revision 1)
> +  <title>Test media element CaptureStream with MediaStream</title>
> +  <script type="application/javascript" src="/tests/dom/canvas/test/captureStream_common.js"></script>
> +  <script type="application/javascript" src="mediaStreamPlayback.js"></script>
> +  <script type="application/javascript" src="head.js"></script>
> +</head>
> +<body>
> +<pre id="test">
> +<script>
> +
> +createHTML({
> +  bug: "1259788",
> +  title: "Test CaptureStream video content on HTMLMediaElement playing a gUM MediaStream",

Don't need both <title> and createHTML().

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 25 • 8 years ago

Comment on attachment 8778260 [details]
Bug 1259788 - Add test for output MediaStreamTracks when media element captures MediaStream.

https://reviewboard.mozilla.org/r/69576/#review66900

One of the haveEvents is messed up, otherwise neat test! Pardon all my comments again.

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:4
(Diff revision 1)
> +  <title>Test media element CaptureStream with MediaStream</title>
> +  <script type="application/javascript" src="mediaStreamPlayback.js"></script>
> +  <script type="application/javascript" src="head.js"></script>
> +</head>
> +<body>
> +<pre id="test">
> +<script>
> +
> +createHTML({
> +  bug: "1259788",
> +  title: "Test CaptureStream track output on HTMLMediaElement playing a gUM MediaStream",

Don't need both <title> and createHTML().

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:18
(Diff revision 1)
> +var createElement = (name, id) => {
> +  let elem = document.createElement(name);
> +  elem.id = id;
> +  elem.width = 150;
> +  elem.height = 100;
> +  document.getElementById("content").appendChild(elem);
> +  return elem;
> +};

How about:

    var createElement = (name, id) =>
      Object.assign(document.getElementById("content")
                            .appendChild(document.createElement(name)),
                    {id, width: 150, height: 100});

?

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:27
(Diff revision 1)
> +var audioElement;
> +var audioCaptureStream;
> +var videoElement;
> +var videoCaptureStream;
> +var untilEndedElement;
> +var streamUntilEnded;

Same comment as on the other tests about using const and initializing elements early. Treating page setup as invariant helps when there are so many other variables for a reader to keep track of IMHO.

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:56
(Diff revision 1)
> +    videoCaptureStream = videoElement.mozCaptureStream();
> +    info("Capturing video element");
> +
> +    return new Promise(r => videoElement.onloadedmetadata = r);
> +  })
> +  .then(() => {
> +    is(videoCaptureStream.getAudioTracks().length,
> +       videoElement.srcObject.getAudioTracks().length,
> +       "video element should capture all audio tracks");

Just curious, what would happen if we .mozCaptureStream()ed after loadedmetadata? Would immediately calling .getAudioTracks() still work?

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:69
(Diff revision 1)
> +  .then(() => getUserMedia({audio: true, video: true})).then(stream => {
> +    info("Testing CaptureStreamUntilEnded");

Again would love to see us stopping tracks to avoid gUM concurrency. I know default devices don't care today, but I have patches in bug 1088621 that make them more like real devices in that they have different code paths for concurrency, which might restrict what they output (not here, but in general if constraints were used). Also, why call gUM again here in the first place?

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:97
(Diff revision 1)
> +    is(streamUntilEnded.getAudioTracks().filter(t => t.readyState == "live")
> +                                        .length, 0,
> +       "video element should not capture any audio tracks after ending");
> +    is(streamUntilEnded.getVideoTracks().filter(t => t.readyState == "live")
> +                                        .length, 0,
> +       "video element should not capture any video tracks after ending");

Looks like these can be combined to one is() using getTracks()?

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:106
(Diff revision 1)
> +    return Promise.all([
> +      haveEvent(audioCaptureStream, "addtrack",
> +                wait(5000, "No addtrack event")),
> +      haveEvent(audioCaptureStream, "addtrack",
> +                wait(5000, "No second addtrack event")),
> +      haveEvent(audioCaptureStream, "addtrack", wait(0))
> +        .then(() => Promise.reject(new Error("Too many tracks added")),
> +              () => Promise.resolve())

I think all three haveEvent() calls execute at the same time here, registering listeners, so I suspect they all resolve at the same time after just a single "addtrack" event is fired, which is probably not what you meant.

I see below that you do these in sequence which makes more sense.

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:114
(Diff revision 1)
> +    ].concat(audioCaptureStream.getTracks()
> +                               .filter(t => t.readyState == "live")
> +                               .map(t => new Promise(r => t.onended = r))));

Just me: concat reads funny here ("there's more!"). How about the spread operator:

      ...audioCaptureStream.getTracks()
                           .filter(t => t.readyState == "live")
                           .map(t => new Promise(r => t.onended = r))
    ]);

?

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:119
(Diff revision 1)
> +    ].concat(audioCaptureStream.getTracks()
> +                               .filter(t => t.readyState == "live")
> +                               .map(t => new Promise(r => t.onended = r))));
> +  })
> +  .then(() => {
> +    info("Resetting video element source");

Maybe also mention what we're testing next?

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:134
(Diff revision 1)
> +    return haveEvent(videoCaptureStream, "addtrack",
> +                     wait(5000, "No addtrack event"));
> +  })
> +  .then(() => haveEvent(videoCaptureStream, "addtrack",
> +                        wait(5000, "No second addtrack event")))
> +  .then(() => haveEvent(videoCaptureStream, "addtrack", wait(0))
> +    .then(() => Promise.reject(new Error("Too many tracks added")),
> +          () => Promise.resolve()))

I see the spec [1] guarantees that we queue a task for each event fired, so assuming we follow that in our implementation, this should be ok.

[1] https://w3c.github.io/mediacapture-main/getusermedia.html#stream-remove-track

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 26 • 8 years ago

Comment on attachment 8778262 [details]
Bug 1259788 - Make AudioStreamAnalyser (test-only) work with dynamically added tracks.

https://reviewboard.mozilla.org/r/69580/#review66914

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 27 • 8 years ago

Comment on attachment 8778263 [details]
Bug 1259788 - Fix AudioStreamAnalyser debug canvas drawing.

https://reviewboard.mozilla.org/r/69582/#review66916

	Andreas Pehrson [:pehrsons] Assignee
	Comment 28 • 8 years ago

From IRC friday:
> [16:18:47] <pehrsons> jesup: so my discussion point is from the comment of the big patch, how we should interact with media file capturing
> [16:19:32] <pehrsons> jesup: as the patches stand you can't capture a file, then switch to a stream as source and expect the output to continue
> [16:19:49] <pehrsons> I *can* fix it but it's a bit ugly
> [16:20:26] <pehrsons> The reason is that MediaDecoder capture just interacts with a SourceMediaStream, but MediaStream capture interacts with the main thread objects
> [16:20:32] <pehrsons> TrackID management, etc.
> [16:21:40] <pehrsons> So I could fix it by having an intermediate MediaStreamTrack for each captured MediaStreamTrack before they get added to the captureStream output
> [16:22:45] <pehrsons> so now we have MST->captureStream, but I could make it MST->MST->captureStream and it'd work
> [16:23:16] <pehrsons> And a single MST like that intermediate step will create three new TrackUnionStreams, which is unnecessary because we don't expose this thing to script
> [16:24:14] <pehrsons> Best solution is to refactor MediaDecoder capture to the same method of creating tracks, and to follow the spec
> [16:24:46] <pehrsons> once it follows the spec all track changes from MediaDecoder are triggered by main thread so we won't really need the whole dynamic track creation requirement we have today


Thoughts on this Randell?
Ugly workaround for interop with MediaDecoder sources OK?
Or, we can live with MediaDecoder and MediaStream sources being treated separately because mozCaptureStream prefixed and MediaDecoder capture not up to spec?

	Andreas Pehrson [:pehrsons] Assignee
	Comment 29 • 8 years ago
mozreview-review-reply

Comment on attachment 8778258 [details]
Bug 1259788 - Add audio content test for captureStream of MediaElement playing a MediaStream.

https://reviewboard.mozilla.org/r/69572/#review66760

> This calls gUM a second time without having stopped the previous gUM track (gUM concurrency). Is that on purpose?
> 
> If it's important that it be a new source, then you may want to stop the old track first (or you could reuse the track even).

I think I wanted an all-new source that the element hadn't seen. Shouldn't matter though (or need to be tested here) as the element has been reset. Simplifying.

> Here you wait for loadedmetadata before calling play(), whereas in the test above you didn't. Is this different on purpose? Why not: gUMAudioElement.autoplay = true; ?

No, shouldn't matter. I had this and the MediaElementCapture_tracks.html test joint in the beginning so likely fallout from that.

And it turned out the element already has autoplay="autoplay" set through the createMediaElement() wrapper.

> Before combining the tracks, would it be interesting to also test setting
> 
>     gUMAudioElement.srcObject = oscOut.stream;
> 
> ?

No, I think it's fine. Different sources should just work (we have tests for that for playout, and capture is simpler as it just forwards tracks). What I want to test is that adding a track to the source is picked up in the capture. I should perhaps test that removing one works as well.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 30 • 8 years ago
mozreview-review-reply

Comment on attachment 8778258 [details]
Bug 1259788 - Add audio content test for captureStream of MediaElement playing a MediaStream.

https://reviewboard.mozilla.org/r/69572/#review66786

> gUMAudioElement could be initialized here, and made const.

No :(

Because in head.js, function createMediaElement:
>   document.getElementById('content').appendChild(element);

	Andreas Pehrson [:pehrsons] Assignee
	Comment 31 • 8 years ago
mozreview-review-reply

Comment on attachment 8778258 [details]
Bug 1259788 - Add audio content test for captureStream of MediaElement playing a MediaStream.

https://reviewboard.mozilla.org/r/69572/#review66760

> No, I think it's fine. Different sources should just work (we have tests for that for playout, and capture is simpler as it just forwards tracks). What I want to test is that adding a track to the source is picked up in the capture. I should perhaps test that removing one works as well.

And that revealed a bug, hah!

	Andreas Pehrson [:pehrsons] Assignee
	Comment 32 • 8 years ago
mozreview-review-reply

Comment on attachment 8778259 [details]
Bug 1259788 - Add video content test for captureStream of MediaElement playing a MediaStream.

https://reviewboard.mozilla.org/r/69574/#review66784

> gUMAudioElement and captureStreamElement could be initialized here, and made const.
> 
> It should also be gUMVideoElement...

Uhm, yes, copy-paste for president. But can't define the element here since the content div doesn't exist yet.

> Nit: could h be initialized here (and made const), so readers don't have to guess what h is based on use?

It can!

> Nit: (Promoting my indent-style in lieu of official code style guidance:) What if we indent line overflow by 4 and brackets by 2?
> 
>     var checkVideoPlaying = video =>
>         h.waitForPixel(video, offsetX, offsetY,
>                        px => (px[3] = 255, h.isPixelNot(px, h.blackTransparent, threshold)))
>       .then(() => {
>         // ...
>       })
> 
> Produces less "bracket-gap" (indentation-creep on following lines), and seems more consistent with your indent-to-open-paren use below.

SGTM

> Remove either commented-out code or info debug cruft.
> 
> If the former, why not:
> 
>     return h.waitForPixel(video, offsetX, offsetY, px => {
>       info("Checking playing, " + Array.slice(px) + " vs " +
>            Array.slice(startPixel.data) + ". Pass=" + result);
>       return h.isPixelNot(px, startPixel, threshold);
>     });
> 
> ?

I think there's good value in the logs and will remove the comment. Though, did you see the `". Pass=" + result`?

> How about adding:
> 
>     h.isOpaquePixelNot = (px, col, thr) => (px[3] = 255, h.isPixelNot(px, col, thr));
> 
> ?
> 
> Some re-use value, and would save a comment or two.

Added it to captureStream_common.js

> Again maybe stop previous track unless gUM concurrency is desired.

I'll reuse.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 33 • 8 years ago
mozreview-review-reply

Comment on attachment 8778260 [details]
Bug 1259788 - Add test for output MediaStreamTracks when media element captures MediaStream.

https://reviewboard.mozilla.org/r/69576/#review66900

> How about:
> 
>     var createElement = (name, id) =>
>       Object.assign(document.getElementById("content")
>                             .appendChild(document.createElement(name)),
>                     {id, width: 150, height: 100});
> 
> ?

I'll reuse createMediaElement from head.js instead.

> Same comment as on the other tests about using const and initializing elements early. Treating page setup as invariant helps when there are so many other variables for a reader to keep track of IMHO.

Agreed, but as mentioned setting them depends on scripts having finished loading and we doing DOM updates. Async stuff living in the testing framework.

> Just curious, what would happen if we .mozCaptureStream()ed after loadedmetadata? Would immediately calling .getAudioTracks() still work?

It would work yes, though the spec is not explicit about it.

> Again would love to see us stopping tracks to avoid gUM concurrency. I know default devices don't care today, but I have patches in bug 1088621 that make them more like real devices in that they have different code paths for concurrency, which might restrict what they output (not here, but in general if constraints were used). Also, why call gUM again here in the first place?

I'll fix so there's no concurrency.

	Randell Jesup [:jesup] (needinfo me)
	Comment 46 • 8 years ago

> Ugly workaround for interop with MediaDecoder sources OK?
> Or, we can live with MediaDecoder and MediaStream sources being treated
> separately because mozCaptureStream prefixed and MediaDecoder capture not up
> to spec?

I think they can land as-is, and file follow-up bugs, especially due to the spec issue

	Randell Jesup [:jesup] (needinfo me)
	Comment 47 • 8 years ago
mozreview-review

Comment on attachment 8778264 [details]
Bug 1259788 - Allow MediaStreamTrack::ForwardTrackContentsTo to take an explicit TrackID.

https://reviewboard.mozilla.org/r/69584/#review67332

	Randell Jesup [:jesup] (needinfo me)
	Comment 48 • 8 years ago
mozreview-review

Comment on attachment 8778261 [details]
Bug 1259788 - Multi-track support for MediaTrackList.

https://reviewboard.mozilla.org/r/69578/#review67334

::: dom/html/HTMLMediaElement.h:347
(Diff revision 2)
>     * whether it's appropriate to fire an error event.
>     */
>    void NotifyLoadError();
>  
> +  /**
> +   *

Comment?

::: dom/html/HTMLMediaElement.h:352
(Diff revision 2)
> +   *
> +   */
>    void NotifyMediaTrackEnabled(MediaTrack* aTrack);
>  
>    /**
> +   *

Comment?

::: dom/html/HTMLMediaElement.cpp:1283
(Diff revision 2)
> +             (!aTrack->AsVideoTrack() || !aTrack->AsVideoTrack()->Selected()));
> +
> +
> +  if (aTrack->AsAudioTrack()) {
> +    bool shouldMute = true;
> +    for (size_t i = 0; i < AudioTracks()->Length(); ++i) {

Not really important, but is this Length() size_t?

	Randell Jesup [:jesup] (needinfo me)
	Comment 49 • 8 years ago
mozreview-review

Comment on attachment 8778265 [details]
Bug 1259788 - Rename CaptureStreamTrackSource to DecoderCaptureTrackSource.

https://reviewboard.mozilla.org/r/69586/#review67346

	Randell Jesup [:jesup] (needinfo me)
	Comment 50 • 8 years ago
mozreview-review

Comment on attachment 8778266 [details]
Bug 1259788 - Support MediaStream sources for HTMLMediaElement.mozCaptureStream().

https://reviewboard.mozilla.org/r/69588/#review67358

::: dom/html/HTMLMediaElement.cpp:1319
(Diff revision 2)
> +  for (OutputMediaStream& ms : mOutputStreams) {
> +    if (ms.mCapturingDecoder) {
> +      MOZ_ASSERT(!ms.mCapturingMediaStream);
> +      continue;
> +    }
> +    for (int32_t i = ms.mTrackPorts.Length() - 1; i >= 0; --i) {

int32 is probably good, though not quite pedantically correct

	Randell Jesup [:jesup] (needinfo me)
	Comment 51 • 8 years ago
mozreview-review

Comment on attachment 8778267 [details]
Bug 1259788 - Support MediaStream sources for HTMLMediaElement.mozCaptureStream().

https://reviewboard.mozilla.org/r/69590/#review67420

::: dom/media/DOMMediaStream.h:520
(Diff revision 2)
>     * are carrying that track.
>     *
>     * Creates a MediaStreamTrack, adds it to mTracks, raises "addtrack" and
>     * returns it.
>     *
> +   * XXX update the below doc

update?

	Randell Jesup [:jesup] (needinfo me)
	Comment 52 • 8 years ago
mozreview-review

Comment on attachment 8778268 [details]
Bug 1259788 - Add a new disabled mode for MSG tracks.

https://reviewboard.mozilla.org/r/69592/#review67424

	Randell Jesup [:jesup] (needinfo me)
	Comment 53 • 8 years ago
mozreview-review

Comment on attachment 8778960 [details]
Bug 1259788 - MediaTrack should notify HTMLMediaElement when track is removed (for captureStream).

https://reviewboard.mozilla.org/r/70044/#review67426

	Andreas Pehrson [:pehrsons] Assignee
	Comment 54 • 8 years ago
mozreview-review-reply

Comment on attachment 8778261 [details]
Bug 1259788 - Multi-track support for MediaTrackList.

https://reviewboard.mozilla.org/r/69578/#review67334

> Not really important, but is this Length() size_t?

Oh, no this one is uint32_t.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 55 • 8 years ago
mozreview-review-reply

Comment on attachment 8778267 [details]
Bug 1259788 - Support MediaStream sources for HTMLMediaElement.mozCaptureStream().

https://reviewboard.mozilla.org/r/69590/#review67420

> update?

I had a second read of the spec and will remove this patch completely.

It reads:

> When the User Agent initiates adding a track to a MediaStream, with the exception of initializing a newly created MediaStream with tracks, the User Agent MUST queue a task that runs the following steps:
> 1. Let *track* be the MediaStreamTrack in question and *stream* the MediaStream object to which *track* is to be added.
> 2. Add *track* to *stream*.
> 3. If the operation in the previous step was aborted prematurely, then abort these steps.
> 4. Fire a track event named addtrack with *track* at *stream*.

So I'll instead of raising "addtrack" async, run CreateDOMTrack() async when we have to create a MediaStreamTrack dynamically.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 56 • 8 years ago
mozreview-review-reply

Comment on attachment 8778260 [details]
Bug 1259788 - Add test for output MediaStreamTracks when media element captures MediaStream.

https://reviewboard.mozilla.org/r/69576/#review66900

> It would work yes, though the spec is not explicit about it.

I have to revise this.

"loadedmetadata" then mozCaptureStream will work.

However, mozCaptureStream and then waiting for "loadedmetadata" doesn't guarantee that tracks exist. They're added in the same task as in which we raise "addtrack" on the stream. See comment 55.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 70 • 8 years ago

Comment on attachment 8778260 [details]
Bug 1259788 - Add test for output MediaStreamTracks when media element captures MediaStream.

I need another look on this jib. Cheers.

I still need to fix so that all tracks are stopped before the test ends, but I'll do that after I have gotten r+ and rebased so it can be tested properly.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 71 • 8 years ago

Comment on attachment 8778267 [details]
Bug 1259788 - Support MediaStream sources for HTMLMediaElement.mozCaptureStream().

A second look at this would be great as well. It's a bit messy and I'm sorry for that.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 72 • 8 years ago
mozreview-review

Comment on attachment 8781997 [details]
Bug 1259788 - Create haveEvents() and friends in head.js.

https://reviewboard.mozilla.org/r/72234/#review69926

::: dom/media/tests/mochitest/head.js:614
(Diff revision 1)
> +function haveEvents(target, name, count, cancelPromise) {
> +  var listener;
> +  return Promise.race([
> +    (cancelPromise || new Promise()).then(e => Promise.reject(e)),
> +    new Promise(resolve =>
> +        target.addEventListener(name, listener = () => ((!--count) && resolve())))

How about (--count < 1 && resolve()) ?

Then count is optional, i.e. haveEvents(target, name) and haveEvents(target, name, 1) work the same. Seems useful/expected.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 73 • 8 years ago
mozreview-review-reply

Comment on attachment 8781997 [details]
Bug 1259788 - Create haveEvents() and friends in head.js.

https://reviewboard.mozilla.org/r/72234/#review69926

> How about (--count < 1 && resolve()) ?
> 
> Then count is optional, i.e. haveEvents(target, name) and haveEvents(target, name, 1) work the same. Seems useful/expected.

And then var haveEvent = (target, name, cancelPromise) => hasEvents(target, name, 1, cancelPromise); ?

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 74 • 8 years ago
mozreview-review

Comment on attachment 8778260 [details]
Bug 1259788 - Add test for output MediaStreamTracks when media element captures MediaStream.

https://reviewboard.mozilla.org/r/69576/#review69932

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:28
(Diff revisions 1 - 3)
>  runTest(() => getUserMedia({audio: true, video: true})
>    .then(stream => {
>      // We need to test with multiple tracks. We add an extra of each kind.
>      stream.getTracks().forEach(t => stream.addTrack(t.clone()));
>  
> -    audioElement = createElement("audio", "gUMCaptureAudioOnly");
> +    audioElement = createMediaElement("gUMAudio", "local", "gUMAudio", true);

(Unrelated to this patch, but) IMHO createMediaElement [1] would have been better taking a type string instead of a boolean arg:

    createMediaElement("audio", "gUMAudio", "local", "gUMAudio");

Then we could even allow streamId to be left out to reuse the label:

    createMediaElement("audio", "gUMAudio", "local");

[1] https://dxr.mozilla.org/mozilla-central/rev/fe895421dfbe1f1f8f1fc6a39bb20774423a6d74/dom/media/tests/mochitest/head.js#243

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:45
(Diff revisions 1 - 3)
>      is(audioCaptureStream.getVideoTracks().length, 0,
>         "audio element should not capture any video tracks");
>  
> -    videoElement = createElement("video", "gUMCapture");
> -    videoElement.srcObject = audioElement.srcObject;
> -    videoElement.play();
> +    return haveEvent(audioCaptureStream, "addtrack", wait(0))
> +        .then(() => Promise.reject(new Error("Too many tracks added")),
> +              () => Promise.resolve());

Nit: or just: () => {}, to fit on the same line.

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:50
(Diff revisions 1 - 3)
> -    videoElement.play();
> +              () => Promise.resolve());
> +  })
> +  .then(() => {
> +    videoElement = createMediaElement("gUMVideo", "local", "gUMVideo", false);
> +
> +    info("Capturing video element (captureStream -> loadedmetadata)");

I assume also testing "Capturing video element (loadedmetadata -> captureStream)" and "Capturing audio element (captureStream -> loadedmetadata)" would be redundant?

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:57
(Diff revisions 1 - 3)
> -    return new Promise(r => videoElement.onloadedmetadata = r);
> +    return haveEvents(videoCaptureStream, "addtrack", 3,
> +                     wait(5000, new Error("No event")));

I see you don't fail on a 4th event here, though you do elsewhere.

Might a haveEventsButNoMore() helper be useful to capture that repeating pattern?

Nit: indent on second line is off by one 's'.

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:96
(Diff revisions 1 - 3)
> +  .then(() => haveEvent(videoCaptureStream, "addtrack", wait(0))
> +      .then(() => Promise.reject(new Error("Too many tracks added")),
> +            () => Promise.resolve()))

It's tempting to hide this repeating pattern in a haveNoEvent helper...

E.g.:

    var haveEventsButNoMore = (...args) => haveEvents(...args)
      .then(() => haveNoEvent(...args));

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:133
(Diff revisions 1 - 3)
>      videoElement.srcObject = stream;
> -    videoElement.play();
> +    return Promise.all(videoCaptureStream.getTracks()
> -
> -    return Promise.all(
> -      videoCaptureStream.getTracks()
> -                        .filter(t => t.readyState == "live")
> +        .filter(t => t.readyState == "live")
> -                        .map(t => new Promise(r => t.onended = r))
> +        .map(t => haveEvent(t, "ended", wait(5000, new Error("No event")))));
> -        .concat(new Promise(r => videoElement.onloadedmetadata = r)));
>    })
> +  .then(() => haveEvents(videoCaptureStream, "addtrack", 2,
> +                        wait(5000, new Error("No event"))))

We're waiting upto 5 seconds for all "ended" events to fire here. Is it possible for "addtrack" to fire before then? If so, is there a potential intermittent here?

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:141
(Diff revisions 1 - 3)
>      is(videoCaptureStream.getAudioTracks()
> -                         .filter(t => t.readyState == "live")
> -                         .length,
> +        .filter(t => t.readyState == "ended").length, 3,
> +       "Video element should have three ended audio tracks");

s/Video element/Captured stream/

(3 more below)

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_tracks.html:171
(Diff revisions 1 - 3)
> +    untilEndedElement.srcObject.getTracks().forEach(t => t.stop());
> +    // We stop the stream to make the media element end.
> +    untilEndedElement.srcObject.stop();

Unsure what this comment implies. Does stream.stop() accomplish something here that stream.getTracks().forEach(t => t.stop()) doesn't? I hope not, since the former is going away.

	Randell Jesup [:jesup] (needinfo me)
	Comment 75 • 8 years ago
mozreview-review

Comment on attachment 8781998 [details]
Bug 1259788 - Fix MediaStreamTrack logging.

https://reviewboard.mozilla.org/r/72236/#review70110

	Randell Jesup [:jesup] (needinfo me)
	Comment 76 • 8 years ago
mozreview-review

Comment on attachment 8781999 [details]
Bug 1259788 - Break out AddTrackInternal() from DOMMediaStream::CreateDOMTrack.

https://reviewboard.mozilla.org/r/72238/#review70112

	Andreas Pehrson [:pehrsons] Assignee
	Comment 77 • 8 years ago
mozreview-review-reply

Comment on attachment 8781997 [details]
Bug 1259788 - Create haveEvents() and friends in head.js.

https://reviewboard.mozilla.org/r/72234/#review69926

> And then var haveEvent = (target, name, cancelPromise) => hasEvents(target, name, 1, cancelPromise); ?

Hmm, you sure about that?

> -> --undefined < 1
> <- false
> 
> -> !--undefined
> <- true


Then haveEvent() returns a promise that resolves to the actual event raised. I could implement haveEvents to resolve to the last event raised perhaps, to be able to merge them.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 78 • 8 years ago
mozreview-review-reply

Comment on attachment 8778260 [details]
Bug 1259788 - Add test for output MediaStreamTracks when media element captures MediaStream.

https://reviewboard.mozilla.org/r/69576/#review69932

> I assume also testing "Capturing video element (loadedmetadata -> captureStream)" and "Capturing audio element (captureStream -> loadedmetadata)" would be redundant?

Yeah, same mechanism, just with some track filtering for audio elements.

> We're waiting upto 5 seconds for all "ended" events to fire here. Is it possible for "addtrack" to fire before then? If so, is there a potential intermittent here?

No, all existing tracks end before new ones appear. If this becomes intermittent we'll have to fix it.

> Unsure what this comment implies. Does stream.stop() accomplish something here that stream.getTracks().forEach(t => t.stop()) doesn't? I hope not, since the former is going away.

Yes, it makes media elements end. We need bug 1208316 for full spec compliance here.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 95 • 8 years ago

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a57e0a4a868

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 96 • 8 years ago
mozreview-review-reply

Comment on attachment 8778260 [details]
Bug 1259788 - Add test for output MediaStreamTracks when media element captures MediaStream.

https://reviewboard.mozilla.org/r/69576/#review69932

> Yes, it makes media elements end. We need bug 1208316 for full spec compliance here.

Maybe add 'TODO' and that bug number to this comment?

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 97 • 8 years ago
mozreview-review

Comment on attachment 8781997 [details]
Bug 1259788 - Create haveEvents() and friends in head.js.

https://reviewboard.mozilla.org/r/72234/#review70668

Love it!

::: dom/media/tests/mochitest/head.js:653
(Diff revisions 1 - 2)
> +  let e = new Error("Too many " + name + " events");
> +  return haveEvent(target, name, timeoutPromise || wait(0))
> +    .then(() => Promise.reject(e), () => {});

Nit: `e` could be inlined to avoid being created.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 98 • 8 years ago
mozreview-review-reply

Comment on attachment 8781997 [details]
Bug 1259788 - Create haveEvents() and friends in head.js.

https://reviewboard.mozilla.org/r/72234/#review70668

> Nit: `e` could be inlined to avoid being created.

What does that do to the stack trace though??

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 99 • 8 years ago
mozreview-review-reply

Comment on attachment 8781997 [details]
Bug 1259788 - Create haveEvents() and friends in head.js.

https://reviewboard.mozilla.org/r/72234/#review69926

> Hmm, you sure about that?
> 
> > -> --undefined < 1
> > <- false
> > 
> > -> !--undefined
> > <- true
> 
> 
> Then haveEvent() returns a promise that resolves to the actual event raised. I could implement haveEvents to resolve to the last event raised perhaps, to be able to merge them.

Ah, !--undefined is true, because --undefined == NaN... WAT! How about:

    var listener;
    var counter = count || 1;
    return Promise.race([
      (cancelPromise || new Promise()).then(e => Promise.reject(e)),
      new Promise(resolve =>
         target.addEventListener(name, listener = () => --count || resolve()))
?

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 101 • 8 years ago
mozreview-review-reply

Comment on attachment 8781997 [details]
Bug 1259788 - Create haveEvents() and friends in head.js.

https://reviewboard.mozilla.org/r/72234/#review69926

> Ah, !--undefined is true, because --undefined == NaN... WAT! How about:
> 
>     var listener;
>     var counter = count || 1;
>     return Promise.race([
>       (cancelPromise || new Promise()).then(e => Promise.reject(e)),
>       new Promise(resolve =>
>          target.addEventListener(name, listener = () => --count || resolve()))
> ?

s/--count/--counter/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 102 • 8 years ago
mozreview-review-reply

Comment on attachment 8781997 [details]
Bug 1259788 - Create haveEvents() and friends in head.js.

https://reviewboard.mozilla.org/r/72234/#review70668

> What does that do to the stack trace though??

We have async stacktraces now, at least in Nightly and Dev edition.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 119 • 8 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d2361bb298c

	Randell Jesup [:jesup] (needinfo me)
	Comment 120 • 8 years ago
mozreview-review

Comment on attachment 8782859 [details]
Bug 1259788 - Raise "addtrack" in separate tasks.

https://reviewboard.mozilla.org/r/72892/#review71072

	Randell Jesup [:jesup] (needinfo me)
	Comment 121 • 8 years ago
mozreview-review

Comment on attachment 8782860 [details]
Bug 1259788 - Use RefPtr<MediaStream> in NewRunnableMethod() in DOMMediaStream.

https://reviewboard.mozilla.org/r/72894/#review71074

	Randell Jesup [:jesup] (needinfo me)
	Comment 122 • 8 years ago
mozreview-review

Comment on attachment 8782861 [details]
Bug 1259788 - Ensure ready state is updated when first track added after NotifyTracksAvailable().

https://reviewboard.mozilla.org/r/72896/#review71076

	Randell Jesup [:jesup] (needinfo me)
	Comment 141 • 8 years ago
mozreview-review

Comment on attachment 8784021 [details]
Bug 1259788 - Implement disabled track modes for direct track listeners.

https://reviewboard.mozilla.org/r/73620/#review71446

wow, is all this a pain... r+

	Randell Jesup [:jesup] (needinfo me)
	Comment 142 • 8 years ago
mozreview-review

Comment on attachment 8784022 [details]
Bug 1259788 - Remove direct track listeners from TrackUnionStream before inputs are removed.

https://reviewboard.mozilla.org/r/73622/#review71448

	Andreas Pehrson [:pehrsons] Assignee
	Comment 157 • 8 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=640466a8c685
and latest rev with a fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=906858cc9dab

	Pulsebot
	Comment 158 • 8 years ago

Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/38cf33e16b19
Create haveEvents() and friends in head.js. r=jib
https://hg.mozilla.org/integration/autoland/rev/b933c2445489
Add audio content test for captureStream of MediaElement playing a MediaStream. r=jib
https://hg.mozilla.org/integration/autoland/rev/49854a4729fc
Add video content test for captureStream of MediaElement playing a MediaStream. r=jib
https://hg.mozilla.org/integration/autoland/rev/922aa0f34d14
Add test for output MediaStreamTracks when media element captures MediaStream. r=jib
https://hg.mozilla.org/integration/autoland/rev/31407bd36633
Multi-track support for MediaTrackList. r=jesup
https://hg.mozilla.org/integration/autoland/rev/2ed8b9eae324
Make AudioStreamAnalyser (test-only) work with dynamically added tracks. r=jib
https://hg.mozilla.org/integration/autoland/rev/fa6864d843d5
Fix AudioStreamAnalyser debug canvas drawing. r=jib
https://hg.mozilla.org/integration/autoland/rev/ea78adb873ee
Fix MediaStreamTrack logging. r=jesup
https://hg.mozilla.org/integration/autoland/rev/c12a426a665a
Allow MediaStreamTrack::ForwardTrackContentsTo to take an explicit TrackID. r=jesup
https://hg.mozilla.org/integration/autoland/rev/d63f814b204d
Rename CaptureStreamTrackSource to DecoderCaptureTrackSource. r=jesup
https://hg.mozilla.org/integration/autoland/rev/7b797b5a2da3
Break out AddTrackInternal() from DOMMediaStream::CreateDOMTrack. r=jesup
https://hg.mozilla.org/integration/autoland/rev/da81522029f7
Support MediaStream sources for HTMLMediaElement.mozCaptureStream(). r=jesup
https://hg.mozilla.org/integration/autoland/rev/e8c6e5a386d9
Add a new disabled mode for MSG tracks. r=jesup
https://hg.mozilla.org/integration/autoland/rev/39a8d2d98940
Raise "addtrack" in separate tasks. r=jesup
https://hg.mozilla.org/integration/autoland/rev/33f8f7b476bc
Use RefPtr<MediaStream> in NewRunnableMethod() in DOMMediaStream. r=jesup
https://hg.mozilla.org/integration/autoland/rev/e558f877e09a
Ensure ready state is updated when first track added after NotifyTracksAvailable(). r=jesup
https://hg.mozilla.org/integration/autoland/rev/a41eb891431c
Implement disabled track modes for direct track listeners. r=jesup
https://hg.mozilla.org/integration/autoland/rev/5bd27fdb7140
Remove direct track listeners from TrackUnionStream before inputs are removed. r=jesup

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 159 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/38cf33e16b19
https://hg.mozilla.org/mozilla-central/rev/b933c2445489
https://hg.mozilla.org/mozilla-central/rev/49854a4729fc
https://hg.mozilla.org/mozilla-central/rev/922aa0f34d14
https://hg.mozilla.org/mozilla-central/rev/31407bd36633
https://hg.mozilla.org/mozilla-central/rev/2ed8b9eae324
https://hg.mozilla.org/mozilla-central/rev/fa6864d843d5
https://hg.mozilla.org/mozilla-central/rev/ea78adb873ee
https://hg.mozilla.org/mozilla-central/rev/c12a426a665a
https://hg.mozilla.org/mozilla-central/rev/d63f814b204d
https://hg.mozilla.org/mozilla-central/rev/7b797b5a2da3
https://hg.mozilla.org/mozilla-central/rev/da81522029f7
https://hg.mozilla.org/mozilla-central/rev/e8c6e5a386d9
https://hg.mozilla.org/mozilla-central/rev/39a8d2d98940
https://hg.mozilla.org/mozilla-central/rev/33f8f7b476bc
https://hg.mozilla.org/mozilla-central/rev/e558f877e09a
https://hg.mozilla.org/mozilla-central/rev/a41eb891431c
https://hg.mozilla.org/mozilla-central/rev/5bd27fdb7140

	Andreas Pehrson [:pehrsons] Assignee
	Comment 160 • 8 years ago
dev-doc-info

This deserves some dev-docs.

Note that this is implemented per the spec from HTMLMediaElement.captureStream for MediaStream sources only (note that we don't have captureStream yet, for now it's still mozCaptureStream), [1].

The output from HTMLMediaElement.mozCaptureStream() when the element is playing a non-MediaStream source is incompatible with the output of when the element is playing a MediaStream.

That is, mozCaptureStream() with a non-MediaStream source will return a stream which works like it used to. Switching the source to a MediaStream will result in no MediaStreamTracks being added to this stream. Switching the source back to a non-MediaStream will again add MediaStreamTracks to the stream.

Likewise, mozCaptureStream() with a MediaStream source will return a MediaStream that only contains tracks while the element is playing MediaStreams.

mozCaptureStream() with no source will lock on to the first type of source added.

This special behaviour will disappear once the non-MediaStream cases are up to spec and we have an unprefixed mozCaptureStream.


[1] https://w3c.github.io/mediacapture-fromelement/#methods

	Eric Shepherd [:sheppy]
	Comment 161 • 8 years ago

This turned into a partial revamp of the entire doc set for MediaRecorder before I got a hold of myself and set up a separate issue in our doc work tracking system for that, so bear with me here...

Docs updated:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/captureStream
https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/captureStream (to add some links and make some minor improvements based on new experience with streams)
https://developer.mozilla.org/en-US/Firefox/Releases/51#DOM_HTML_DOM
https://developer.mozilla.org/en-US/docs/Web/API/MediaRecorder
https://developer.mozilla.org/en-US/docs/Web/API/MediaRecorder/MediaRecorder

New pages:

https://developer.mozilla.org/en-US/docs/Web/API/MediaStream_Recording_API
https://developer.mozilla.org/en-US/docs/Web/API/MediaStream_Recording_API/Recording_a_media_element

A couple of older pages have been moved or redirected to the ones above as well.

Marking as docs complete; please let me know if you see issues!