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

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

Merged
merged 3 commits into from Feb 14, 2019

Conversation

Projects
None yet
4 participants
@padenot
Copy link
Member

commented Jan 15, 2019

This fixes #1772.


Preview | Diff

@padenot padenot added this to the Web Audio V1 milestone Jan 15, 2019

@padenot padenot self-assigned this Jan 15, 2019

@padenot padenot requested a review from hoch Jan 15, 2019

@padenot padenot force-pushed the padenot:1772-sorting-track-id branch from 4cf8380 to 2498c06 Jan 15, 2019

@hoch hoch added this to In PR Review in V1 Jan 15, 2019

@padenot padenot force-pushed the padenot:1772-sorting-track-id branch from 2498c06 to 8676d96 Jan 16, 2019

@hoch
Copy link
Member

left a comment

Few nits and questions.

index.bs Outdated
<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

This comment has been minimized.

Copy link
@hoch

hoch Jan 17, 2019

Member

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

This comment has been minimized.

Copy link
@padenot

padenot Jan 18, 2019

Author Member

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

This comment has been minimized.

Copy link
@hoch

hoch Jan 18, 2019

Member

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

index.bs Outdated
<code>"audio"</code>.
4. Sort the elements in <var>tracks</var> based on their <code>id</code>
attribute, ordering by Unicode code point
5. Let the first element of <var>tracks</var> be the

This comment has been minimized.

Copy link
@hoch

hoch Jan 17, 2019

Member

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

This comment has been minimized.

Copy link
@padenot

padenot Jan 18, 2019

Author Member

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

This comment has been minimized.

Copy link
@hoch

hoch Jan 18, 2019

Member

ACK

index.bs Outdated
5. Let the first element of <var>tracks</var> be the
[[mediacapture-streams#mediastreamtrack|MediaStreamTrack]] used as the
input audio for this {{MediaStreamAudioSourceNode}}.
6. Let <var>node</var> be a new {{MediaStreamAudioSourceNode}}

This comment has been minimized.

Copy link
@hoch

hoch Jan 17, 2019

Member

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

This comment has been minimized.

Copy link
@padenot

padenot Jan 18, 2019

Author Member

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

This comment has been minimized.

Copy link
@padenot

padenot Jan 18, 2019

Author Member

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.

This comment has been minimized.

Copy link
@hoch

hoch Jan 18, 2019

Member

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

index.bs Outdated
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

This comment has been minimized.

Copy link
@hoch

hoch Jan 17, 2019

Member

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

Also perhaps: s/weird/inappropriate/

This comment has been minimized.

Copy link
@padenot

padenot Jan 18, 2019

Author Member

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.

This comment has been minimized.

Copy link
@hoch

hoch Jan 18, 2019

Member

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

This comment has been minimized.

Copy link
@padenot

padenot Jan 21, 2019

Author Member

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.

@rtoy
Copy link
Contributor

left a comment

Mostly minor typos and such that need to be fixed.

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

This comment has been minimized.

Copy link
@rtoy

rtoy Jan 24, 2019

Contributor

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.

index.bs Outdated
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

This comment has been minimized.

Copy link
@rtoy

rtoy Jan 24, 2019

Contributor

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

index.bs Outdated
throw an {{InvalidStateError}} and abort these steps. Else, let
this stream be <var>inputStream</var>.
3. Let <var>tracks</var> the list of all
[[mediacapture-streams#mediastreamtrack|MediaStreamTrack]] of

This comment has been minimized.

Copy link
@rtoy

rtoy Jan 24, 2019

Contributor

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

index.bs Outdated
<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 lexicographic ordering on sequences of code unit

This comment has been minimized.

Copy link
@rtoy

rtoy Jan 24, 2019

Contributor

Probable typo: "ordering" is used twice here. Need to rephrase.

index.bs Outdated
7. Return <var>node</var>.

Note: The behaviour for picking the track to output is arbitrary for
legacy reasons. {{MediaStreamTrackAudioSourceNode}} should be used

This comment has been minimized.

Copy link
@rtoy

rtoy Jan 24, 2019

Contributor

I think "should" has specific spec meaning here. Is this what we want?

@rtoy

rtoy approved these changes Feb 7, 2019

Copy link
Contributor

left a comment

One minor nit. Otherwise looks good

Show resolved Hide resolved index.bs
@hoch

hoch approved these changes Feb 7, 2019

@svgeesus svgeesus merged commit 1a6fa79 into WebAudio:master Feb 14, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
ipr PR deemed acceptable.
Details

@hoch hoch removed this from In PR Review in V1 Feb 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.