1142933 - [B2G] New audio channel type for system usages

Status: RESOLVED FIXED

(Firefox OS Graveyard :: AudioChannel, defect)

Description

[Alastor Wu [:alwu]]

We defined the UX sound spec in the Bug1068219. The spec mentioned that we need a audio type "system" to handle the usages of UI sound, keyboard, dial pad, screen unlock and screenshot.

Now these usages are implemented by "notification" type, but their sound behaviors don't correspond to the sound spec. Therefore, we need to create a new audio channel type - "system".

Comment 1

[Alastor Wu [:alwu]]

Attachment: Add new audio channel type - "system"

Hi, Ehsan,
Sorry to bother you,
Could you help me review this patch?
This patch implements the new audio channel type - "system", and it can ensure we get the correct sound behaviors which are defined in UX sound spec.

ps. Sound behaviors are defined in the attachment of Bug1068219, page 12.

Thanks a lot :)

Comment 2

[(no longer active)]

Comment on attachment 8577884 [details] [diff] [review]
Add new audio channel type - "system"

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

I prefer Andrea to review this, but he is out this week.  If this can't wait until he gets back, please let me know!

Comment 3

[Alastor Wu [:alwu]]

Hi, Ehsan,
It's not very emergency, I will wait for him coming back.
Thanks you :)

Comment 4

[Andrea Marchesini [:baku]]

Alwu, where do we use 'notification'?

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8577884 [details] [diff] [review]
Add new audio channel type - "system"

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

::: dom/audiochannel/nsIAudioChannelAgent.idl
@@ +44,1 @@
>  interface nsIAudioChannelAgent : nsISupports

change the UUID of this interface.

::: dom/media/CubebUtils.cpp
@@ +183,5 @@
>        return CUBEB_STREAM_TYPE_VOICE_CALL;
>      case dom::AudioChannel::Ringer:
>        return CUBEB_STREAM_TYPE_RING;
> +    case dom::AudioChannel::System:
> +      return CUBEB_STREAM_TYPE_SYSTEM;

where is this defined?

::: dom/system/gonk/AudioManager.cpp
@@ +732,5 @@
>      case AudioChannel::Alarm:
>        status = SetStreamVolumeIndex(AUDIO_STREAM_ALARM, aIndex);
>        break;
> +    case AudioChannel::System:
> +      status = SetStreamVolumeIndex(AUDIO_STREAM_SYSTEM, aIndex);

Where is this defined?

Comment 6

[Alastor Wu [:alwu]]

(In reply to Andrea Marchesini (:baku) from comment #5)
> Alwu, where do we use 'notification'?

As I know, We use the 'notification' in the dialer :) 
https://github.com/mozilla-b2g/gaia/blob/master/shared/js/dialer/keypad.js#L201

> 
> ::: dom/audiochannel/nsIAudioChannelAgent.idl
> @@ +44,1 @@
> >  interface nsIAudioChannelAgent : nsISupports
> 
> change the UUID of this interface.
> 

Sorry, I don't very understand the UUID.
Whether we need to change it when every time we change the interface?

> ::: dom/media/CubebUtils.cpp
> @@ +183,5 @@
> > +    case dom::AudioChannel::System:
> > +      return CUBEB_STREAM_TYPE_SYSTEM;
> 
> where is this defined?
> 

In Cubeb.h.
https://dxr.mozilla.org/mozilla-central/source/media/libcubeb/include/cubeb.h?from=cubeb.h#106

> ::: dom/system/gonk/AudioManager.cpp
> @@ +732,5 @@
> > +    case AudioChannel::System:
> > +      status = SetStreamVolumeIndex(AUDIO_STREAM_SYSTEM, aIndex);
> 
> Where is this defined?

In AudioSystem.h.
https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/android_audio/AudioSystem.h#68

Comment 7

[Andrea Marchesini [:baku]]

> Sorry, I don't very understand the UUID.
> Whether we need to change it when every time we change the interface?


[uuid(2b0222a5-8f7b-49d2-9ab8-cd01b744b23e)]
interface nsIAudioChannelAgent : nsISupports

Anytime you change an IDL interface, change the UUID.

Comment 8

[Alastor Wu [:alwu]]

Attachment: Add new audio channel type - "system". r=baku.

Try-server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2626e94ab030

Comment 9

[Alastor Wu [:alwu]]

Sorry to change the flag frequently, this issue still need to wait for Bug1113086.

Comment 10

[Andrew Overholt [:overholt]]

I am under the impression that bug 1113086 is ready to go and we need to land it *and* this bug (+ perhaps bug 1100822) at the same time.

Alternatively, is there a way to land pieces independently?

Comment 11

[Alastor Wu [:alwu]]

I think we should land the Bug1100822 first, then Bug1113086.
After that, landing other patches (This bug + Bug1129882).

Comment 12

[Andrew Overholt [:overholt]]

Thanks, Alastor.  The bug hierarchy appears correct so I guess once bug 1100822 gets r+ and lands, bug 1113086 can land and so on.

Comment 13

[Alastor Wu [:alwu]]

Attachment: Add new audio channel type, "system". r=baku.

Rebase, and here is the try-server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29c65c3a7e97

Comment 14

[Alastor Wu [:alwu]]

Hi, Ryan,
The failed test case is related with the layout, so I think it might not caused by my patch.
Thanks!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/609926a39887

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/609926a39887