Closed Bug 1296531 Opened 3 years ago Closed 2 years ago

MediaRecorder doesn't record tracks added with MediaStream.addTrack()

Categories

(Core :: Audio/Video: Recording, defect, P3)

51 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox49 --- wontfix
firefox-esr45 --- unaffected
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: tristan.fraipont, Assigned: pehrsons)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(44 files)

1.98 KB, text/html
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
padenot
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
bryce
: review+
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
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.
Rank: 27
Priority: -- → P2
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.
Blocks: 1257318
Status: UNCONFIRMED → NEW
Rank: 27 → 17
Ever confirmed: true
Keywords: regression
Priority: P2 → P1
Assignee: nobody → pehrson
Status: NEW → ASSIGNED
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.
Rank: 17 → 27
Keywords: regression
Priority: P1 → P2
No longer blocks: 1257318
Suspending MediaRecorder must be broken since we moved to direct listeners.
Blocks: 1319446
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).
(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.
(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.
Mass wontfix for bugs affecting firefox 52.
Comment on attachment 8871057 [details]
Bug 1296531 - Remove deprecated MediaRecorder test.

https://reviewboard.mozilla.org/r/142292/#review146210
Attachment #8871057 - Flags: review?(jib) → review+
Comment on attachment 8871058 [details]
Bug 1296531 - Add error checks to test_mediarecorder_avoid_recursion.html.

https://reviewboard.mozilla.org/r/142294/#review146212
Attachment #8871058 - Flags: review?(jib) → 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());

?
Attachment #8871075 - Flags: review?(jib) → 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?
Attachment #8871050 - Flags: review?(jib) → review-
Comment on attachment 8871059 [details]
Bug 1296531 - Log error first in test_mr_unsupported_src.html.

https://reviewboard.mozilla.org/r/142296/#review146282
Attachment #8871059 - Flags: review?(jwwang) → 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

> 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.
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?
Attachment #8871048 - Flags: review?(rjesup) → review+
Comment on attachment 8871049 [details]
Bug 1296531 - Allow MediaSegment::AppendSlice to combine with last chunk.

https://reviewboard.mozilla.org/r/142276/#review148344
Attachment #8871049 - Flags: review?(rjesup) → 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... :-)
Attachment #8871051 - Flags: review?(rjesup) → 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
Attachment #8871053 - Flags: review?(rjesup) → review+
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.
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.
Attachment #8872364 - Flags: review?(jib) → 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);
Attachment #8872365 - Flags: review?(jib) → 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.
Attachment #8871050 - Flags: review?(jib) → review+
Comment on attachment 8871054 [details]
Bug 1296531 - Notify of realtime data after NotifyDirectListenerInstalled.

https://reviewboard.mozilla.org/r/142286/#review149010
Attachment #8871054 - Flags: review?(rjesup) → review+
Comment on attachment 8871055 [details]
Bug 1296531 - Change MSG ControlMessages from virtual to override.

https://reviewboard.mozilla.org/r/142288/#review149012
Attachment #8871055 - Flags: review?(rjesup) → review+
Comment on attachment 8871056 [details]
Bug 1296531 - Notify MSG track listeners of removal during shutdown.

https://reviewboard.mozilla.org/r/142290/#review149014
Attachment #8871056 - Flags: review?(rjesup) → review+
Comment on attachment 8871057 [details]
Bug 1296531 - Remove deprecated MediaRecorder test.

https://reviewboard.mozilla.org/r/142292/#review149016
Attachment #8871057 - Flags: review+
Comment on attachment 8871060 [details]
Bug 1296531 - Add gtest checking that we encode by timestamp.

https://reviewboard.mozilla.org/r/142298/#review149018
Attachment #8871060 - Flags: review?(rjesup) → review+
Comment on attachment 8871061 [details]
Bug 1296531 - Add gtest for suspending the video encoder.

https://reviewboard.mozilla.org/r/142300/#review149020
Attachment #8871061 - Flags: review?(rjesup) → review+
Comment on attachment 8871062 [details]
Bug 1296531 - Add gtest for a VideoTrackEncoder suspended until the end.

https://reviewboard.mozilla.org/r/142302/#review149022
Attachment #8871062 - Flags: review?(rjesup) → 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
Attachment #8871063 - Flags: review?(rjesup) → 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
Attachment #8871064 - Flags: review?(rjesup) → 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
Attachment #8871065 - Flags: review?(rjesup) → 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
Attachment #8871066 - Flags: review?(rjesup) → review+
Comment on attachment 8871067 [details]
Bug 1296531 - Rename gtest TestTrackEncoder.cpp to TestAudioTrackEncoder.cpp.

https://reviewboard.mozilla.org/r/142312/#review149032
Attachment #8871067 - Flags: review?(rjesup) → review+
Comment on attachment 8871068 [details]
Bug 1296531 - Rename existing AudioTrackEncoder gtests to match TestVideoTrackEncoder.

https://reviewboard.mozilla.org/r/142314/#review149034
Attachment #8871068 - Flags: review?(rjesup) → review+
Comment on attachment 8871069 [details]
Bug 1296531 - Add gtest for AudioTrackEncoder metadata.

https://reviewboard.mozilla.org/r/142316/#review149036
Attachment #8871069 - Flags: review?(rjesup) → review+
Comment on attachment 8871070 [details]
Bug 1296531 - Break out SineWaveGenerator from MediaEngineDefault.

https://reviewboard.mozilla.org/r/142318/#review149038
Attachment #8871070 - Flags: review?(rjesup) → review+
Comment on attachment 8871071 [details]
Bug 1296531 - Add AudioGenerator to TestAudioTrackEncoder for simple generation of audio.

https://reviewboard.mozilla.org/r/142320/#review149040
Attachment #8871071 - Flags: review?(rjesup) → 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
Attachment #8871072 - Flags: review?(rjesup) → review+
Comment on attachment 8871073 [details]
Bug 1296531 - Fix AudioTrackEncoder resampling gtest to match comment.

https://reviewboard.mozilla.org/r/142324/#review154900
Attachment #8871073 - Flags: review?(rjesup) → review+
Comment on attachment 8871074 [details]
Bug 1296531 - Order MSGImpl.h-includes alphabetically.

https://reviewboard.mozilla.org/r/142326/#review154906
Attachment #8871074 - Flags: review?(rjesup) → 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
Attachment #8871076 - Flags: review?(rjesup) → 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?
Attachment #8871077 - Flags: review?(rjesup) → review+
Comment on attachment 8871078 [details]
Bug 1296531 - Add MediaSegment::IsNull.

https://reviewboard.mozilla.org/r/142334/#review154922
Attachment #8871078 - Flags: review?(rjesup) → 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
Attachment #8871080 - Flags: review?(rjesup) → review+
Comment on attachment 8871081 [details]
Bug 1296531 - Change track encoder gtests to better mimic Gecko usage.

https://reviewboard.mozilla.org/r/142340/#review155124
Attachment #8871081 - Flags: review?(rjesup) → 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
Attachment #8871082 - Flags: review?(rjesup) → review+
Comment on attachment 8871083 [details]
Bug 1296531 - Break out TracksAvailableCallback logic to Session method.

https://reviewboard.mozilla.org/r/142344/#review155134
Attachment #8871083 - Flags: review?(rjesup) → 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
Attachment #8871084 - Flags: review?(rjesup) → 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?
Attachment #8871085 - Flags: review?(rjesup) → 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
Attachment #8871086 - Flags: review?(rjesup) → 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
Attachment #8871087 - Flags: review?(rjesup) → 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?
Attachment #8871079 - Flags: review?(rjesup) → review+
Attachment #8871052 - Flags: review?(padenot)
Comment on attachment 8871052 [details]
Bug 1296531 - Make AudioNodeStream work with track listeners.

https://reviewboard.mozilla.org/r/142282/#review158216
Attachment #8871052 - Flags: review?(padenot) → 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
Attachment #8871079 - Flags: review?(bvandyk) → review+
Duplicate of this bug: 1382206
Duplicate of this bug: 1377816
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.
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.
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.
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.
Comment on attachment 8902612 [details]
Bug 1296531 - Don't keep OutputMediaStreams with a null mStream member.

https://reviewboard.mozilla.org/r/174218/#review179432
Attachment #8902612 - Flags: review?(jwwang) → review+
Comment on attachment 8872364 [details]
Bug 1296531 - Let waitForAnalysisSuccess take a cancelPromise.

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

Lgtm with nits.

::: dom/media/tests/mochitest/head.js:130
(Diff revisions 1 - 2)
> -  waitForAnalysisSuccess: function(analysisFunction, cancelPromise) {
> -    let self = this;
> -    let rejected = false;
> -    return Promise.race([
> +  waitForAnalysisSuccess: async function(analysisFunction, cancelPromise) {
> +    let cancelled = false;
> +    let cancellation = async () => {
> +      let e = await (cancelPromise || wait(5000, "Audio analysis timed out"));

Nit: we can use default args to simplify:

    waitForAnalysisSuccess: async function(analysisFunction,
                                           cancelPromise = wait(5000, "Audio analysis timed out")) {

::: dom/media/tests/mochitest/head.js:130
(Diff revisions 1 - 2)
> -  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;
> -          }
> -          let success = analysisFunction(self.getByteFrequencyData());
> -          if (success) {
> +  waitForAnalysisSuccess: async function(analysisFunction, cancelPromise) {
> +    let cancelled = false;
> +    let cancellation = async () => {
> +      let e = await (cancelPromise || wait(5000, "Audio analysis timed out"));
> +      cancelled = true;
> +      throw e;
> +    };
> +    let analysis = async () => {
> +      // We need to give the Analyser some time to start gathering data.
> +      await wait(200);
> +      await new Promise(resolve => {
> +        let loop = () => {
> +          if (cancelled || analysisFunction(this.getByteFrequencyData())) {
>              resolve();
>              return;
>            }
>            // else, we need more time
> -          requestAnimationFrame(analysisLoop);
> +          requestAnimationFrame(loop);
>          };
> -        // We need to give the Analyser some time to start gathering data.
> -        wait(200).then(analysisLoop);
> -      }),
> -    ]);
> +        requestAnimationFrame(loop);
> +      });
> +    };
> +    await Promise.race([cancellation(), analysis()]);
>    },

That should work, though async/await lets us loop naturally, which may be easier to follow. Something like:

    async waitForAnalysisSuccess(analysisFunction,
                                 cancelPromise = wait(5000, "Audio analysis timed out")) {
      let aborted = false;
      cancelPromise.then(() => aborted = true);

      await wait(200); // give the Analyser some time to start gathering data
      do {
        await new Promise(resolve => requestAnimationFrame(resolve));
      } while (!aborted && !analysisFunction(this.getByteFrequencyData()));
    }

::: dom/media/tests/mochitest/head.js:131
(Diff revisions 1 - 2)
> -    let self = this;
> -    let rejected = false;
> +    let cancelled = false;
> +    let cancellation = async () => {

Nit: 'aborted' may be preferable due to the two ways to spell cancelation.

(Not kidding, it was enough to change spec apis https://dom.spec.whatwg.org/#aborting-ongoing-activities ).

::: dom/media/tests/mochitest/head.js:922
(Diff revisions 1 - 2)
> -  constructor(color1, color2) {
> -    this._helper = new CaptureStreamTestHelper2D(50,50);
> +  constructor(color1, color2, size) {
> +    if (!size) {
> +      size = 50;
> +    }

Nit: constructor(color1, color2, size = 50) seems nicer (no need to guard against malicious passing in of 0 I think)

::: dom/media/tests/mochitest/head.js:940
(Diff revisions 1 - 2)
> +  helper() {
> +    return this._helper;
> +  }

Nit: If we use getters in cases like these:

    get helper() {
      return this._helper;
    }

Not much more to write, and now callers can use:

    emitter.helper.waitForPixelColor(...);

instead of

    emitter.helper().waitForPixelColor(...)

which seems more natural to me.

That said, I see this matches stream() above so just fyi.

::: dom/media/tests/mochitest/head.js:944
(Diff revisions 1 - 2)
> +  colors(color1, color2) {
> +    this._color1 = color1 ? color1 : this._helper.green;
> +    this._color2 = color2 ? color2 : this._helper.red;

colors(color1 = this._helper.green,
       color2 = this._helper.red) {
Attachment #8872364 - Flags: review?(jib) → review+
Comment on attachment 8872365 [details]
Bug 1296531 - Let waitForPixel and friends take a cancelPromise.

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

Looks great! Mostly lots of nits and opinions, but nothing to hold up review over.

::: dom/canvas/test/captureStream_common.js:65
(Diff revisions 1 - 3)
>     * Returns the pixel at (|offsetX|, |offsetY|) (from top left corner) of
>     * |video| as an array of the pixel's color channels: [R,G,B,A].  Allows
>     * optional scaling of the drawImage() call (so that a 1x1 black image
>     * won't just draw 1 pixel in the corner)
>     */
> -  getPixel: function (video, offsetX, offsetY, width, height) {
> +  getPixel: function (video, offsetX=0, offsetY=0, width=0, height=0) {

spaces around =

Applies throughout.

::: dom/canvas/test/captureStream_common.js:112
(Diff revisions 1 - 3)
> -  waitForPixel: function (video, offsetX, offsetY, test, timeout, width, height, cancelPromise) {
> -    let rejected = false;
> -    return Promise.race([
> -      (cancelPromise || new Promise(() => {})).then(e => (rejected = true, Promise.reject(e))),
> -      new Promise(resolve => {
> -        const startTime = video.currentTime;
> +  waitForPixel: async function (video, test, {
> +    offsetX,
> +    offsetY,
> +    width,
> +    height,
> +    cancelPromise=new Promise(() => {}),
> +  }={}) {
> +    let cancelled = false;
> +    let cancellation = async () => {
> +      let e = await cancelPromise;
> +      cancelled = true;
> +      throw e;
> +    };
> +    let analysis = () => new Promise(resolve => {
> -        let ontimeupdate = () => {
> +      let ontimeupdate = () => {
> -          let pixelMatch = false;
> +        if (cancelled) {
> -          try {
> -            pixelMatch = test(this.getPixel(video, offsetX, offsetY, width, height));
> -          } catch (e) {
> -            info("Waiting for pixel but no video available: " + e + "\n" + e.stack);
> -          }
> -          if (!rejected &&
> -              !pixelMatch &&
> -              (!timeout || video.currentTime < startTime + (timeout / 1000.0))) {
> -            // No match yet and,
> -            // No timeout (waiting indefinitely) or |timeout| has not passed yet.
> -            return;
> +          return;
> -          }
> +        }
> +        try {
> +          if (test(this.getPixel(video, offsetX, offsetY, width, height))) {
> -          video.removeEventListener("timeupdate", ontimeupdate);
> +            video.removeEventListener("timeupdate", ontimeupdate);
> -          if (!rejected) {
> -            resolve(pixelMatch);
> +            resolve();
> +          }
> +        } catch (e) {
> +          info("Waiting for pixel but no video available: " + e + "\n" + e.stack);
> -          }
> +        }
> -        };
> +      };
> -        video.addEventListener("timeupdate", ontimeupdate);
> +      video.addEventListener("timeupdate", ontimeupdate);
> -      }),
> -    ]);
> +    });
> +    await Promise.race([cancellation(), analysis()]);
>    },

Works, but async/await can simplify looping, which might be easier to read:

    async function waitForPixel(video, test, {
                                  offsetX = 0, offsetY = 0, width = 0, height = 0,
                                  cancel = new Promise(() => {})
                                } = {}) {
      let aborted = false;
      cancel.then(e => aborted = true);
      while (true) {
        await Promise.race([new Promise(r => video.addEventListener("timeupdate", r, {once: true})),
                            cancel]);
        if (aborted) {
          throw await cancel;
        }
        try {
          if (test(this.getPixel(video, offsetX, offsetY, width, height))) {
            return;
          }
        } catch (e) {
          info("Waiting for pixel but no video available: " + e + "\n" + e.stack);
        }
      }
    }

::: dom/canvas/test/captureStream_common.js:112
(Diff revisions 1 - 3)
> -  waitForPixel: function (video, offsetX, offsetY, test, timeout, width, height, cancelPromise) {
> -    let rejected = false;
> -    return Promise.race([
> -      (cancelPromise || new Promise(() => {})).then(e => (rejected = true, Promise.reject(e))),
> -      new Promise(resolve => {
> +  waitForPixel: async function (video, test, {
> +    offsetX,
> +    offsetY,
> +    width,
> +    height,

Do we want to default these to 0 maybe?

0 seems like a commonly used value in many tests, and it's easy to leave out arguments now, so this might slim down a few more calls? Just an idea.

::: dom/canvas/test/captureStream_common.js:117
(Diff revisions 1 - 3)
> -  waitForPixel: function (video, offsetX, offsetY, test, timeout, width, height, cancelPromise) {
> -    let rejected = false;
> -    return Promise.race([
> -      (cancelPromise || new Promise(() => {})).then(e => (rejected = true, Promise.reject(e))),
> -      new Promise(resolve => {
> -        const startTime = video.currentTime;
> +  waitForPixel: async function (video, test, {
> +    offsetX,
> +    offsetY,
> +    width,
> +    height,
> +    cancelPromise=new Promise(() => {}),

Opinion: s/cancelPromise/cancel/ maybe? Not a fan of types in names, and having brief names can be useful with destructuring.

::: dom/canvas/test/captureStream_common.js:149
(Diff revisions 1 - 3)
> -  waitForPixelColor: function (video, refColor, threshold, infoString, cancelPromise) {
> +  waitForPixelColor: async function (video, refColor, {
> +    threshold=0,
> +    infoString="n/a",
> +    cancelPromise=new Promise(() => {}),

No need for a default value for cancel here (it's just passed up).

::: dom/canvas/test/captureStream_common.js:165
(Diff revisions 1 - 3)
>        }
>       return this.isPixel(px, refColor, threshold);

funky indentation here (not new)

::: dom/canvas/test/captureStream_common.js:172
(Diff revisions 1 - 3)
> -    }, /*timeout = */0, /*width = */0, /*height = */0, cancelPromise)
> -      .then(() => ok(true, video.id + " " + infoString));
> +    }, {
> +      offsetX: 0,
> +      offsetY: 0,
> +      width: 0,
> +      height: 0,
> +      cancelPromise: cancelPromise,

We can use destructuring: s/cancel: cancel/cancel/

::: dom/canvas/test/captureStream_common.js:182
(Diff revisions 1 - 3)
> -  waitForPixelColorTimeout: function (video, refColor, threshold, timeout, infoString, cancelPromise) {
> +  waitForPixelColorTimeout: async function (video, refColor, {
> +    threshold=0,
> +    timeout=5000,
> +    infoString="n/a",
> +    cancelPromise=new Promise(() => {}),

This function name always confused me. I keep thinking it's like waitForPixelColor but with timeout, but now that function has timeout... Instead this function is not waiting for something to happen, rather it lays down a period of time where something must NOT happen.

It's also a test function, like waitForPixelColor(), but unlike waitForPixel() which is a pure utility function. I find this confusing. How about instead of

    waitForPixelColor
    waitForPixelColorTimeout

we rename them:

    mustBecomePixelColor
    mustNotBecomePixelColor

? Seems to fit will with the typical infoStrings I see for these. Also, maybe s/timeout/time/ to emphasize that timeout here is part of the test, not a failure condition?

Lastly, why does the latter (this) function take a cancelPromise? Seems unused and redundant given its intended use.

::: dom/canvas/test/captureStream_common.js:201
(Diff revisions 1 - 3)
> +        width: 0,
> +        height: 0,
> +      });
> +      throw new Error("Got color " + refColor.name + ". " + infoString);
> +    };
> +    await Promise.race([timer, cancellation(), analysis()]);

s/timer/wait(timeout)/ ?

::: dom/canvas/test/test_capture.html:41
(Diff revision 3)
> +      infoString: "should become red when we get"
> +        + " to stable state (first frame)",

one-liner

::: dom/media/tests/mochitest/head.js:999
(Diff revision 3)
> +    }, {
> +      offsetX: offsetX,
> +      offsetY: offsetY,
>      });

}, {offsetX, offsetY});

::: dom/media/tests/mochitest/head.js:1011
(Diff revision 3)
> -    await this.checkHasFrame(video, offsetX, offsetY, threshold);
> -    let startPixel = { data: h.getPixel(video, offsetX, offsetY)
> -                     , name: "startcolor"
> +    await this.checkHasFrame(video, {
> +      offsetX: offsetX,
> +      offsetY: offsetY,
> +      threshold: threshold,
> +    });

await this.checkHasFrame(video, {offsetX, offsetY, threshold});

::: dom/media/tests/mochitest/head.js:1026
(Diff revision 3)
> +    }, {
> +      offsetX: offsetX,
> +      offsetY: offsetY,
>      });

}, {offsetX, offsetY});

::: dom/media/tests/mochitest/head.js:1039
(Diff revision 3)
> -    await this.checkHasFrame(video, offsetX, offsetY, threshold);
> -    let startPixel = { data: h.getPixel(video, offsetX, offsetY)
> -                     , name: "startcolor"
> +    await this.checkHasFrame(video, {
> +      offsetX: offsetX,
> +      offsetY: offsetY,
> +      threshold, threshold
> +    });

await this.checkHasFrame(video, {offsetX, offsetY, threshold});

::: dom/media/tests/mochitest/head.js:1055
(Diff revision 3)
> -    }, timeout);
> -    ok(!changed, "Frame shouldn't change within " + timeout / 1000 + " seconds.");
> +      }, {
> +        offsetX: offsetX,
> +        offsetY: offsetY,
> +        cancelPromise: wait(timeout, "timeout"),
> +      });

}, {offsetX, offsetY, cancel: wait(timeout, "timeout")});

::: dom/media/tests/mochitest/head.js:1062
(Diff revision 3)
> +        offsetY: offsetY,
> +        cancelPromise: wait(timeout, "timeout"),
> +      });
> +      ok(false, "Frame changed within " + timeout/1000 + " seconds");
> +    } catch (e) {
> +      ok(e == "timeout", "Frame shouldn't change for " + timeout/1000 + " seconds");

Use

    is(e, "timeout", ...)

that way it logs other errors.

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_video.html:51
(Diff revision 3)
>    })
>    .then(() => {
>      info("Video flowing. Pausing.");
>      gUMVideoElement.pause();
>  
> -    return h.checkVideoPaused(captureStreamElement, 10, 10, 16, pausedTimeout);
> +    return h.checkVideoPaused(captureStreamElement, { timeout: pausedTimeout });

Tip: if we s/pausedTimeout/timeout/ then we could have used destructuring here:

    , {timeout});

::: dom/media/tests/mochitest/test_peerConnection_multiple_captureStream_canvas_2d.html:68
(Diff revision 3)
> -                 h.waitForPixelColor(vremote1, h.red, 128,
> -                                     "pcRemote's remote1 should become red"),
> -                 h.waitForPixelColor(vremote2, h.blue, 128,
> -                                     "pcRemote's remote2 should become blue")
> +                 h.waitForPixelColor(vremote1, h.red, {
> +                   threshold: 128,
> +                   infoString: "pcRemote's remote1 should become red",
> +                 }),

Tip: If we use

    let threshold = 128;

first in this file, then we can use destructuring x 6:

    h.waitForPixelColor(vremote1, h.red, {
      threshold,
      infoString: "pcRemote's remote1 should become red",
    });

::: dom/media/tests/mochitest/test_peerConnection_trackDisabling.html:48
(Diff revision 3)
> -                       px => (px[3] = 255, h.isPixelNot(px, h.black, threshold)));
> -      var checkVideoDisabled = video =>
> -        h.waitForPixel(video, offsetX, offsetY,
> -                       px => (px[3] = 255, h.isPixel(px, h.black, threshold, offsetX*2, offsetY*2)));
> +            offsetX: offsetX,
> +            offsetY: offsetY,
> +          });
> +      var checkVideoDisabled = video => h.waitForPixel(video,
> +          px => (px[3] = 255, h.isPixel(px, h.black, threshold)), {
> +            offsetX: offsetX,
> +            offsetY: offsetY,

offsetX, offsetY

::: dom/media/tests/mochitest/test_peerConnection_trackDisabling_clones.html:65
(Diff revision 3)
> -                       px => (px[3] = 255, h.isPixelNot(px, h.black, threshold)));
> -      var checkVideoDisabled = video =>
> -        h.waitForPixel(video, offsetX, offsetY,
> -                       px => (px[3] = 255, h.isPixel(px, h.black, threshold)));
> +            offsetX: offsetX,
> +            offsetY: offsetY,
> +          });
> +      var checkVideoDisabled = video => h.waitForPixel(video,
> +          px => (px[3] = 255, h.isPixel(px, h.black, threshold)), {
> +            offsetX: offsetX,
> +            offsetY: offsetY,

offsetX, offsetY
Attachment #8872365 - Flags: review?(jib) → review+
Comment on attachment 8872364 [details]
Bug 1296531 - Let waitForAnalysisSuccess take a cancelPromise.

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

> That should work, though async/await lets us loop naturally, which may be easier to follow. Something like:
> 
>     async waitForAnalysisSuccess(analysisFunction,
>                                  cancelPromise = wait(5000, "Audio analysis timed out")) {
>       let aborted = false;
>       cancelPromise.then(() => aborted = true);
> 
>       await wait(200); // give the Analyser some time to start gathering data
>       do {
>         await new Promise(resolve => requestAnimationFrame(resolve));
>       } while (!aborted && !analysisFunction(this.getByteFrequencyData()));
>     }

That turned out a lot a lot better, thanks!

> Nit: constructor(color1, color2, size = 50) seems nicer (no need to guard against malicious passing in of 0 I think)

I honestly don't know why this is here. The rebase I guess, sorry about that.

> colors(color1 = this._helper.green,
>        color2 = this._helper.red) {

Same as above, from the rebase.
Comment on attachment 8872365 [details]
Bug 1296531 - Let waitForPixel and friends take a cancelPromise.

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

> No need for a default value for cancel here (it's just passed up).

True, but I like when I don't have to jump around between functions to figure out what value a variable has. I think this has more value than saving a couple of chars.

> This function name always confused me. I keep thinking it's like waitForPixelColor but with timeout, but now that function has timeout... Instead this function is not waiting for something to happen, rather it lays down a period of time where something must NOT happen.
> 
> It's also a test function, like waitForPixelColor(), but unlike waitForPixel() which is a pure utility function. I find this confusing. How about instead of
> 
>     waitForPixelColor
>     waitForPixelColorTimeout
> 
> we rename them:
> 
>     mustBecomePixelColor
>     mustNotBecomePixelColor
> 
> ? Seems to fit will with the typical infoStrings I see for these. Also, maybe s/timeout/time/ to emphasize that timeout here is part of the test, not a failure condition?
> 
> Lastly, why does the latter (this) function take a cancelPromise? Seems unused and redundant given its intended use.

SGTM

I'll make them
    pixelMustBecome
    pixelMustNotBecome
    
Since it's the pixel that becomes a color, and the color is implied from args.

It takes a cancelPromise because of consistency but without much thought I think. I'll remove it.

> s/timer/wait(timeout)/ ?

We don't have wait() here.
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
Blocks: 1400757
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6272e456e63e
Implement MediaSegment move constructor. r=jesup
https://hg.mozilla.org/integration/autoland/rev/bb7f0a1416e3
Allow MediaSegment::AppendSlice to combine with last chunk. r=jesup
https://hg.mozilla.org/integration/autoland/rev/c6f57ba4e67d
Add mochitest for recording a MediaStream with an extra added track. r=jib
https://hg.mozilla.org/integration/autoland/rev/96755ae5bb88
Rip out direct stream listeners from MediaRecorder. r=jesup
https://hg.mozilla.org/integration/autoland/rev/2799df85e817
Make AudioNodeStream work with track listeners. r=padenot
https://hg.mozilla.org/integration/autoland/rev/d59f3574e5e8
Expose GraphRate through DOMMediaStream. r=jesup
https://hg.mozilla.org/integration/autoland/rev/38c52a4cbb77
Notify of realtime data after NotifyDirectListenerInstalled. r=jesup
https://hg.mozilla.org/integration/autoland/rev/2b4e007978a1
Change MSG ControlMessages from virtual to override. r=jesup
https://hg.mozilla.org/integration/autoland/rev/112382900659
Notify MSG track listeners of removal during shutdown. r=jesup
https://hg.mozilla.org/integration/autoland/rev/43a5598d0fc6
Remove deprecated MediaRecorder test. r=jesup,jib
https://hg.mozilla.org/integration/autoland/rev/e9335ee7fdfd
Add error checks to test_mediarecorder_avoid_recursion.html. r=jib
https://hg.mozilla.org/integration/autoland/rev/57e339b35453
Log error first in test_mr_unsupported_src.html. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/b0dda92d1574
Add gtest checking that we encode by timestamp. r=jesup
https://hg.mozilla.org/integration/autoland/rev/6695e96afbdf
Add gtest for suspending the video encoder. r=jesup
https://hg.mozilla.org/integration/autoland/rev/bc3d9b8fa333
Add gtest for a VideoTrackEncoder suspended until the end. r=jesup
https://hg.mozilla.org/integration/autoland/rev/c4fae1605712
Add gtest for a VideoTrackEncoder suspended throughout the entire recording. r=jesup
https://hg.mozilla.org/integration/autoland/rev/81c3fc64fe9f
Add gtest for an encoding that is suspended in the beginning. r=jesup
https://hg.mozilla.org/integration/autoland/rev/9ff34ff8a745
Add gtest for encoding where suspend/resume timestamps happen in the middle of frame durations. r=jesup
https://hg.mozilla.org/integration/autoland/rev/ff830ca6e0d2
Add gtest for an encoding that ends before all pushed data has been consumed. r=jesup
https://hg.mozilla.org/integration/autoland/rev/445802994033
Rename gtest TestTrackEncoder.cpp to TestAudioTrackEncoder.cpp. r=jesup
https://hg.mozilla.org/integration/autoland/rev/bb47b1aba6c7
Rename existing AudioTrackEncoder gtests to match TestVideoTrackEncoder. r=jesup
https://hg.mozilla.org/integration/autoland/rev/7ee3807be534
Add gtest for AudioTrackEncoder metadata. r=jesup
https://hg.mozilla.org/integration/autoland/rev/a832d1310e63
Break out SineWaveGenerator from MediaEngineDefault. r=jesup
https://hg.mozilla.org/integration/autoland/rev/3c010e4b44e0
Add AudioGenerator to TestAudioTrackEncoder for simple generation of audio. r=jesup
https://hg.mozilla.org/integration/autoland/rev/463b0c40f07e
Add gtest for encoding audio data. r=jesup
https://hg.mozilla.org/integration/autoland/rev/dd39cf469d20
Fix AudioTrackEncoder resampling gtest to match comment. r=jesup
https://hg.mozilla.org/integration/autoland/rev/f265edf5234c
Order MSGImpl.h-includes alphabetically. r=jesup
https://hg.mozilla.org/integration/autoland/rev/555d0dc71713
Break out ShutdownTicket and GetShutdownBarrier from MSG to MediaUtils. r=jib
https://hg.mozilla.org/integration/autoland/rev/f1f93df303b6
Don't notify of queued data after a track ended. r=jesup
https://hg.mozilla.org/integration/autoland/rev/e70956077d8b
Remove MediaStream blocking logic from HTMLMediaElement. r=jesup
https://hg.mozilla.org/integration/autoland/rev/5cba097bf1ed
Add MediaSegment::IsNull. r=jesup
https://hg.mozilla.org/integration/autoland/rev/02d44644658b
Refactor MediaRecorder. r=jesup,SingingTree
https://hg.mozilla.org/integration/autoland/rev/2f254c866347
Don't use direct audio listener with full duplex in MediaRecorder. r=jesup
https://hg.mozilla.org/integration/autoland/rev/9fb91aa67681
Change track encoder gtests to better mimic Gecko usage. r=jesup
https://hg.mozilla.org/integration/autoland/rev/fbb794b4ab77
Add gtests for starting a video track at t > 0. r=jesup
https://hg.mozilla.org/integration/autoland/rev/55682948b181
Break out TracksAvailableCallback logic to Session method. r=jesup
https://hg.mozilla.org/integration/autoland/rev/d84ded64c0bb
Don't wait for TracksAvailableCallback if tracks are already available. r=jesup
https://hg.mozilla.org/integration/autoland/rev/dae100b29584
Make sure test_pc_capturedVideo.html doesn't run out of source before connecting. r=jesup
https://hg.mozilla.org/integration/autoland/rev/fcf31ee07627
Don't notify of ended tracks when adding a direct listener. r=jesup
https://hg.mozilla.org/integration/autoland/rev/7e3345ae9751
Make logic that passes buffered data to direct listener generic. r=jesup
https://hg.mozilla.org/integration/autoland/rev/173b2aca33b5
Let waitForAnalysisSuccess take a cancelPromise. r=jib
https://hg.mozilla.org/integration/autoland/rev/137df0819027
Let waitForPixel and friends take a cancelPromise. r=jib
https://hg.mozilla.org/integration/autoland/rev/8448eee20c9a
Don't keep OutputMediaStreams with a null mStream member. r=jwwang
https://hg.mozilla.org/mozilla-central/rev/6272e456e63e
https://hg.mozilla.org/mozilla-central/rev/bb7f0a1416e3
https://hg.mozilla.org/mozilla-central/rev/c6f57ba4e67d
https://hg.mozilla.org/mozilla-central/rev/96755ae5bb88
https://hg.mozilla.org/mozilla-central/rev/2799df85e817
https://hg.mozilla.org/mozilla-central/rev/d59f3574e5e8
https://hg.mozilla.org/mozilla-central/rev/38c52a4cbb77
https://hg.mozilla.org/mozilla-central/rev/2b4e007978a1
https://hg.mozilla.org/mozilla-central/rev/112382900659
https://hg.mozilla.org/mozilla-central/rev/43a5598d0fc6
https://hg.mozilla.org/mozilla-central/rev/e9335ee7fdfd
https://hg.mozilla.org/mozilla-central/rev/57e339b35453
https://hg.mozilla.org/mozilla-central/rev/b0dda92d1574
https://hg.mozilla.org/mozilla-central/rev/6695e96afbdf
https://hg.mozilla.org/mozilla-central/rev/bc3d9b8fa333
https://hg.mozilla.org/mozilla-central/rev/c4fae1605712
https://hg.mozilla.org/mozilla-central/rev/81c3fc64fe9f
https://hg.mozilla.org/mozilla-central/rev/9ff34ff8a745
https://hg.mozilla.org/mozilla-central/rev/ff830ca6e0d2
https://hg.mozilla.org/mozilla-central/rev/445802994033
https://hg.mozilla.org/mozilla-central/rev/bb47b1aba6c7
https://hg.mozilla.org/mozilla-central/rev/7ee3807be534
https://hg.mozilla.org/mozilla-central/rev/a832d1310e63
https://hg.mozilla.org/mozilla-central/rev/3c010e4b44e0
https://hg.mozilla.org/mozilla-central/rev/463b0c40f07e
https://hg.mozilla.org/mozilla-central/rev/dd39cf469d20
https://hg.mozilla.org/mozilla-central/rev/f265edf5234c
https://hg.mozilla.org/mozilla-central/rev/555d0dc71713
https://hg.mozilla.org/mozilla-central/rev/f1f93df303b6
https://hg.mozilla.org/mozilla-central/rev/e70956077d8b
https://hg.mozilla.org/mozilla-central/rev/5cba097bf1ed
https://hg.mozilla.org/mozilla-central/rev/02d44644658b
https://hg.mozilla.org/mozilla-central/rev/2f254c866347
https://hg.mozilla.org/mozilla-central/rev/9fb91aa67681
https://hg.mozilla.org/mozilla-central/rev/fbb794b4ab77
https://hg.mozilla.org/mozilla-central/rev/55682948b181
https://hg.mozilla.org/mozilla-central/rev/d84ded64c0bb
https://hg.mozilla.org/mozilla-central/rev/dae100b29584
https://hg.mozilla.org/mozilla-central/rev/fcf31ee07627
https://hg.mozilla.org/mozilla-central/rev/7e3345ae9751
https://hg.mozilla.org/mozilla-central/rev/173b2aca33b5
https://hg.mozilla.org/mozilla-central/rev/137df0819027
https://hg.mozilla.org/mozilla-central/rev/8448eee20c9a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Blocks: 1224079
Depends on: 1406027
Depends on: 1411322
Depends on: 1411578
Depends on: 1414277
Duplicate of this bug: 1427183
Added a footnote to the compatibility information:

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

And mentioned on Firefox 58 for developers.
Depends on: 1447273
Depends on: 1453078
You need to log in before you can comment on or make changes to this bug.