Kaiido Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160814184416

Steps to reproduce:

Create a gUM stream (the bug also applies for canvas streams) 'vStream', with audio and video constraints.
Remove the audio tracks from vStream
Create an audio stream 'aStream',
Call vStream.addTrack(aStream.getAudioTracks()[0]) to add the audio stream to the video one
use MediaRecorder(vStream)

live fiddle : https://jsfiddle.net/twzhwfwk/
tested on 49 and 51 osX versions


Actual results:

MediaRecorder does record vStream without aStream's track, and even keeps vStream audioTracks.
Logging recorder's stream's and vStream's tracks correctly outputs the new tracks from aStream as part of vStream.



Expected results:

MediaRecorder should have recorded vStream with aStream's tracks.

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

This is actually a regression from bug 1257318, in which we changed to using a direct stream listener on the passed-in stream's internal input stream, instead of the regular MediaInputPort between the internal playback stream and the MediaRecorder::Session::mTrackUnionStream which would have a regular stream listener.


addTrack() landed in 44 and bug 1257318 in 48 (and uplifted to 47). I bet ESR 45 works.

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

Hmm, addTrack landed in 44 but we didn't have per-track ports in MediaRecorder until bug 1208371 (i.e., 48). As such this isn't a regression after all.

Still, let's move MediaRecorder to direct track listeners instead of fiddling with ports and internal streams.

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

Suspending MediaRecorder must be broken since we moved to direct listeners.

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

Note that a workaround for this bug from 51 onwards is to route your tracks through a HTMLMediaElement and do mozCaptureStream() on it. That stream can be fed to MediaRecorder (which will still only lock onto the first audio and video track as available, and will raise an error and stop the recording if the track set changes).

	Kaiido Reporter
	Comment 5 • 7 years ago

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #4)
> Note that a workaround for this bug from 51 onwards is to route your tracks
> through a HTMLMediaElement and do mozCaptureStream() on it. That stream can
> be fed to MediaRecorder (which will still only lock onto the first audio and
> video track as available, and will raise an error and stop the recording if
> the track set changes).

Working around the bug is not an issue, new MediaRecorder([video_track, audio_track]) does work.  

Also, one could even workaround the specs limitations of changing the sources of a recording stream by recording a canvas and a WebAudioAPI stream, and feeding it with different sources while the recording is live. 

But still, addTrack and removeTrack don't work, and nothing tells so, apart the MediaRecorder result.

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

(In reply to Kaiido from comment #5)
> Working around the bug is not an issue, new MediaRecorder([video_track,
> audio_track]) does work.
Ahh, right. Because a constructed MediaStream doesn't take a direct listener either. Sorry, internal stuff.


> Also, one could even workaround the specs limitations of changing the
> sources of a recording stream by recording a canvas and a WebAudioAPI
> stream, and feeding it with different sources while the recording is live. 
Sure, though that again depends on a workaround to this issue or you wouldn't be able to record those tracks together.

	Julien Cristau [:jcristau]
	Comment 7 • 7 years ago

Mass wontfix for bugs affecting firefox 52.

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

Yay! Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d2a85decee4130482451269bd28a5b9cf10ef35

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 49 • 7 years ago
mozreview-review

Comment on attachment 8871057 [details]
Bug 1296531 - Remove deprecated MediaRecorder test.

https://reviewboard.mozilla.org/r/142292/#review146210

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 50 • 7 years ago
mozreview-review

Comment on attachment 8871058 [details]
Bug 1296531 - Add error checks to test_mediarecorder_avoid_recursion.html.

https://reviewboard.mozilla.org/r/142294/#review146212

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 51 • 7 years ago
mozreview-review

Comment on attachment 8871075 [details]
Bug 1296531 - Break out ShutdownTicket and GetShutdownBarrier from MSG to MediaUtils.

https://reviewboard.mozilla.org/r/142328/#review146218

::: dom/media/MediaStreamGraph.cpp:3532
(Diff revision 1)
> -          RefPtr<MediaStreamGraphImpl::ShutdownTicket> ticket =
> -              new MediaStreamGraphImpl::ShutdownTicket(gMediaStreamGraphShutdownBlocker.get());
> +          RefPtr<media::ShutdownTicket> ticket =
> +              new media::ShutdownTicket(gMediaStreamGraphShutdownBlocker.get());

How about:

    auto ticket =
        MakeRefPtr<media::ShutdownTicket>(gMediaStreamGraphShutdownBlocker.get());

?

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 52 • 7 years ago
mozreview-review

Comment on attachment 8871050 [details]
Bug 1296531 - Add mochitest for recording a MediaStream with an extra added track.

https://reviewboard.mozilla.org/r/142278/#review146236

Can we rewrite this test using async functions? E.g. like https://jsfiddle.net/jib1/pkc16k9r/

I think that would make the sequence of events clearer, make sure we're not missing any events, and bail on fail.

::: dom/media/test/test_mediarecorder_record_addtracked_stream.html:42
(Diff revision 1)
> +mediaRecorder.onstart = () => {
> +  info("onstart fired");
> +
> +  is(mediaRecorder.state, "recording",
> +     "Media recorder is recording before being stopped");
> +  mediaRecorder.stop();
> +  is(mediaRecorder.state, "inactive",
> +     "Media recorder is inactive after being stopped");
> +  is(mediaRecorder.stream, stream,
> +     "Media recorder stream = constructed stream post recording");
> +};

E.g. this tests that everything is correct *if* onstart fires, except not whether onstart actually fires.

::: dom/media/test/test_mediarecorder_record_addtracked_stream.html:109
(Diff revision 1)
> +  helper.waitForPixelColor(video, helper.red, 128, "Should become red")
> +    .then(() => finish());

This checks the video, should we check the audio as well?

	JW Wang [:jwwang] [:jw_wang]
	Comment 53 • 7 years ago
mozreview-review

Comment on attachment 8871059 [details]
Bug 1296531 - Log error first in test_mr_unsupported_src.html.

https://reviewboard.mozilla.org/r/142296/#review146282

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

Comment on attachment 8871050 [details]
Bug 1296531 - Add mochitest for recording a MediaStream with an extra added track.

https://reviewboard.mozilla.org/r/142278/#review146236

> E.g. this tests that everything is correct *if* onstart fires, except not whether onstart actually fires.

The failure mode for "start" not firing is a test timeout. Logs will reveal that it was "start" we were waiting for.

> This checks the video, should we check the audio as well?

The biggest hurdle with that is that in mediarecorder tests we don't have access to the gUM tests' `head.js` where the audio testing helpers are. I'll see how easily they can be incorporated.

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

Comment on attachment 8871048 [details]
Bug 1296531 - Implement MediaSegment move constructor.

https://reviewboard.mozilla.org/r/142274/#review148338

::: dom/media/MediaSegment.h:429
(Diff revision 1)
> +#ifdef MOZILLA_INTERNAL_API
> +    , mTimeStamp(Move(aSegment.mTimeStamp))
> +#endif
> +  {}

Do we need the INTERNAL_API stuff here now?

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

Comment on attachment 8871049 [details]
Bug 1296531 - Allow MediaSegment::AppendSlice to combine with last chunk.

https://reviewboard.mozilla.org/r/142276/#review148344

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

Comment on attachment 8871051 [details]
Bug 1296531 - Rip out direct stream listeners from MediaRecorder.

https://reviewboard.mozilla.org/r/142280/#review148354

So long as we don't end up with the same problems that led to us adding them... :-)

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

Comment on attachment 8871053 [details]
Bug 1296531 - Expose GraphRate through DOMMediaStream.

https://reviewboard.mozilla.org/r/142284/#review148360

::: dom/media/DOMMediaStream.cpp:848
(Diff revision 2)
> +  if (mPlaybackStream) { return mPlaybackStream->GraphRate(); }
> +  if (mOwnedStream) { return mOwnedStream->GraphRate(); }
> +  if (mInputStream) { return mInputStream->GraphRate(); }

violates style, though I'm not sure I care

	Andreas Pehrson [:pehrsons] Assignee
	Comment 99 • 7 years ago
mozreview-review-reply

Comment on attachment 8871051 [details]
Bug 1296531 - Rip out direct stream listeners from MediaRecorder.

https://reviewboard.mozilla.org/r/142280/#review148354

We'll still be using direct track listeners so there's no change in functionality.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 100 • 7 years ago
mozreview-review

Comment on attachment 8872364 [details]
Bug 1296531 - Let waitForAnalysisSuccess take a cancelPromise.

https://reviewboard.mozilla.org/r/143846/#review148464

::: dom/media/tests/mochitest/head.js:125
(Diff revision 1)
>     * Return a Promise, that will be resolved when the function passed as
>     * argument, when called, returns true (meaning the analysis was a
>     * success).
>     *
>     * @param {function} analysisFunction
> -   *        A fonction that performs an analysis, and returns true if the
> +   *        A function that performs an analysis, and returns true if the

s/returns/resolves with/

::: dom/media/tests/mochitest/head.js:130
(Diff revision 1)
> -  waitForAnalysisSuccess: function(analysisFunction) {
> -    var self = this;
> -    return new Promise((resolve, reject) => {
> -      function analysisLoop() {
> -        var success = analysisFunction(self.getByteFrequencyData());
> +  waitForAnalysisSuccess: function(analysisFunction, cancelPromise) {
> +    let self = this;
> +    let rejected = false;
> +    return Promise.race([
> +      (cancelPromise || new Promise(() => {})).then(e => (rejected = true, Promise.reject(e))),
> +      new Promise((resolve, reject) => {
> +        let analysisLoop = () => {
> +          if (rejected) {
> +            return;

Thanks for touching this.

I'm going to save myself some brain cycles and ask for this to be rewritten with async/await though.

It's a veritable poster child for it.

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

Comment on attachment 8872365 [details]
Bug 1296531 - Let waitForPixel and friends take a cancelPromise.

https://reviewboard.mozilla.org/r/143848/#review148474

::: dom/canvas/test/captureStream_common.js:138
(Diff revision 1)
> +          if (!rejected) {
> -        resolve(pixelMatch);
> +            resolve(pixelMatch);

This test is redundant, since it's harmless to call resolve() after reject() has been called, and vice versa.

::: dom/canvas/test/captureStream_common.js:178
(Diff revision 1)
> -    return this.waitForPixel(video, 0, 0,
> +    return this.waitForPixel(video, /*offsetX = */0, /*offsetY = */0,
> -                             px => this.isPixel(px, refColor, threshold),
> +        px => this.isPixel(px, refColor, threshold),
> -                             timeout)
> +        timeout, /*width = */0, /*height = */0, cancelPromise)

I'm not a fan of this /* embedded */ comment solution.

Maybe we should fix the problem function instead to take an options object instead of all those args? 

With es6 destructuring this gives us named arguments essentially:

    let result = await this.waitForPixel(video, {
      offsetX: 0, offsetY: 0,
      callback: px => this.isPixel(px, refColor, threshold),
      timeout, width: 0, height: 0, cancelPromise
    });
    ok(!result, video.id + " " + infoString);

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

Comment on attachment 8871050 [details]
Bug 1296531 - Add mochitest for recording a MediaStream with an extra added track.

https://reviewboard.mozilla.org/r/142278/#review148516

Nice! Just some nits.

::: dom/media/test/test_mediarecorder_record_addtracked_stream.html:130
(Diff revisions 1 - 2)
> +         + upperFreq + ": " + upperAmp);
> +    return lowerAmp < 50 && freqAmp > 200 && upperAmp < 50;
> +  }, endedNoError.then(() => new Error("Audio check failed")));
> +
> +  const videoReady = helper.waitForPixelColor(
> +      video, helper.red, 128, "Should become red", 

It did become red! (trailing space) (more fun in the editor)

::: dom/media/test/test_mediarecorder_record_addtracked_stream.html:135
(Diff revisions 1 - 2)
> -  video.onerror = err => {
> -    ok(false, "Should be able to play the recording. Got error. code=" + video.error.code);
> -    finish();
> -  };
> +  await addFinallyToPromise(endedNoError).finally(() => {
> +    analyser.disconnect();
> +    delete video.src;
> +    URL.revokeObjectURL(video.src);
> +  });

We should use:

    try {
      await endedNoError;
    } finally {
      analyser.disconnect();
      let url = video.src;
      video.src = "";
      URL.revokeObjectURL(url);
    }

::: dom/media/test/test_mediarecorder_record_addtracked_stream.html:137
(Diff revisions 1 - 2)
> -    video.play();
> +  video.play();
> -  };
>  
> -  video.onerror = err => {
> -    ok(false, "Should be able to play the recording. Got error. code=" + video.error.code);
> -    finish();
> +  await addFinallyToPromise(endedNoError).finally(() => {
> +    analyser.disconnect();
> +    delete video.src;

Pretty sure delete doesn't do anything here. delete only works on own properties.

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

Comment on attachment 8871054 [details]
Bug 1296531 - Notify of realtime data after NotifyDirectListenerInstalled.

https://reviewboard.mozilla.org/r/142286/#review149010

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

Comment on attachment 8871055 [details]
Bug 1296531 - Change MSG ControlMessages from virtual to override.

https://reviewboard.mozilla.org/r/142288/#review149012

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

Comment on attachment 8871056 [details]
Bug 1296531 - Notify MSG track listeners of removal during shutdown.

https://reviewboard.mozilla.org/r/142290/#review149014

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

Comment on attachment 8871057 [details]
Bug 1296531 - Remove deprecated MediaRecorder test.

https://reviewboard.mozilla.org/r/142292/#review149016

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

Comment on attachment 8871060 [details]
Bug 1296531 - Add gtest checking that we encode by timestamp.

https://reviewboard.mozilla.org/r/142298/#review149018

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

Comment on attachment 8871061 [details]
Bug 1296531 - Add gtest for suspending the video encoder.

https://reviewboard.mozilla.org/r/142300/#review149020

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

Comment on attachment 8871062 [details]
Bug 1296531 - Add gtest for a VideoTrackEncoder suspended until the end.

https://reviewboard.mozilla.org/r/142302/#review149022

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

Comment on attachment 8871063 [details]
Bug 1296531 - Add gtest for a VideoTrackEncoder suspended throughout the entire recording.

https://reviewboard.mozilla.org/r/142304/#review149024

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

Comment on attachment 8871064 [details]
Bug 1296531 - Add gtest for an encoding that is suspended in the beginning.

https://reviewboard.mozilla.org/r/142306/#review149026

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

Comment on attachment 8871065 [details]
Bug 1296531 - Add gtest for encoding where suspend/resume timestamps happen in the middle of frame durations.

https://reviewboard.mozilla.org/r/142308/#review149028

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

Comment on attachment 8871066 [details]
Bug 1296531 - Add gtest for an encoding that ends before all pushed data has been consumed.

https://reviewboard.mozilla.org/r/142310/#review149030

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

Comment on attachment 8871067 [details]
Bug 1296531 - Rename gtest TestTrackEncoder.cpp to TestAudioTrackEncoder.cpp.

https://reviewboard.mozilla.org/r/142312/#review149032

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

Comment on attachment 8871068 [details]
Bug 1296531 - Rename existing AudioTrackEncoder gtests to match TestVideoTrackEncoder.

https://reviewboard.mozilla.org/r/142314/#review149034

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

Comment on attachment 8871069 [details]
Bug 1296531 - Add gtest for AudioTrackEncoder metadata.

https://reviewboard.mozilla.org/r/142316/#review149036

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

Comment on attachment 8871070 [details]
Bug 1296531 - Break out SineWaveGenerator from MediaEngineDefault.

https://reviewboard.mozilla.org/r/142318/#review149038

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

Comment on attachment 8871071 [details]
Bug 1296531 - Add AudioGenerator to TestAudioTrackEncoder for simple generation of audio.

https://reviewboard.mozilla.org/r/142320/#review149040

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

Comment on attachment 8871072 [details]
Bug 1296531 - Add gtest for encoding audio data.

https://reviewboard.mozilla.org/r/142322/#review154898

::: dom/media/gtest/TestAudioTrackEncoder.cpp:162
(Diff revision 2)
> +  // Verify that encoded data is 5 seconds long.
> +  uint64_t totalDuration = 0;
> +  for (auto& frame : container.GetEncodedFrames()) {
> +    totalDuration += frame->GetDuration();
> +  }
> +  const uint64_t five = 48000 * 5;

Add a comment that we assume it's encoding at 48000Hz

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

Comment on attachment 8871073 [details]
Bug 1296531 - Fix AudioTrackEncoder resampling gtest to match comment.

https://reviewboard.mozilla.org/r/142324/#review154900

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

Comment on attachment 8871074 [details]
Bug 1296531 - Order MSGImpl.h-includes alphabetically.

https://reviewboard.mozilla.org/r/142326/#review154906

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

Comment on attachment 8871076 [details]
Bug 1296531 - Don't notify of queued data after a track ended.

https://reviewboard.mozilla.org/r/142330/#review154908

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

Comment on attachment 8871077 [details]
Bug 1296531 - Remove MediaStream blocking logic from HTMLMediaElement.

https://reviewboard.mozilla.org/r/142332/#review154912

r+ if this really holds true (always a TrackUnion).  Is there any way we can assert this in debug builds?

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

Comment on attachment 8871078 [details]
Bug 1296531 - Add MediaSegment::IsNull.

https://reviewboard.mozilla.org/r/142334/#review154922

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

Comment on attachment 8871080 [details]
Bug 1296531 - Don't use direct audio listener with full duplex in MediaRecorder.

https://reviewboard.mozilla.org/r/142338/#review154938

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

Comment on attachment 8871081 [details]
Bug 1296531 - Change track encoder gtests to better mimic Gecko usage.

https://reviewboard.mozilla.org/r/142340/#review155124

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

Comment on attachment 8871082 [details]
Bug 1296531 - Add gtests for starting a video track at t > 0.

https://reviewboard.mozilla.org/r/142342/#review155126

::: dom/media/gtest/TestVideoTrackEncoder.cpp:864
(Diff revision 2)
> +  segment.AppendFrame(generator.GenerateI420Image(),
> +                      mozilla::StreamTime(180000), // 2s
> +                      generator.GetSize(),

Perhaps should be 2 * VIDEO_TRACK_RATE for clarity
(aka 90000)

::: dom/media/gtest/TestVideoTrackEncoder.cpp:871
(Diff revision 2)
> +  encoder.SetStartOffset(45000);
> +  encoder.AppendVideoSegment(Move(segment));
> +  encoder.AdvanceCurrentTime(45000);

and VIDEO_TRACK_RATE/2

::: dom/media/gtest/TestVideoTrackEncoder.cpp:902
(Diff revision 2)
> +  YUVBufferGenerator generator;
> +  generator.Init(mozilla::gfx::IntSize(640, 480));
> +  TimeStamp now = TimeStamp::Now();
> +  VideoSegment segment;
> +  segment.AppendFrame(generator.GenerateI420Image(),
> +                      mozilla::StreamTime(180000), // 2s

ditto

::: dom/media/gtest/TestVideoTrackEncoder.cpp:909
(Diff revision 2)
> +  encoder.SetStartOffset(45000);
> +  encoder.AdvanceCurrentTime(45000);

ditto

::: dom/media/gtest/TestVideoTrackEncoder.cpp:938
(Diff revision 2)
> +  YUVBufferGenerator generator;
> +  generator.Init(mozilla::gfx::IntSize(640, 480));
> +  TimeStamp now = TimeStamp::Now();
> +  VideoSegment segment;
> +  segment.AppendFrame(generator.GenerateI420Image(),
> +                      mozilla::StreamTime(90000), // 1s

ditto

::: dom/media/gtest/TestVideoTrackEncoder.cpp:944
(Diff revision 2)
> +  encoder.SetStartOffset(900000);
> +  encoder.AppendVideoSegment(Move(segment));
> +  encoder.AdvanceCurrentTime(45000);

ditto

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

Comment on attachment 8871083 [details]
Bug 1296531 - Break out TracksAvailableCallback logic to Session method.

https://reviewboard.mozilla.org/r/142344/#review155134

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

Comment on attachment 8871084 [details]
Bug 1296531 - Don't wait for TracksAvailableCallback if tracks are already available.

https://reviewboard.mozilla.org/r/142346/#review155136

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

Comment on attachment 8871085 [details]
Bug 1296531 - Make sure test_pc_capturedVideo.html doesn't run out of source before connecting.

https://reviewboard.mozilla.org/r/142348/#review155138

::: dom/media/tests/mochitest/test_peerConnection_capturedVideo.html:33
(Diff revision 2)
>  .catch(e => ok(false, "Unexpected " + e + ":\n" + e.stack));
>  
>  function startTest(media, token) {
>    let manager = window.mediaTestManager;
>    manager.started(token);
> +  info("Starting test for " + media.name);

isn't this redundant with TEST-START?

::: dom/media/tests/mochitest/test_peerConnection_capturedVideo.html:39
(Diff revision 2)
>    var video = document.createElement('video');
>    video.id = "id_" + media.name;
>    video.width = 160;
>    video.height = 120;
>    video.muted = true;
> -  video.loop = true;
> +  video.controls = true;

having it loop doesn't work?

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

Comment on attachment 8871086 [details]
Bug 1296531 - Don't notify of ended tracks when adding a direct listener.

https://reviewboard.mozilla.org/r/142350/#review155140

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

Comment on attachment 8871087 [details]
Bug 1296531 - Make logic that passes buffered data to direct listener generic.

https://reviewboard.mozilla.org/r/142474/#review155142

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

Comment on attachment 8871079 [details]
Bug 1296531 - Refactor MediaRecorder.

https://reviewboard.mozilla.org/r/142336/#review156998

::: dom/media/MediaRecorder.h:70
(Diff revision 2)
>    // raise a dataavailable event containing the Blob of collected data on every timeSlice milliseconds.
>    // If timeSlice isn't provided, UA should call the RequestData to obtain the Blob data, also set the mTimeSlice to zero.
>    void Start(const Optional<int32_t>& timeSlice, ErrorResult & aResult);
>    // Stop the recording activiy. Including stop the Media Encoder thread, un-hook the mediaStreamListener to encoder.
>    void Stop(ErrorResult& aResult);
> -  // Pause the mTrackUnionStream
> +  // Pause a live recording.

Perhaps:
Pause a recording

::: dom/media/MediaRecorder.cpp:508
(Diff revision 2)
> -    if (foundInputPort) {
> -      // A recorded track was removed or ended. End it in the recording.
> -      // Don't raise an error.
> -      foundInputPort->Destroy();
> +    if (mEncoder) {
> +      mEncoder->RemoveMediaStreamTrack(aTrack);
> +    } else {
> +      MOZ_ASSERT(false);

MOZ_ASSERT(mEncoder);
if (mEncoder) {
  mEncoder->...
}

::: dom/media/MediaRecorder.cpp:525
(Diff revision 2)
>        // Get the available tracks from the DOMMediaStream.
>        // The callback will report back tracks that we have to connect to
>        // mTrackUnionStream and listen to principal changes on.

Is there still an mTrackUnionStream?

::: dom/media/MediaRecorder.cpp:752
(Diff revision 2)
> +    if (!pool) {
> +      LOG(LogLevel::Debug, ("Session.InitEncoder !mEncoderThread %p", this));

the log message seems a little abstract... Perhaps "can't create MediaRecorderReadThread pool"

::: dom/media/MediaRecorder.cpp:849
(Diff revision 2)
>      // Media stream is ready but UA issues a stop method follow by start method.
>      // The Session::stop would clean the mTrackUnionStream. If the AfterTracksAdded
>      // comes after stop command, this function would crash.

Is there still an mTrackUnionStream? and is the comment otherwise still correct?

::: dom/media/encoder/MediaEncoder.cpp:58
(Diff revision 2)
> -  MOZ_ASSERT(mVideoEncoder);
> -  // If we're suspended (paused) we don't forward frames
> +    MOZ_RELEASE_ASSERT(mEncoder);
> +    MOZ_RELEASE_ASSERT(mEncoderThread);

What's the reasoning for RELEASE_ASSERT here?

::: dom/media/encoder/MediaEncoder.cpp:91
(Diff revision 2)
> -  mDirectConnected = aConnected;
> +    MOZ_RELEASE_ASSERT(mEncoder);
> +    MOZ_RELEASE_ASSERT(mEncoderThread);

ditto - if they're null and it was MOZ_ASSERT, then it would simply null-deref crash.  This isn't protecting from UAFs.  and it would likely crash only a few lines later with a clear signature

::: dom/media/encoder/MediaEncoder.cpp:127
(Diff revision 2)
> -  if (mSuspended) {
> +    MOZ_RELEASE_ASSERT(mEncoder);
> +    MOZ_RELEASE_ASSERT(mEncoderThread);

etc

::: dom/media/encoder/MediaEncoder.cpp:885
(Diff revision 2)
> +MediaEncoder::IsShutdown()
> +{
> +  MOZ_ASSERT(mEncoderThread->IsCurrentThreadIn());
> +  return mShutdown;

Since this is an atomic, do we need to assert the thread?

::: dom/media/encoder/MediaEncoder.cpp:1003
(Diff revision 2)
> +
> +  if (!mInitialized) {
> +    return;
> +  }
> +
> +  for (int32_t i = mListeners.Length() - 1; i >= 0; --i) {

I presume there's a reason we reverse-iterate mListeners all the time, instead of:
for (auto& listener : mListeners) {
  listener->DataAvailable();
}
or whatever

::: dom/media/encoder/TrackEncoder.h:413
(Diff revision 2)
> +   * Appends source video frames to mIncomingBuffer. We only append the source chunk
> +   * if it is unique to mLastChunk. Called on the MediaStreamGraph thread.

reword slightly perhaps?  "we only append the source chunk if it the image is different than mLastChunk" perhaps?

	Paul Adenot (:padenot)
	Comment 134 • 7 years ago
mozreview-review

Comment on attachment 8871052 [details]
Bug 1296531 - Make AudioNodeStream work with track listeners.

https://reviewboard.mozilla.org/r/142282/#review158216

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail
	Comment 135 • 7 years ago
mozreview-review

Comment on attachment 8871079 [details]
Bug 1296531 - Refactor MediaRecorder.

https://reviewboard.mozilla.org/r/142336/#review158456

::: dom/media/MediaRecorder.cpp:663
(Diff revision 2)
>  
>      // Append pulled data into cache buffer.
>      for (uint32_t i = 0; i < encodedBuf.Length(); i++) {
>        if (!encodedBuf[i].IsEmpty()) {
>          mEncodedBufferCache->AppendBuffer(encodedBuf[i]);
>          // Fire the start event when encoded data is available.

Comment no longer needed?

::: dom/media/encoder/MediaEncoder.h:147
(Diff revision 2)
> -  void NotifyEvent(MediaStreamGraph* aGraph,
> +  void RemoveMediaStreamTrack(dom::MediaStreamTrack* aTrack);
> -                   MediaStreamGraphEvent event) override;
>  
>    /**
>     * Creates an encoder with a given MIME type. Returns null if we are unable
>     * to create the encoder. For now, default aMIMEType to "audio/ogg" and use

Is this defaulting behaviour still the case?

::: dom/media/encoder/MediaEncoder.cpp:206
(Diff revision 2)
> +  void NotifyDirectListenerInstalled(InstallationResult aResult) override
> +  {
> +    if (aResult == InstallationResult::SUCCESS) {
> +      mDirectConnected = true;
> -      } else {
> +    } else {
> -        AudioSegment segment;
> +      LOG(LogLevel::Info, ("Audio track failed to install direct listener"));

Audio -> Video in log. Audio listener logs on installation, could do so here too.

::: dom/media/encoder/MediaEncoder.cpp:240
(Diff revision 2)
> +        NewRunnableMethod<StreamTime>(
> +          mEncoder, &VideoTrackEncoder::SetStartOffset, aTrackOffset));
> +      mInitialized = true;
> +    }
> +
> +    if (aQueuedMedia.IsNull()) {

We don't need a direct connect check here (unlike in audio) because we're always directly connected? However we have checks elsewhere to see if we're direct connected. Can the video track listener work if not direct connected? If not, is it worth removing the other checks?

::: dom/media/encoder/MediaEncoder.cpp:595
(Diff revision 2)
>    PROFILER_LABEL("MediaEncoder", "CreateEncoder",
>      js::ProfileEntry::Category::OTHER);
>  
> -  nsAutoPtr<ContainerWriter> writer;
> -  nsAutoPtr<AudioTrackEncoder> audioEncoder;
> -  nsAutoPtr<VideoTrackEncoder> videoEncoder;
> +  UniquePtr<ContainerWriter> writer;
> +  RefPtr<AudioTrackEncoder> audioEncoder;
> +  RefPtr<AudioTrackListener> audioListener;

These Audio and Video listeners look to be unused.

::: dom/media/encoder/MediaEncoder.cpp:866
(Diff revision 2)
> +  if (!aTrackEncoder) {
> +    NS_ERROR("No track encoder to get metadata from");
> +    return NS_ERROR_FAILURE;
>    }
>  
>    PROFILER_LABEL("MediaEncoder", "CopyMetadataToMuxer",

Can we move this to the start of the function to be consistent with other profiler labels?

::: dom/media/encoder/TrackEncoder.h:537
(Diff revision 2)
> -   * Only accessed in AppendVideoSegment().
>     */
>    StreamTime mEncodedTicks;
>  
>    /**
> -   * The time of the first real video frame passed to the encoder.
> +   * The time of the first real video frame passed to mOutgoingBuffer (at t=0).

Could this comment be expanded upon to mention that start time will be shifted by suspends?

::: dom/media/encoder/TrackEncoder.cpp:27
(Diff revision 2)
>  static const int DEFAULT_CHANNELS = 1;
>  static const int DEFAULT_SAMPLING_RATE = 16000;
>  static const int DEFAULT_FRAME_WIDTH = 640;
>  static const int DEFAULT_FRAME_HEIGHT = 480;
>  static const int DEFAULT_TRACK_RATE = USECS_PER_S;
>  // 30 seconds threshold if the encoder still can't not be initialized.

can't not -> cannot/can't

	Andreas Pehrson [:pehrsons] Assignee
	Comment 138 • 7 years ago
mozreview-review-reply

Comment on attachment 8871050 [details]
Bug 1296531 - Add mochitest for recording a MediaStream with an extra added track.

https://reviewboard.mozilla.org/r/142278/#review148516

> Pretty sure delete doesn't do anything here. delete only works on own properties.

You are right.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 139 • 7 years ago
mozreview-review-reply

Comment on attachment 8871079 [details]
Bug 1296531 - Refactor MediaRecorder.

https://reviewboard.mozilla.org/r/142336/#review156998

> Is there still an mTrackUnionStream?

Somewhat. But in MediaEncoder to support recording audio nodes. I'll rephrase.

> Is there still an mTrackUnionStream? and is the comment otherwise still correct?

I don't think it makes sense anymore. This failure mode (tracks available after stop()) will now be caught by an earlier guard at the top of TracksAvailableCallback.

> Since this is an atomic, do we need to assert the thread?

`mShutdown` is not an atomic. Are you thinking of `mSuspended`?

> I presume there's a reason we reverse-iterate mListeners all the time, instead of:
> for (auto& listener : mListeners) {
>   listener->DataAvailable();
> }
> or whatever

Mainly that we run into trouble if a handler removes the listener (and modifies the array mid-iteration).
Recently I've taken a copy before iterating to make it a bit clearer. I'll go through the ones I find here and do the same.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 140 • 7 years ago
mozreview-review-reply

Comment on attachment 8871079 [details]
Bug 1296531 - Refactor MediaRecorder.

https://reviewboard.mozilla.org/r/142336/#review158456

> Is this defaulting behaviour still the case?

Yeah. I didn't touch this behavior. Code appears to default to audio/ogg as long as there's an audio track available.

> Audio -> Video in log. Audio listener logs on installation, could do so here too.

Good catch!

> We don't need a direct connect check here (unlike in audio) because we're always directly connected? However we have checks elsewhere to see if we're direct connected. Can the video track listener work if not direct connected? If not, is it worth removing the other checks?

Right - audio should always be non-direct (but can still be direct at times), and video should always be direct.

The video listener keeps track of whether the direct listener is installed or not so NotifyDirectListenerUninstalled() and NotifyRemoved() doesn't race over who's going to reset state. Technically the non-direct and the direct listeners are installed separately so we cannot get rid of these sync issues, and there are no other checks.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 141 • 7 years ago
mozreview-review-reply

Comment on attachment 8871085 [details]
Bug 1296531 - Make sure test_pc_capturedVideo.html doesn't run out of source before connecting.

https://reviewboard.mozilla.org/r/142348/#review155138

> isn't this redundant with TEST-START?

`startTest` is run once for each video tested. Videos tested are the ones in `gLongerTests` in dom/media/test/manifest.js.

> having it loop doesn't work?

loop doesn't do anything, because looping to the beginning is the same as seeking to the beginning, and our `HTMLMediaElement.mozCaptureStream` ends all tracks on a seek, and creates new ones as the seek finishes.