Mass change P2->P3 to align with new Mozilla triage process.

We changed the version of ReSpec we used on the spec, so the link have changed: https://webaudio.github.io/web-audio-api/#MediaStreamTrackAudioSourceNode is the correct link.

Comment on attachment 8920050 [details] Bug 1324548 - Add MediaStreamTrackAudioSourceNode. https://reviewboard.mozilla.org/r/191040/#review196246 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.cpp:31 (Diff revision 1) > + > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(MediaStreamTrackAudioSourceNode, AudioNode) > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mInputTrack) > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END > + > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(MediaStreamTrackAudioSourceNode) Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Comment on attachment 8920050 [details] Bug 1324548 - Add MediaStreamTrackAudioSourceNode. https://reviewboard.mozilla.org/r/191040/#review196312 Good start, needs a bit of work still! ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.h:94 (Diff revision 1) > + > + // Attaches to the first available audio track in aMediaStream. > + void AttachToFirstTrack(const RefPtr<DOMMediaStream>& aMediaStream); > + > + // From DOMMediaStream::TrackListener. > + void NotifyTrackAdded(const RefPtr<MediaStreamTrack>& aTrack) override; I don't think we need those two (Added and Removed). ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.cpp:47 (Diff revision 1) > +{ > +} > + > +/* static */ already_AddRefed<MediaStreamTrackAudioSourceNode> > +MediaStreamTrackAudioSourceNode::Create(AudioContext& aAudioContext, > + const MediaStreamTrackAudioSourceOptions& aOptions, Indentation is off here. You can do `./mach clang-format` to fix it up. ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.cpp:77 (Diff revision 1) > +{ > + if (!aMediaStreamTrack) { > + aRv.Throw(NS_ERROR_FAILURE); > + return; > + } > + We want to check if the MediaStream track is an audio track here. ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.cpp:101 (Diff revision 1) > + > +void > +MediaStreamTrackAudioSourceNode::AttachToTrack(const RefPtr<MediaStreamTrack>& aTrack) > +{ > + MOZ_ASSERT(!mInputTrack); > + MOZ_ASSERT(aTrack->AsAudioStreamTrack()); Otherwise, we'll crash here in debug. ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.cpp:129 (Diff revision 1) > + mInputPort = nullptr; > + } > +} > + > +void > +MediaStreamTrackAudioSourceNode::AttachToFirstTrack(const RefPtr<DOMMediaStream>& aMediaStream) I think this is dead code. ::: dom/media/webaudio/test/test_mediaStreamTrackAudioSourceNode.html:34 (Diff revision 1) > + source.buffer = createBuffer(context); > + var dest = sourceGraph.createMediaStreamDestination(); > + source.connect(dest); > + > + // Extract first audio track from dest.stream > + var track = dest.stream.getAudioTracks()[0]; You also want a test that tries to pass in a video track, and check that it throws. ::: dom/webidl/MediaStreamTrackAudioSourceNode.webidl:19 (Diff revision 1) > + required MediaStreamTrack mediaStreamTrack; > +}; > + > +[Pref="dom.webaudio.enabled", > + Constructor(AudioContext context, MediaStreamTrackAudioSourceOptions options)] > +interface MediaStreamTrackAudioSourceNode : AudioNode { When we make changes to .webidl files, it is mandatory to have a DOM peer (a person that vouch that this new API can be exposed to web pages). Here, we're copying directly from the spec, but we still need a review. :baku is a good person in Europe for that.

Comment on attachment 8920050 [details] Bug 1324548 - Add MediaStreamTrackAudioSourceNode. https://reviewboard.mozilla.org/r/191040/#review200982

Comment on attachment 8924540 [details] Bug 1324548 - Add context.createMediaStreamTrackSource(). https://reviewboard.mozilla.org/r/195796/#review200980 Can you please split the patch in 2? The restyling of the code should be in a separate patch. This will make the review process faster. Thanks. ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.h:56 (Diff revision 1) > - public DOMMediaStream::TrackListener, > - public PrincipalChangeObserver<MediaStreamTrack> > + : public AudioNode > + , public DOMMediaStream::TrackListener > + , public PrincipalChangeObserver<MediaStreamTrack> > { > public: > - static already_AddRefed<MediaStreamTrackAudioSourceNode> > + static already_AddRefed<MediaStreamTrackAudioSourceNode> Create( Why are you using this style?

I've uploaded a cleaner patch for baku and pehrsons to look at.

Comment on attachment 8925004 [details] Bug 1324548 - Implement MediaStreamTrackAudioSourceNode. https://reviewboard.mozilla.org/r/196248/#review201424 Good start. We'll need the missing file, missing error string, better lifetime support and more extensive tests before this can land though. ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.h:1 (Diff revision 1) > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* vim:set ts=2 sw=2 sts=2 et cindent: */ Per bug 1412125 this should be: ``` /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ /* vim: set ts=8 sts=2 et sw=2 tw=80: */ ``` ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.h:11 (Diff revision 1) > + > +#ifndef MediaStreamTrackAudioSourceNode_h_ > +#define MediaStreamTrackAudioSourceNode_h_ > + > +#include "AudioNode.h" > +#include "DOMMediaStream.h" Is this really needed? (I guess it's residue from copying `MediaStreamAudioSourceNode`) ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.h:47 (Diff revision 1) > + public DOMMediaStream::TrackListener, > + public PrincipalChangeObserver<MediaStreamTrack> Indentation ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.h:47 (Diff revision 1) > +private: > + bool mEnabled; > +}; > + > +class MediaStreamTrackAudioSourceNode : public AudioNode, > + public DOMMediaStream::TrackListener, Is this really needed? ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.h:78 (Diff revision 1) > + return "MediaStreamTrackAudioSourceNode"; > + } > + > + virtual const char* CrossOriginErrorString() const > + { > + return "MediaStreamTrackAudioSourceNodeCrossOrigin"; The patch adding this string for english is missing. See http://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/dom/locales/en-US/chrome/dom/dom.properties#104 ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.h:84 (Diff revision 1) > + // Attaches to aTrack so that its audio content will be used as input. > + void AttachToTrack(const RefPtr<MediaStreamTrack>& aTrack); > + > + // Detaches from the currently attached track if there is one. > + void DetachFromTrack(); This looks like residue from the stream source. It was needed there because tracks could come and go. Here they can't so we can simplify to get rid of them. ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.cpp:1 (Diff revision 1) > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* vim:set ts=2 sw=2 sts=2 et cindent: */ Same here regarding mode line. ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.cpp:47 (Diff revision 1) > + const MediaStreamTrackAudioSourceOptions& aOptions, > + ErrorResult& aRv) indentation ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.cpp:73 (Diff revision 1) > + if (!aMediaStreamTrack) { > + aRv.Throw(NS_ERROR_FAILURE); > + return; > + } Is NS_ERROR_FAILURE the right thing to throw here? I know we do it in places but I recently got pushed back because it is an internal error. A DOM peer would know. ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.cpp:105 (Diff revision 1) > + MOZ_ASSERT(!mInputTrack); > + MOZ_ASSERT(aTrack->AsAudioStreamTrack()); > + > + if (!mStream) { > + return; > + } > + > + mInputTrack = aTrack; > + ProcessedMediaStream* outputStream = > + static_cast<ProcessedMediaStream*>(mStream.get()); > + mInputPort = mInputTrack->ForwardTrackContentsTo(outputStream); > + PrincipalChanged(mInputTrack); // trigger enabling/disabling of the connector > + mInputTrack->AddPrincipalChangeObserver(this); Inline this at the callsite. ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.cpp:112 (Diff revision 1) > + mInputTrack = aTrack; > + ProcessedMediaStream* outputStream = > + static_cast<ProcessedMediaStream*>(mStream.get()); > + mInputPort = mInputTrack->ForwardTrackContentsTo(outputStream); > + PrincipalChanged(mInputTrack); // trigger enabling/disabling of the connector > + mInputTrack->AddPrincipalChangeObserver(this); We also need to listen for the track ending; and when it does, mark this node inactive. For a listener, see MediaStreamTrackConsumer, [1]. For marking it inactive, see [2]. [1] http://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/dom/media/MediaStreamTrack.h#235 [2] http://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/dom/media/webaudio/MediaStreamAudioSourceNode.cpp#159 ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.cpp:116 (Diff revision 1) > + > + mInputTrack = aTrack; > + ProcessedMediaStream* outputStream = > + static_cast<ProcessedMediaStream*>(mStream.get()); > + mInputPort = mInputTrack->ForwardTrackContentsTo(outputStream); > + PrincipalChanged(mInputTrack); // trigger enabling/disabling of the connector Connector? ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.cpp:123 (Diff revision 1) > + if (mInputTrack) { > + mInputTrack->RemovePrincipalChangeObserver(this); > + mInputTrack = nullptr; > + } > + if (mInputPort) { > + mInputPort->Destroy(); > + mInputPort = nullptr; > + } Inline this at the callsite. ::: dom/media/webaudio/test/mochitest.ini:180 (Diff revision 1) > [test_mediaStreamAudioSourceNodeNoGC.html] > [test_mediaStreamAudioSourceNodePassThrough.html] > [test_mediaStreamAudioSourceNodeResampling.html] > tags=capturestream > +[test_mediaStreamTrackAudioSourceNode.html] > +[test_mediaStreamTrackAudioSourceNodeVideo.html] This file is missing. ::: dom/media/webaudio/test/test_mediaStreamTrackAudioSourceNode.html:23 (Diff revision 1) > +var gTest = { > + length: 2048, > + skipOfflineContextTests: true, > + createGraph: function(context) { > + var sourceGraph = new AudioContext(); > + var source = sourceGraph.createBufferSource(); > + source.buffer = createBuffer(context); > + var dest = sourceGraph.createMediaStreamDestination(); > + source.connect(dest); > + > + // Extract first audio track from dest.stream > + var track = dest.stream.getAudioTracks()[0]; > + > + source.start(0); > + > + var mediaStreamTrackSource = new MediaStreamTrackAudioSourceNode(context, { mediaStreamTrack: track }); > + // channelCount and channelCountMode should have no effect > + mediaStreamTrackSource.channelCount = 1; > + mediaStreamTrackSource.channelCountMode = "explicit"; > + return mediaStreamTrackSource; > + }, > + createExpectedBuffers: function(context) { > + return createBuffer(context); > + }, > +}; > + > +runTest(); I'm not so well aware of the webaudio test framework we have but in general I think we also need to test: - AudioContext.createMediaStreamTrackSource() - audio content, i.e., that the audio bits we produce and pipe through the MediaStreamTrackAudioSourceNode comes out the same on the output side. - tracks from different sources. We should include tests for audio tracks from getUserMedia and RTCPeerConnection too (see dom/media/tests/mochitest). - that a cross-origin source is blocked. - the node's lifetime. If no reference are held to it other it being connected to a graph it should stay alive while the track is live and be collected after the track has ended.

Comment on attachment 8925004 [details] Bug 1324548 - Implement MediaStreamTrackAudioSourceNode. https://reviewboard.mozilla.org/r/196248/#review201446 r+ only for the WebIDL part. You should also implement "MediaStreamTrackAudioSourceNode createMediaStreamTrackSource(MediaStreamTrack mediaStreamTrack);" in AudioContext. ::: dom/webidl/MediaStreamTrackAudioSourceNode.webidl:7 (Diff revision 1) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. > + * > + * The origin of this IDL file is > + * https://webaudio.github.io/web-audio-api/ https://webaudio.github.io/web-audio-api/#MediaStreamTrackAudioSourceNode

(last push just adds the test file that I forgot to hg add)

Comment on attachment 8925004 [details] Bug 1324548 - Implement MediaStreamTrackAudioSourceNode. https://reviewboard.mozilla.org/r/196248/#review201852 These comments are for the added tests, previous ones still apply and are the reason for clearing the r? ::: dom/media/webaudio/test/test_mediaStreamTrackAudioSourceNodeVideo.html:14 (Diff revision 2) > + createVideoTrack = () => { > + let c = document.createElement("canvas"); > + c.getContext("2d"); > + return c.captureStream().getTracks()[0]; > + } Inline this at the callsite. ::: dom/media/webaudio/test/test_mediaStreamTrackAudioSourceNodeVideo.html:23 (Diff revision 2) > + expectException(() => { > + let mediaStreamTrackSource = new MediaStreamTrackAudioSourceNode( > + context, > + { mediaStreamTrack: track }); > + }, DOMException.INVALID_STATE_ERR); Looks good, but we should also test `context.createMediaStreamTrackSource()`.

(In reply to Andreas Pehrson [:pehrsons] from comment #18) > Comment on attachment 8925004 [details] > Bug 1324548 - Implement MediaStreamTrackAudioSourceNode. > > https://reviewboard.mozilla.org/r/196248/#review201852 > > These comments are for the added tests, previous ones still apply and are > the reason for clearing the r? Ah, I missed that you had already cleared it. Still good feedback I suppose :-)

Sorry for not sending this earlier. Some tests are still missing but here is a first part of it.

Comment on attachment 8924540 [details] Bug 1324548 - Add context.createMediaStreamTrackSource(). https://reviewboard.mozilla.org/r/195796/#review216786 ::: dom/webidl/AudioContext.webidl:31 (Diff revision 3) > MediaElementAudioSourceNode createMediaElementSource(HTMLMediaElement mediaElement); > > [NewObject, Throws, UnsafeInPrerendering] > MediaStreamAudioSourceNode createMediaStreamSource(MediaStream mediaStream); > > - // Bug 1324548: MediaStreamTrackAudioSourceNode createMediaStreamTrackSource (AudioMediaStreamTrack mediaStreamTrack); > + [NewObject, Throws, UnsafeInPrerendering] indentation.

Comment on attachment 8920050 [details] Bug 1324548 - Add MediaStreamTrackAudioSourceNode. https://reviewboard.mozilla.org/r/191040/#review216784 ::: dom/locales/en-US/chrome/dom/dom.properties:106 (Diff revision 3) > # LOCALIZATION NOTE: Do not translate HTMLMediaElement and createMediaElementSource. > MediaElementAudioSourceNodeCrossOrigin=The HTMLMediaElement passed to createMediaElementSource has a cross-origin resource, the node will output silence. > # LOCALIZATION NOTE: Do not translate MediaStream and createMediaStreamSource. > MediaStreamAudioSourceNodeCrossOrigin=The MediaStream passed to createMediaStreamSource has a cross-origin resource, the node will output silence. > +# LOCALIZATION NOTE : Do not translate MediaStreamTrack and createMediaStreamTrackSource. > +MediaStreamTrackAudioSourceNodeCrossOrigin=The MediaStreamTrack passed to createMediaStreamTrackSource is a cross-origin resource, the node will output slence. silence ::: dom/webidl/MediaStreamTrackAudioSourceNode.webidl:7 (Diff revision 3) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. > + * > + * The origin of this IDL file is > + * https://webaudio.github.io/web-audio-api/ Maybe use this URL: https://webaudio.github.io/web-audio-api/#MediaStreamTrackAudioSourceNode

Comment on attachment 8920050 [details] Bug 1324548 - Add MediaStreamTrackAudioSourceNode. https://reviewboard.mozilla.org/r/191040/#review217030 What you have here is good for r+, but I'd like to see two more tests first: - That GC works after the input track ends but not before. - That we block cross-origin audio, both on creation of the node and after a dynamic change in origin at the input track. ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.h:96 (Diff revision 3) > + TrackListener(MediaStreamTrackAudioSourceNode* aNode) > + { > + mNode = aNode; > + } TrackListener(aNode) : mNode(aNode) {} ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.cpp:12 (Diff revision 3) > +#include "nsIDocument.h" > +#include "mozilla/CORSMode.h" nit: Includes should be sorted. See the coding style on how. ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.cpp:74 (Diff revision 3) > + if (!aMediaStreamTrack) { > + aRv.Throw(NS_ERROR_FAILURE); > + return; > + } I don't think this is possible. MOZ_ASSERT might be enough. ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.cpp:101 (Diff revision 3) > + mInputPort = mInputTrack->ForwardTrackContentsTo(outputStream); > + PrincipalChanged(mInputTrack); // trigger enabling/disabling of the connector > + mInputTrack->AddPrincipalChangeObserver(this); > + > + mInputTrack->AddConsumer(&mTrackListener); > + nit: superfluous newline ::: dom/media/webaudio/MediaStreamTrackAudioSourceNode.cpp:108 (Diff revision 3) > + > +void > +MediaStreamTrackAudioSourceNode::Destroy() > +{ > + if (mInputTrack) { > + mInputTrack->RemovePrincipalChangeObserver(this); Also mInputTrack->RemoveConsumer(&mTrackConsumer). It's stored as a WeakPtr in the MediaStreamTrack so it's not strictly necessary, but it's good form and expected of users.

Comment on attachment 8924540 [details] Bug 1324548 - Add context.createMediaStreamTrackSource(). https://reviewboard.mozilla.org/r/195796/#review217034

Paul: are these patches still relevant? If so can you review them please?

MozReview-Commit-ID: IdVqfNigMyu

MozReview-Commit-ID: 5rvz0K7Hp0q

MozReview-Commit-ID: IdVqfNigMyu

D15958

Pushed by padenot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8e6135544e33 Tests for MediaStreamTrackAudioSourceNode. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/fda93e03e469 Add MediaStreamTrackAudioSourceNode. r=pehrsons,baku

Pushed by padenot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7aefaab277fe Tests for MediaStreamTrackAudioSourceNode. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/055578135fba Add MediaStreamTrackAudioSourceNode. r=pehrsons,baku

Pushed by padenot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/397a9577d91b Tests for MediaStreamTrackAudioSourceNode. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/8097b8fdb9bc Add MediaStreamTrackAudioSourceNode. r=pehrsons,baku https://hg.mozilla.org/integration/autoland/rev/2bb8fcac7edf Adjust WPT expectations