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.

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.

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.

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

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.

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

Comment on attachment 8871057 [details] Bug 1296531 - Remove deprecated MediaRecorder test. https://reviewboard.mozilla.org/r/142292/#review146210

Comment on attachment 8871058 [details] Bug 1296531 - Add error checks to test_mediarecorder_avoid_recursion.html. https://reviewboard.mozilla.org/r/142294/#review146212

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()); ?

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?

Comment on attachment 8871059 [details] Bug 1296531 - Log error first in test_mr_unsupported_src.html. https://reviewboard.mozilla.org/r/142296/#review146282

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?

Comment on attachment 8871049 [details] Bug 1296531 - Allow MediaSegment::AppendSlice to combine with last chunk. https://reviewboard.mozilla.org/r/142276/#review148344

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... :-)

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

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.

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);

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.

Comment on attachment 8871054 [details] Bug 1296531 - Notify of realtime data after NotifyDirectListenerInstalled. https://reviewboard.mozilla.org/r/142286/#review149010

Comment on attachment 8871055 [details] Bug 1296531 - Change MSG ControlMessages from virtual to override. https://reviewboard.mozilla.org/r/142288/#review149012

Comment on attachment 8871056 [details] Bug 1296531 - Notify MSG track listeners of removal during shutdown. https://reviewboard.mozilla.org/r/142290/#review149014

Comment on attachment 8871057 [details] Bug 1296531 - Remove deprecated MediaRecorder test. https://reviewboard.mozilla.org/r/142292/#review149016

Comment on attachment 8871060 [details] Bug 1296531 - Add gtest checking that we encode by timestamp. https://reviewboard.mozilla.org/r/142298/#review149018

Comment on attachment 8871061 [details] Bug 1296531 - Add gtest for suspending the video encoder. https://reviewboard.mozilla.org/r/142300/#review149020

Comment on attachment 8871062 [details] Bug 1296531 - Add gtest for a VideoTrackEncoder suspended until the end. https://reviewboard.mozilla.org/r/142302/#review149022

Comment on attachment 8871063 [details] Bug 1296531 - Add gtest for a VideoTrackEncoder suspended throughout the entire recording. https://reviewboard.mozilla.org/r/142304/#review149024

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

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

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

Comment on attachment 8871067 [details] Bug 1296531 - Rename gtest TestTrackEncoder.cpp to TestAudioTrackEncoder.cpp. https://reviewboard.mozilla.org/r/142312/#review149032

Comment on attachment 8871068 [details] Bug 1296531 - Rename existing AudioTrackEncoder gtests to match TestVideoTrackEncoder. https://reviewboard.mozilla.org/r/142314/#review149034

Comment on attachment 8871069 [details] Bug 1296531 - Add gtest for AudioTrackEncoder metadata. https://reviewboard.mozilla.org/r/142316/#review149036

Comment on attachment 8871070 [details] Bug 1296531 - Break out SineWaveGenerator from MediaEngineDefault. https://reviewboard.mozilla.org/r/142318/#review149038

Comment on attachment 8871071 [details] Bug 1296531 - Add AudioGenerator to TestAudioTrackEncoder for simple generation of audio. https://reviewboard.mozilla.org/r/142320/#review149040

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

Comment on attachment 8871073 [details] Bug 1296531 - Fix AudioTrackEncoder resampling gtest to match comment. https://reviewboard.mozilla.org/r/142324/#review154900

Comment on attachment 8871074 [details] Bug 1296531 - Order MSGImpl.h-includes alphabetically. https://reviewboard.mozilla.org/r/142326/#review154906

Comment on attachment 8871076 [details] Bug 1296531 - Don't notify of queued data after a track ended. https://reviewboard.mozilla.org/r/142330/#review154908

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?

Comment on attachment 8871078 [details] Bug 1296531 - Add MediaSegment::IsNull. https://reviewboard.mozilla.org/r/142334/#review154922

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

Comment on attachment 8871081 [details] Bug 1296531 - Change track encoder gtests to better mimic Gecko usage. https://reviewboard.mozilla.org/r/142340/#review155124

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

Comment on attachment 8871083 [details] Bug 1296531 - Break out TracksAvailableCallback logic to Session method. https://reviewboard.mozilla.org/r/142344/#review155134

Comment on attachment 8871084 [details] Bug 1296531 - Don't wait for TracksAvailableCallback if tracks are already available. https://reviewboard.mozilla.org/r/142346/#review155136

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?

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

Comment on attachment 8871087 [details] Bug 1296531 - Make logic that passes buffered data to direct listener generic. https://reviewboard.mozilla.org/r/142474/#review155142

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?

Comment on attachment 8871052 [details] Bug 1296531 - Make AudioNodeStream work with track listeners. https://reviewboard.mozilla.org/r/142282/#review158216

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

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.