983984 - Default AudioChannel from a pref

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrea Marchesini [:baku]]

Currently the default audio channel is 'normal'. I think it's better to have it from a pref entry. I think in Desktop we want 'content'.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch.patch

mchen, can you give me a feedback about this bug?
The reason why I filed it is because I implemented a nice feature for audio volume/mute per window (bug 923247). This is based on AudioChannelService but on desktop, if we enabled this service, the media elements are muted by default when not visible.

Comment 2

[(no longer active)]

Comment on attachment 8391696 [details] [diff] [review]
patch.patch

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

r=me on the Web Audio bits.

::: content/html/content/src/HTMLMediaElement.cpp
@@ +4030,5 @@
> +        { "alarm",              AudioChannel::Alarm },
> +        { "telephony",          AudioChannel::Telephony },
> +        { "ringer",             AudioChannel::Ringer },
> +        { "publicnotification", AudioChannel::Publicnotification },
> +        { 0 }

Same nit!

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +37,5 @@
> +  { "alarm",              AudioChannel::Alarm },
> +  { "telephony",          AudioChannel::Telephony },
> +  { "ringer",             AudioChannel::Ringer },
> +  { "publicnotification", AudioChannel::Publicnotification },
> +  { 0 }

Nit: nullptr.

Comment 3

[Marco Chen [:mchen]]

Hi Andrew,

Sorry to that I will finish the review tomorrow.
And one quick comment is that 

  Do you think we can put audiochannel mapping table and channel comparison into XXXUtil class?
  Because there are two places used mapping table and three places tried to do comparison.

Thanks.

Comment 4

[Andrea Marchesini [:baku]]

(In reply to Marco Chen [:mchen] from comment #3)
> Hi Andrew,
> 
> Sorry to that I will finish the review tomorrow.
> And one quick comment is that 
> 
>   Do you think we can put audiochannel mapping table and channel comparison
> into XXXUtil class?

Yep, Maybe AudioChannelCommon.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch.patch

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

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8392826 [details] [diff] [review]
patch.patch

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

::: dom/audiochannel/AudioChannelService.h
@@ +110,5 @@
> +    const char* string;
> +    AudioChannel value;
> +  };
> +
> +  AudioChannelTable* GetAudioChannelTable();

Instead of returning the table directly, why not make this a static method that just returns the AudioChannel value corresponding to the current value of the pref?

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch.patch

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8393024 [details] [diff] [review]
patch.patch

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +4017,5 @@
> +      if (audioChannel.IsEmpty()) {
> +        return AudioChannel::Normal;
> +      }
> +
> +      return AudioChannelService::GetAudioChannelFromString(audioChannel);

Sorry, I meant that you should get the pref inside the function too. So call it GetDefaultAudioChannel() with no parameters.

::: dom/audiochannel/AudioChannelService.cpp
@@ +884,5 @@
> +/* static */ AudioChannel
> +AudioChannelService::GetAudioChannelFromString(const nsAString& aName)
> +{
> +  // Mappings from 'mozaudiochannel' attribute strings to an enumeration.
> +  static struct AudioChannelTable

add "const"

Comment 9

[Marco Chen [:mchen]]

Comment on attachment 8393024 [details] [diff] [review]
patch.patch

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +2368,5 @@
> +  }
> +
> +  // Maybe this audio channel is equal to the default value from the pref.
> +  nsString audioChannel = Preferences::GetString("media.defaultAudioChannel");
> +  if (!audioChannel.IsEmpty() && audioChannel.Equals(aString)) {

Do we allow user to choose all channel types as preferred one?
For me, it seems that we don't need to expose channel types other then normal and content.

Once user used "public notification" as preferred one, the audio explicitly defined audio channel type will be muted.

::: dom/audiochannel/AudioChannelService.cpp
@@ +884,5 @@
> +/* static */ AudioChannel
> +AudioChannelService::GetAudioChannelFromString(const nsAString& aName)
> +{
> +  // Mappings from 'mozaudiochannel' attribute strings to an enumeration.
> +  static struct AudioChannelTable

Same concern.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch.patch

Comment 11

[Andrea Marchesini [:baku]]

> Do we allow user to choose all channel types as preferred one?
> For me, it seems that we don't need to expose channel types other then
> normal and content.

Playing with about:config you can do a lot of disaster in firefox.
I think we should allow users to choose any channel they want.
Addons could need them for some particular reason.
The documentation is also written on the wiki.

Comment 12

[Marco Chen [:mchen]]

Comment on attachment 8393395 [details] [diff] [review]
patch.patch

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

::: dom/audiochannel/AudioChannelService.cpp
@@ +924,5 @@
> +    aString.AssignASCII("normal");
> +    return;
> +  }
> +
> +  aString = audioChannel;

I think we should make sure that the string from preference is one of our legal types.
Then just let it return or we leak an illegal string into other places.

ex: 
1. set preference as "foo"
2. set audio.mozaudiochannel = "foo"
3. HTMLMediaElement::CheckAudioChannelPermissions() will treat illegal one as valid after the modification in this patch.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8393395 [details] [diff] [review]
patch.patch

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

I agree with marco but the rest looks good.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch.patch

Comment 15

[Marco Chen [:mchen]]

Comment on attachment 8393479 [details] [diff] [review]
patch.patch

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

It is great to see AudioChannelService working on Desktop too.
Nice work.

Comment 16

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb274bf64633

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bb274bf64633