852831 - When playing HTML audio/video using the volume hardware keys on device changes ringer volume not media volume

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

Steps to reproduce:

1) Go to http://cd.pn/b
2) Press Play
3) When it starts playing, using the volume hardware keys to change volume

Expected result:

4) Devices media volume is changed

Actual result:

4) Devices ringer volume is changed. This means the volume of the playing audio is not changed.

This is a regression. I tested a month old build at it worked as expected result. I'll try to identify the regression.

Comment 1

[cajbir (:cajbir)]

On advice from Matthew Gregan I commented out the opensl_init call in media/libcubeb/src/cubeb.c and tried the test again. With that build the media volume was changed when I used the hardware volume keys as expected.

Comment 2

[Matthew Gregan [:kinetik]]

This might actually be bug 839415, which ported the stream type stuff that we use on B2G to cubeb's OpenSL backend.  It used to be B2G only, but in the cubeb version it's enabled for anything that defines __ANDROID__.

The OpenSL default is SL_ANDROID_STREAM_MEDIA, but MediaDecoder defaults to dom::AUDIO_CHANNEL_NORMAL, which we map to CUBEB_STREAM_TYPE_SYSTEM and then to SL_ANDROID_STREAM_SYSTEM.  That's probably the correct value for B2G, but not for Android.

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: possible fix

Comment 4

[Marco Chen [:mchen]]

Yes, that is a regression from bug 839415.
Thanks Matthew's help here.

Comment 5

[cajbir (:cajbir)]

Comment on attachment 727056 [details] [diff] [review]
possible fix

This approach works.

Comment 6

[Matthew Gregan [:kinetik]]

Comment on attachment 727056 [details] [diff] [review]
possible fix

Thanks for testing!

Let's do this for now, and we'll sort everything else out in bug 809558.

Comment 7

[Marco Chen [:mchen]]

Comment on attachment 727056 [details] [diff] [review]
possible fix

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

Thanks for Chris & Matthew's help on this regression.

::: content/media/AudioStream.cpp
@@ +781,5 @@
>    cubeb_stream_params params;
>    params.rate = aRate;
>    params.channels = aNumChannels;
>  #if defined(__ANDROID__)
> +  params.stream_type = CUBEB_STREAM_TYPE_MUSIC;

Maybe we can use

#if !defined(MOZ_B2G)
  params.stream_type = CUBEB_STREAM_TYPE_MUSIC;
#else
  params.stream_type = ConvertChannelToCubebType(aAudioChannelType);
#endif

So stream_type will not be assiged twice in B2G version.

Comment 8

[Matthew Gregan [:kinetik]]

We made stream_type depend on __ANDROID__ being defined, so change it to the following when I check in:

#if defined(MOZ_B2G)
  params.stream_type = ConvertChannelToCubebType(aAudioChannelType);
#elif defined(__ANDROID__)
  params.stream_type = CUBEB_STREAM_TYPE_MUSIC;
#endif

Comment 9

[Matthew Gregan [:kinetik]]

Er, well, more like what you suggested. :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/07e459058893

Comment 10

[Matthew Gregan [:kinetik]]

And backed out, because ConvertChannelToCubebType is defined for (MOZ_CUBEB && __ANDROID__), but only used when MOZ_B2G is defined, which resulted in a warning that failed due to warnings-as-errors.

https://hg.mozilla.org/integration/mozilla-inbound/rev/37aa10498494

Comment 11

[Matthew Gregan [:kinetik]]

Relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fac4d09757b

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7fac4d09757b