Andrea Marchesini [:baku] Reporter
	Description • 7 years ago

https://webaudio.github.io/web-audio-api/#idl-def-MediaStreamTrackAudioSourceNode

	Bulk Bug Changes for mreavy's org
	Comment 1 • 7 years ago

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

	Paul Adenot (:padenot) Assignee
	Comment 2 • 7 years ago

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.

	Code Review Bot [:reviewbot]
	Comment 4 • 7 years ago
mozreview-review

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]

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

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.

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

Comment on attachment 8920050 [details]
Bug 1324548 - Add MediaStreamTrackAudioSourceNode.

https://reviewboard.mozilla.org/r/191040/#review200982

	Andrea Marchesini [:baku] Reporter
	Comment 9 • 7 years ago
mozreview-review

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?

	Paul Adenot (:padenot) Assignee
	Comment 13 • 7 years ago

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

	Andreas Pehrson [:pehrsons]
	Comment 14 • 7 years ago
mozreview-review

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.

	Andrea Marchesini [:baku] Reporter
	Comment 15 • 7 years ago
mozreview-review

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

	Paul Adenot (:padenot) Assignee
	Comment 17 • 7 years ago

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

	Andreas Pehrson [:pehrsons]
	Comment 18 • 7 years ago
mozreview-review

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()`.

	Andreas Pehrson [:pehrsons]
	Comment 19 • 7 years ago

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

	Léo Paquet
	Comment 22 • 6 years ago

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

	Andrea Marchesini [:baku] Reporter
	Comment 23 • 6 years ago
mozreview-review

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.

	Andrea Marchesini [:baku] Reporter
	Comment 24 • 6 years ago
mozreview-review

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

	Andreas Pehrson [:pehrsons]
	Comment 25 • 6 years ago
mozreview-review

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.

	Andreas Pehrson [:pehrsons]
	Comment 26 • 6 years ago
mozreview-review

Comment on attachment 8924540 [details]
Bug 1324548 - Add context.createMediaStreamTrackSource().

https://reviewboard.mozilla.org/r/195796/#review217034

	Nils Ohlmeier [:drno]
	Comment 27 • 6 years ago

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

	Paul Adenot (:padenot) Assignee
	Comment 28 • 5 years ago

MozReview-Commit-ID: IdVqfNigMyu

	Paul Adenot (:padenot) Assignee
	Comment 29 • 5 years ago

MozReview-Commit-ID: 5rvz0K7Hp0q

	Paul Adenot (:padenot) Assignee
	Comment 30 • 5 years ago

MozReview-Commit-ID: IdVqfNigMyu

	Paul Adenot (:padenot) Assignee
	Comment 32 • 5 years ago

D15958

	Pulsebot
	Comment 34 • 5 years ago

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

	Pulsebot
	Comment 37 • 5 years ago

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

	Pulsebot
	Comment 42 • 5 years ago

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