AudioContext.mozAudioChannel attribute

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla27
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:-)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 814854 [details] [diff] [review]
attribute.patch
Attachment #814854 - Flags: review?(ehsan)

Comment 2

5 years ago
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.
blocking-b2g: --- → koi?

Comment 3

5 years ago
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...)
Attachment #814854 - Flags: review?(ehsan) → review-
(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?
(Assignee)

Comment 5

5 years ago
Created attachment 815471 [details] [diff] [review]
attribute.patch
Attachment #814854 - Attachment is obsolete: true
Attachment #815471 - Flags: review?(ehsan)

Comment 6

5 years ago
(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

5 years ago
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!
Attachment #815471 - Flags: review?(ehsan) → review+
(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?
Flags: needinfo?(skamat)
(Assignee)

Comment 9

5 years ago
Created attachment 815666 [details] [diff] [review]
attribute.patch

Follow up for HTMLMediaElements and WebIDL enums: Bug 925594
Attachment #815471 - Attachment is obsolete: true
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/387ff170ae27
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
I believe the only question remaining is whether this fix is important enough to v1.2 to uplift it into Fx 26 for koi.
Minus.
blocking-b2g: koi? → -
(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.
Flags: needinfo?(skamat)
The dialer is being ported to Web Audio.

Comment 17

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

For 1.2?

Comment 18

5 years ago
Also, do you have a bug number for that?
(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?
Flags: needinfo?(felash)

Comment 20

5 years ago
(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.
Sounds like comment 20 addresses the Gaia integration use case question then for determining blocking.
Flags: needinfo?(felash)
Ccing Étienne and Anthony who are Dialer's peers. I know nothing about this ;)
Étienne, please see Paul's comment 16: do you need this in 1.2 or is it solely for master ?
Flags: needinfo?(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)
Flags: needinfo?(etienne)
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.