924870 - AudioContext.mozAudioChannel attribute

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: attribute.patch

Comment 2

[(no longer active)]

We probably need to do this for 1.2 if we want to be able to use Web Audio for some Gaia use cases (such as the dialer app.)  If no such use cases exist for 1.2, this would be fine to delay until 1.3.

Comment 3

[(no longer active)]

Comment on attachment 814854 [details] [diff] [review]
attribute.patch

Review of attachment 814854 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/AudioContext.cpp
@@ +616,5 @@
> +
> +void
> +AudioContext::SetMozAudioChannelType(const nsAString& aValue)
> +{
> +  mDestination->SetMozAudioChannelType(aValue);

We should probably throw if you attempt to set this on an offline context, because that doesn't make much sense.

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +410,5 @@
> +      break;
> +
> +    default:
> +      aValue.AssignASCII("normal");
> +      break;

Using a WebIDL enum would make this dance unnecessary.  :-)

@@ +430,5 @@
> +  } else if (aValue.EqualsASCII("ringer")) {
> +    type = AUDIO_CHANNEL_RINGER;
> +  } else if (aValue.EqualsASCII("publicnotification")) {
> +    type = AUDIO_CHANNEL_PUBLICNOTIFICATION;
> +  }

Ditto.

@@ +436,5 @@
> +  if (type != mAudioChannelType &&
> +      CheckAudioChannelPermissions(aValue)) {
> +    mAudioChannelType = type;
> +
> +    if (mAudioChannelAgent && !Context()->IsOffline()) {

I would assert that the context is not offline here.

::: dom/webidl/AudioContext.webidl
@@ +98,5 @@
>                                               optional unsigned long numberOfOutputChannels = 2);
>  };
>  
> +// Mozilla extensions
> +partial interface AudioContext {

Please hide this behind a pref which is enabled in b2g and disabled elsewhere.  You can call the pref media.webaudio.audioChannels or something.

@@ +100,5 @@
>  
> +// Mozilla extensions
> +partial interface AudioContext {
> +  // Read HTMLMediaElement.webidl for more information about this attribute.
> +  attribute DOMString mozAudioChannelType;

From one perspective, I don't like this moz prefix.  From another perspective, we should keep this for consistency with HTMLMediaElement.

That being said, why is this a string and not an enum?  (It's OK if you wanrt to convert this and HTMLMediaElement to use an enum in a follow-up...)

Comment 4

[Jason Smith [:jsmith]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> We probably need to do this for 1.2 if we want to be able to use Web Audio
> for some Gaia use cases (such as the dialer app.)  If no such use cases
> exist for 1.2, this would be fine to delay until 1.3.

Can you be more specific about what use cases?

Comment 5

[Andrea Marchesini [:baku]]

Attachment: attribute.patch

Comment 6

[(no longer active)]

(In reply to Jason Smith [:jsmith] from comment #4)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> > We probably need to do this for 1.2 if we want to be able to use Web Audio
> > for some Gaia use cases (such as the dialer app.)  If no such use cases
> > exist for 1.2, this would be fine to delay until 1.3.
> 
> Can you be more specific about what use cases?

The dialer app can use the OscillatorNode APIs to generate the DTMF tones, for example.  Or it can use Web Audio to play back ringtones if we want to add some kind of an audio effect on top of it (example: fade-in ring tones)

Comment 7

[(no longer active)]

Comment on attachment 815471 [details] [diff] [review]
attribute.patch

Review of attachment 815471 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +390,5 @@
> +void
> +AudioDestinationNode::SetMozAudioChannelType(AudioChannel aValue, ErrorResult& aRv)
> +{
> +  if (Context()->IsOffline()) {
> +    aRv.Throw(NS_ERROR_FAILURE);

Please throw INVALID_STATE_ERR.

@@ +472,5 @@
> +      type = AUDIO_CHANNEL_RINGER;
> +      break;
> +
> +    case AudioChannel::Publicnotification:
> +      type = AUDIO_CHANNEL_PUBLICNOTIFICATION;

Please file a follow-up to fix HTMLMediaElement to use this enum as well, and hopefully to avoid having to do this.

::: dom/webidl/AudioContext.webidl
@@ +110,5 @@
>  
> +// Mozilla extensions
> +partial interface AudioContext {
> +  // Read HTMLMediaElement.webidl for more information about this attribute.
> +  [Pref="media.useAudioChannelService", SetterThrows]

Oh, right, this is a better pref choice!

Comment 8

[Jason Smith [:jsmith]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #6)
> (In reply to Jason Smith [:jsmith] from comment #4)
> > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> > > We probably need to do this for 1.2 if we want to be able to use Web Audio
> > > for some Gaia use cases (such as the dialer app.)  If no such use cases
> > > exist for 1.2, this would be fine to delay until 1.3.
> > 
> > Can you be more specific about what use cases?
> 
> The dialer app can use the OscillatorNode APIs to generate the DTMF tones,
> for example.  Or it can use Web Audio to play back ringtones if we want to
> add some kind of an audio effect on top of it (example: fade-in ring tones)

Okay.

Sandip - Are there any planned Gaia use cases for 1.2 for integrating with Web Audio APIs?

Comment 9

[Andrea Marchesini [:baku]]

Attachment: attribute.patch

Follow up for HTMLMediaElements and WebIDL enums: Bug 925594

Comment 10

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=293015c1babc

Comment 11

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/387ff170ae27

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/387ff170ae27

Comment 13

[Maire Reavy [:mreavy]]

I believe the only question remaining is whether this fix is important enough to v1.2 to uplift it into Fx 26 for koi.

Comment 14

[Preeti Raghunath(:Preeti)]

Minus.

Comment 15

[Jason Smith [:jsmith]]

(In reply to Preeti Raghunath(:Preeti) from comment #14)
> Minus.

More specifically - we don't have any knowledge of Gaia integration use cases relying on Web Audio, so we're not blocking on this.

Comment 16

[Paul Adenot (:padenot)]

The dialer is being ported to Web Audio.

Comment 17

[(no longer active)]

(In reply to comment #16)
> The dialer is being ported to Web Audio.

For 1.2?

Comment 18

[(no longer active)]

Also, do you have a bug number for that?

Comment 19

[Jason Smith [:jsmith]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #17)
> (In reply to comment #16)
> > The dialer is being ported to Web Audio.
> 
> For 1.2?

Let me ask Julien about this, as he works on comms.

Julien - Do you have knowledge of when the Dialer is being ported to use Web Audio APIs?

Comment 20

[Sandip Kamat]

(In reply to Jason Smith [:jsmith] from comment #8)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #6)
> > (In reply to Jason Smith [:jsmith] from comment #4)
> > > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> > > > We probably need to do this for 1.2 if we want to be able to use Web Audio
> > > > for some Gaia use cases (such as the dialer app.)  If no such use cases
> > > > exist for 1.2, this would be fine to delay until 1.3.
> > > 
> > > Can you be more specific about what use cases?
> > 
> > The dialer app can use the OscillatorNode APIs to generate the DTMF tones,
> > for example.  Or it can use Web Audio to play back ringtones if we want to
> > add some kind of an audio effect on top of it (example: fade-in ring tones)
> 
> Okay.
> 
> Sandip - Are there any planned Gaia use cases for 1.2 for integrating with
> Web Audio APIs?

I do not intend to block on this for v1.2. Unless someone makes a strong case for having this in v1.2, moving Gaia to use WebAudio could be done in v1.3 timeframe.

Comment 21

[Jason Smith [:jsmith]]

Sounds like comment 20 addresses the Gaia integration use case question then for determining blocking.

Comment 22

[Julien Wajsberg [:julienw]]

Ccing Étienne and Anthony who are Dialer's peers. I know nothing about this ;)

Comment 23

[Julien Wajsberg [:julienw]]

Étienne, please see Paul's comment 16: do you need this in 1.2 or is it solely for master ?

Comment 24

[Etienne Segonzac (:etienne)]

(In reply to Julien Wajsberg [:julienw] from comment #23)
> Étienne, please see Paul's comment 16: do you need this in 1.2 or is it
> solely for master ?

master.
(we don't have a final patch ready so 1.2 would be a real stretch :) probably on track for 1.3)