Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Define which track to use in MediaStreamAudioSourceNode more clearly. #1811

Merged
merged 3 commits into from Feb 14, 2019
Merged

Define which track to use in MediaStreamAudioSourceNode more clearly. #1811

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8676d96
Define which track to use in MediaStreamAudioSourceNode more clearly.
padenot Jan 15, 2019
51748a8
Address review comments
padenot Jan 18, 2019
6d9c6fb
Address comments from @rtoy
padenot Feb 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 21 additions & 16 deletions index.bs
View file
Open in desktop
Expand Up @@ -7869,17 +7869,7 @@ Attributes</h4>
The {{MediaStreamAudioSourceNode}} Interface</h3>

This interface represents an audio source from a
[[mediacapture-streams#mediastream|MediaStream]]. The track that will be used as the source
of audio and will be output from this node is the first
[[mediacapture-streams#mediastreamtrack|MediaStreamTrack]] whose <code>kind</code> attribute has
the value <code>"audio"</code>, when alphabetically sorting the
tracks of this [[mediacapture-streams#mediastream|MediaStream]] by their <code>id</code>
attribute. Those interfaces are described in
[[!mediacapture-streams]].

Note: The behaviour for picking the track to output is weird for legacy
reasons. {{MediaStreamTrackAudioSourceNode}} should be used
instead.
[[mediacapture-streams#mediastream|MediaStream]].

<pre class=include>
path: audionode-noinput.include
Expand All @@ -7888,9 +7878,9 @@ macros:
tail-time: No
</pre>

The number of channels of the output corresponds to the number of
channels of the [[mediacapture-streams#mediastreamtrack|MediaStreamTrack]]. If there is no valid
audio track, then the number of channels output will be one silent
The number of channels of the output corresponds to the number of channels of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As best as I can tell, nothing really changed here except for line wrapping. Please don't do that. It makes review harder.

You can leave this, but please verify there were no changes.

the [[mediacapture-streams#mediastreamtrack|MediaStreamTrack]]. If there is no
valid audio track, then the number of channels output will be one silent
channel.

<pre class="idl">
Expand All @@ -7914,10 +7904,25 @@ Constructors</h4>
[[mediacapture-streams#mediastream|MediaStream]] that has at least one
[[mediacapture-streams#mediastreamtrack|MediaStramTrack]] whose
<code>kind</code> attribute has the value <code>"audio"</code>,
throw an {{InvalidStateError}} and abort these steps.
3. Let <var>node</var> be a new {{MediaStreamAudioSourceNode}}
throw an {{InvalidStateError}} and abort these steps. Else, let
this stream be <var>inputStream</var>.
3. Let <var>tracks</var> the list of all
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "Let tracks the" -> "Let tracks be the"

[[mediacapture-streams#mediastreamtrack|MediaStreamTrack]] of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "MediaStreamTrack" -> "MediaStreamTracks" (but probably "[[...|MediaStreamTrack]]s"

<var>inputStream</var> that have a <code>kind</code> of
<code>"audio"</code>.
4. Sort the elements in <var>tracks</var> based on their <code>id</code>
attribute, ordering by Unicode code point
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add a period after "point".
question: Do we need a reference link on "Unicode code point"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear. Strictly speaking what we want here is a "lexicographic ordering on sequences of code unit values". Maybe it's best to just say this. It's what sort uses in JavaScript.

I found this here: https://tc39.github.io/ecma262/#sec-abstract-relational-comparison

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on your suggestion. We don't need a link to the ECMA doc.

5. Let the first element of <var>tracks</var> be the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to describe the rest of tracks as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we leave them alone, they are of no importance for us.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

[[mediacapture-streams#mediastreamtrack|MediaStreamTrack]] used as the
input audio for this {{MediaStreamAudioSourceNode}}.
6. Let <var>node</var> be a new {{MediaStreamAudioSourceNode}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also how do we attach these "sorted tracks" to the node? Should it be explicit? or is it fine as it is now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking maybe that we should stick the chosen track into a private slot, what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically, the other tracks aren't mentioned, because regardless of what happens to them, it does not affect this node. I think it's still good to stick the track into a slot though.

In general, those Media*AudioSourceNode are a bit hand-wavy when it comes to the actual processing.

It's not that hard, but implementations vary widely (in terms of latency, notably), and, for example what happens when the input CORS-status changes during processing is a bit unclear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on putting it into a private slot. Clarifying behaviors of Media*AudioSourceNode can be a good V2 issue.

object. <a href="#audionode-constructor-init">Initialize</a>
<var>node</var>, and return <var>node</var>.

Note: The behaviour for picking the track to output is weird for legacy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what this means. Can you elaborate?

Also perhaps: s/weird/inappropriate/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copy/pasted this from above. I'm going to use the word "arbitrary", I think it reflects the thinking here and is more neutral.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would "unspecified" be too strong for this? So what we want to point out is there might be an interop issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't say it's unspecified if we're speccing it. We know what is going to happen, without ambiguity, it's just a bit strange.

reasons. {{MediaStreamTrackAudioSourceNode}} should be used
instead.

<pre class=argumentdef for="MediaStreamAudioSourceNode/MediaStreamAudioSourceNode()">
context: The {{AudioContext}} this new {{MediaStreamAudioSourceNode}} will be <a href="#associated">associated</a> with.
options: Initial parameter value for this {{MediaStreamAudioSourceNode}}.
Expand Down