795237 - Web API for setting audio stream type.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P1)

Description

[Marco Chen [:mchen]]

B2G leverage the Audio HAL from Android and this provided the ability for adjusting each volume on audio stream types. Ex: we can adjust volume on each MUSIC, RING, SYSTEM and others.

But in the http://mxr.mozilla.org/mozilla-central/source/media/libsydneyaudio/src/sydney_audio_gonk.cpp#126 , it hardcoded the stream type to SYSTEM. 

We need a new WebAPI or modification on current WebAPI for assigning stream type.

Comment 1

[Marco Chen [:mchen]]

Another requirement is there can be a different levels on each stream types so Gaia needs Web API to get the maximum level of each stream type. Then Gaia can show corresponding levels but fixed (ex: always 10 levels for each stream types.)

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Should be blocking coz it's blocking blocking bugs.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

IMO We could simply expose the stream type as an attribute of HTMLAudioElement, and create yet another permission to allow/disallow access of that attribute.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Wasn't there already Settings API for setting per audio stream volume?

Comment 5

[Marco Chen [:mchen]]

For comment 4, yes, there is already a Setting API for volume adjusting.
But there is no API for assigning <audio> or Audio() a stream type.
So currently they are all belonged to system stream type only.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I thought that we were planning on working around this by changing the volume setting temporarily and then setting it back.

Is that not acceptable?

I'm worried about taking on this work past feature freeze.

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Marco Chen [:mchen] from comment #5)
> For comment 4, yes

You can use "reply".

> There is already a Setting API for volume adjusting. But there is no API for assigning
> <audio> or Audio() a stream type. So currently they are all belonged to system stream
> type only.

I still don't understand. Your patch deals with stream volume adjusting. It affects system-wise stream volume, not only one specific audio element. Yes, all audio elements are system streams, but this won't stop you from examining the volume of system stream. If you're going to set stream volume through Settings API, then you should retrieve the volume value through the same interface, still Settings API. So, why retrieving some values from Settings API has anything to do with new audio stream type assignment API?

Take data connection settings for example. The Settings app set per APN config values through Settings API. The RadioInterfaceLayer sees the change and performs corresponding actions. If any failure found, RadioInterfaceLayer resets the value through Settings API again and keeps the settings values and the reality synced.

For audio stream volume adjustments, you set the volume value through Settings API, and AudioManager performs the platform specific call for you. If any failure found, AndioManager should retrive the correct value from platform specific call if necessary, and sets stream volume through Settings API again in order to keep settings values and the hardware settings synced. Do I miss anything?

Comment 8

[Marco Chen [:mchen]]

Hi Vicamo, may we discuss test topic for bug 791642 on it's following up Bug 796874?
I will reply you there and keep here focused on Web API definition.

Thanks.

Comment 9

[Marco Chen [:mchen]]

(In reply to Jonas Sicking (:sicking) from comment #6)
> I thought that we were planning on working around this by changing the
> volume setting temporarily and then setting it back.
> 
> Is that not acceptable?
> 
> I'm worried about taking on this work past feature freeze.

For bug 796259, if the device is on
  a. muting
  b. music playing (or any other audio stream is on going)
then alarm is triggered. 
In this case in order to hear alarm sound, we temporarily set volume up.
The result is that not only alarm sound but also music playing will be hear.
Could this be acceptable?

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Marco Chen [:mchen] from comment #9)
> (In reply to Jonas Sicking (:sicking) from comment #6)
> > I thought that we were planning on working around this by changing the
> > volume setting temporarily and then setting it back.
> > 
> > Is that not acceptable?
> > 
> > I'm worried about taking on this work past feature freeze.
> 
> For bug 796259, if the device is on
>   a. muting
>   b. music playing (or any other audio stream is on going)
> then alarm is triggered. 
> In this case in order to hear alarm sound, we temporarily set volume up.
> The result is that not only alarm sound but also music playing will be hear.
> Could this be acceptable?

IMHO, it is not acceptable. It is audible feature, a user could easily recognize it and feels it is a phone's defect and annoying. I work for developing mobile phones several years, but I do not know a mobile phone work like it.

Comment 11

[Marco Chen [:mchen]]

Could any one make the decision whether it is a blocking-basecamp or not?
Thanks.

Comment 12

[Andrew Overholt [:overholt]]

(In reply to Marco Chen [:mchen] from comment #11)
> Could any one make the decision whether it is a blocking-basecamp or not?

We'll discuss it again in triage today.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, I think this is blocking, but I still don't know how we should solve things here.

Would really like roc's input here.

The short story is that on B2G we have the need to play some audio streams into the loudspeaker of the phone rather than through the normal audio channel.

For example, there's an app which is playing music, but the volume is turned way down. If there's an incoming phone call we want to allow the dialer app to play a loud audio ringer. Simply playing it as an <audio> would result in playing at the same volume as the turned-down music, so that's not a workable solution.

One option that we've previously discussed was to simply let the dialer app use the settings API to turn up the volume before playing the ringer audio. But this would result in the music also playing at full volume, which doesn't seem like a good solution.

The same situation arises in the alarm app which wants to play audio at full volume in order to wake up the user.

Additionally I think that we actually want to mute all other audio in these situations. This is especially the case if the user is playing other sounds with the volume *up* when there's an incoming phone call. We don't want the dialer app to be competing with other audio. That could make it unclear that there's an incoming phone call.

So I think we need the following capabilities:

1. Have multiple audio channels, like "normal", "attention" and "ringer" (but maybe the ringer could simply use "attention").
2. Be able to set the volume for these independently. We can simply use the settings API for this.
3. Allow an app to tell the system "if the user presses the volume up/down buttons, adjust the volume for type X". By default we'd just adjust the "normal" volume, but while the user is in the alarm or dialer app we likely want to hook up the volume buttons to adjust another channel. Possibly this could simply be done by letting the apps call .preventDefault() on the key events for the up/down volume buttons and then have the app set the volume for other channels using an explicit privileged API.
4. Enable privileged apps to use the "attention"/"ringer" channels when playing audio.
5. Enable privileged apps to silence the "normal" audio channel.

Comment 14

[Marco Chen [:mchen]]

(In reply to Jonas Sicking (:sicking) from comment #13)

> Additionally I think that we actually want to mute all other audio in these
> situations. This is especially the case if the user is playing other sounds
> with the volume *up* when there's an incoming phone call. We don't want the
> dialer app to be competing with other audio. That could make it unclear that
> there's an incoming phone call.
> 

In this situation, I proposed related apps stopped their own audio stream by listening the specific event like "call state changing". 
The reason is
  Power Consumption: The voice call is most higher priority on phone product, so we need keep this behavior as long as possible. Since the other audio streams are muted, why do we stop them for saving power.

> So I think we need the following capabilities:
> 
> 1. Have multiple audio channels, like "normal", "attention" and "ringer"
> (but maybe the ringer could simply use "attention").
> 2. Be able to set the volume for these independently. We can simply use the
> settings API for this.

Provide the stream types from Android Audio HAL for reference.
default, voice call, system, ring, music, alarm, notification, BT SCO, enforced audible, DTMF, TTS, FM.

> 3. Allow an app to tell the system "if the user presses the volume up/down
> buttons, adjust the volume for type X". By default we'd just adjust the
> "normal" volume, but while the user is in the alarm or dialer app we likely
> want to hook up the volume buttons to adjust another channel. Possibly this
> could simply be done by letting the apps call .preventDefault() on the key
> events for the up/down volume buttons and then have the app set the volume
> for other channels using an explicit privileged API.

This is why we need to plan the new Web API or modification on existing one to achieve this.
a. Web API for assigning stream type on each <audio>.
b. Web API for adjusting volume with argument of stream type.

> 5. Enable privileged apps to silence the "normal" audio channel.

I proposed the another way on case of in call. Is there any other case needed to do this?

Comment 15

[Marco Chen [:mchen]]

s/why do we/why don't we

Comment 16

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

Matthew may have an opinion here.

Can we simply assign a audio channel type to each *application* (or page) instead of each <audio>/<video> element? That might simplify things especially as we add new ways to generate audio such as Web Audio and MediaStreams.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Marco Chen [:mchen] from comment #14)
> (In reply to Jonas Sicking (:sicking) from comment #13)
> 
> > Additionally I think that we actually want to mute all other audio in these
> > situations. This is especially the case if the user is playing other sounds
> > with the volume *up* when there's an incoming phone call. We don't want the
> > dialer app to be competing with other audio. That could make it unclear that
> > there's an incoming phone call.
> > 
> 
> In this situation, I proposed related apps stopped their own audio stream by
> listening the specific event like "call state changing". 
> The reason is
>   Power Consumption: The voice call is most higher priority on phone
> product, so we need keep this behavior as long as possible. Since the other
> audio streams are muted, why do we stop them for saving power.

We can certainly fire an event on all apps when an incoming phonecall starts and ends, but I don't think that will solve all our problems for two reasons:

1. Not all applications will listen to this event and turn off all audio. I.e. we need a way to *force* audio off if we want to ensure that no other audio is playing.
2. This doesn't solve the alarm app problem. We want to silence other apps when an alarm is sounding.

> > So I think we need the following capabilities:
> > 
> > 1. Have multiple audio channels, like "normal", "attention" and "ringer"
> > (but maybe the ringer could simply use "attention").
> > 2. Be able to set the volume for these independently. We can simply use the
> > settings API for this.
> 
> Provide the stream types from Android Audio HAL for reference.
> default, voice call, system, ring, music, alarm, notification, BT SCO,
> enforced audible, DTMF, TTS, FM.

Cool. I think we should start small and only add as needed when we have strong usecases. That's how we traditionally do web standards since it's so hard to remove features once they are added.

But it's definitely good to know where Android ended up.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> Can we simply assign a audio channel type to each *application* (or page)
> instead of each <audio>/<video> element?

I was thinking about that, but I think it might be too restrictive pretty quickly. For example the dialer app will want to play audio using the "attention" channel to sound the ringer, but it'll want to use the "normal" channel when playing the audio from voicemails or from the actual phonecall.

Actually, that makes me realize that we'll likely need the ability for the dialer app to silence other apps even when not sounding the "alert" channel.

Another use-case for multiple channels per app is an email app which wants to be able to play media files that you receive through email, but also be able to sound an alert if you receive an email matching some particular filter.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Marco Chen [:mchen] from comment #14)
> In this situation, I proposed related apps stopped their own audio stream by
> listening the specific event like "call state changing". 
> The reason is
>   Power Consumption: The voice call is most higher priority on phone
> product, so we need keep this behavior as long as possible. Since the other
> audio streams are muted, why do we stop them for saving power.

The more I think about it, the more I think this is a good idea. What we should do is to fire some event when certain channels are muted and unmuted. That way a music app can pause whatever music you were listening to. And an app that wants to notify the user can wait to do that until the audio channels are unmuted.

Comment 20

[Marco Chen [:mchen]]

I listed the points we discussed as below.

1. Since to support adjusting volume on different kinds of audio types is necessary, 
   we need to start what types are mandatory now.
   
   mchen: mandatory: default (system), alarm, force_audible (shutter sound), ring, voice, FM
          optional: music (out from system) , DTMF, TTS          

2. How to apply audio stream types into <audio>?

   mchen: new parameter? <audio streamType="RING">

3. How to check volume setting is successful from Web APP?
   Currently web APP used the event from setting API to confirm this.
   But it just mean setting API fired the event to trigger volume setting but not mean successful. Any new web API for volume setting in synchronization mode?

   mchen: new synchronous API.

4. During voice call, how to stop other ongoing audio stream? (for power saving also)
   a. Leave this as a responsibility on each Apps. So they should listen the event of phone status then stop their own audio stream.
   b. Muting other audio streams then fire a event to notify Apps to stop audio stream.

   mchen: option a.

Comment 21

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

(In reply to Marco Chen [:mchen] from comment #20)
> 2. How to apply audio stream types into <audio>?
> 
>    mchen: new parameter? <audio streamType="RING">

Maybe we should call the attribute mozAudioOutputType

> 4. During voice call, how to stop other ongoing audio stream? (for power
> saving also)
>    a. Leave this as a responsibility on each Apps. So they should listen the
> event of phone status then stop their own audio stream.
>    b. Muting other audio streams then fire a event to notify Apps to stop
> audio stream.
> 
>    mchen: option a.

With option a, we can be sure other apps will do the wrong thing.

How about option c: force other audio streams to pause and then automatically resume them when the call ends.

Comment 22

[Marco Chen [:mchen]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> (In reply to Marco Chen [:mchen] from comment #20)
> With option a, we can be sure other apps will do the wrong thing.

Yes, some apps will but they should be responsible for fixing this kind of bugs.

> 
> How about option c: force other audio streams to pause and then
> automatically resume them when the call ends.

This is a possbile way but I think it is too complicated on handle stream control between gecko & apps. Ex: during phone call, gecko stop the music stream and user tried to control the music app too. As my opinion, gecko should handle the basic & common feature or function for applications not jump into control the apps behavior in the dark.

Comment 23

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

(In reply to Marco Chen [:mchen] from comment #22)
> This is a possbile way but I think it is too complicated on handle stream
> control between gecko & apps. Ex: during phone call, gecko stop the music
> stream and user tried to control the music app too.

That's not so bad. We can just stop the audio element from advancing; we already do this in some cases, e.g. when the audio resource is an HTTP URL fetched over the network and the network stalls. As far as the app is concerned the element is still playing --- not paused --- it's just not making progress at the moment. The user experience will be that the application looks like it's playing, but no audio is coming out. If the user tries to pause and resume the video, the visible pause state will toggle, but audio output won't be affected.

I hope that most users don't try to operate their music player during a phone call, anyway :-).

Comment 24

[Marco Chen [:mchen]]

Attachment: WIPv1 - Web API

In order to adjust volume on audio streams with different types (ex: RING, VOICE, SYSTEM...etc). Add new argument into HTMLAudioElement.

Comment 25

[Marco Chen [:mchen]]

Attachment: Part 1: WebAPI

Add a attribute called mozAudioStreamType into nsIDOMHTMLMediaElement.idl.

1. Feedback for naming of new attribute?
2. Do we just support it by the [1] as below or need to support case of property too [2]?

[1] var aObject = Audio("URI");
    aObject.mozAudioStreamType = "MUSIC"

[2] <audio src="URI" mozAudioStreamType="MUSIC">

Thanks for all your comment in advance.

Comment 26

[Marco Chen [:mchen]]

Attachment: Part2: Impl on path of Audio() from JS

This patch is implemented for 
  1. Case of audio().mozAudioStreamType = 'XXX' -> ... -> audio().play()
     relaying the mozAudioStreamType attribute to AudioTrack class from Android.
  2. Case of audio().mozSetup()

Next part will be path on AudioSegment used by MediaStreamGraph.

If I set to wrong reviewer, please let me know and sorry for this.

Comment 27

[cajbir (:cajbir)]

Comment on attachment 671372 [details] [diff] [review]
Part2: Impl on path of Audio() from JS

Add documentation for what an "Audio Stream Type" is. What are the valid values? Why is it a string? 

I've passed this on to Matthew since it touches the audio code he maintains.

Comment 28

[Marco Chen [:mchen]]

Attachment: Introduction_mozAudioStreamType

This pdf file introduced that why do we need this attribute and how to do it.

Comment 29

[Matthew Gregan [:kinetik]]

Comment on attachment 671372 [details] [diff] [review]
Part2: Impl on path of Audio() from JS

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

General approach looks fine, comments inline.

::: content/html/content/public/nsHTMLMediaElement.h
@@ +406,5 @@
>  
> +  const nsString& GetAudioStreamType()
> +  {
> +    return mAudioStreamType;
> +  }	

Trailing space.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +1537,5 @@
> +nsHTMLMediaElement::GetMozAudioStreamType(nsAString & aMozAudioStreamType)
> +{
> +  aMozAudioStreamType = mAudioStreamType;
> +  return NS_OK;
> +}

New line between this and the setter.

@@ +1732,5 @@
>      mMediaSecurityVerified(false),
>      mCORSMode(CORS_NONE),
>      mHasAudio(false),
> +    mDownloadSuspendedByCache(false),
> +    mAudioStreamType(NS_LITERAL_STRING("DEFAULT"))

It's declared between mLoadWaitStatus and mVolume in the header, so please initialize it in that order to avoid compiler warnings.

::: content/media/nsAudioStream.cpp
@@ +60,5 @@
>    ~nsNativeAudioStream();
>    nsNativeAudioStream();
>  
> +  nsresult Init(int32_t aNumChannels, int32_t aRate,
> +                nsString aAudioStreamType = nsString(NS_LITERAL_STRING("DEFAULT")));

Please don't add a default initializer for this, I'd rather the callers specified the default everywhere.  The same applies for all of the other Init functions.

@@ +446,5 @@
>      PR_LOG(gAudioStreamLog, PR_LOG_ERROR, ("nsNativeAudioStream: sa_stream_create_pcm error"));
>      return NS_ERROR_FAILURE;
>    }
>  
> +  #ifdef MOZ_WIDGET_GONK

Remove MOZ_WIDGET_GONK so this is called on all platforms.  And in libsydneyaudio, add the function with the UNSUPPORTED macro to all of the non-Gonk backends.  You'll need to handle SA_ERROR_NOT_SUPPORTED as non-fatal in that case.

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +984,5 @@
>    // dispatched event has finished (or even started!) running. Methods which
>    // are unsafe to call with the decoder monitor held are documented as such
>    // in nsAudioStream.h.
>    nsRefPtr<nsAudioStream> audioStream = nsAudioStream::AllocateStream();
> +  audioStream->Init(channels, rate, streamType);

Just pass the result of mDecoder->GetAudioStreamType() directly here, rather than creating a temporary variable.

::: content/media/nsMediaDecoder.cpp
@@ +61,5 @@
>  }
>  
> +const nsString& nsMediaDecoder::GetAudioStreamType()
> +{
> +  return mElement->GetAudioStreamType();

Null check mElement.

::: media/libsydneyaudio/include/sydney_audio.h
@@ +291,5 @@
>  
>  /** Normal way to open a PCM device */
>  int sa_stream_create_pcm(sa_stream_t **s, const char *client_name, sa_mode_t mode, sa_pcm_format_t format, unsigned int rate, unsigned int nchannels);
>  
> +#ifdef MOZ_WIDGET_GONK

This is a third party library, so you can't use MOZ_ defines here.  Add it unconditionally, but implement it in all of the non-Gonk backends using the UNSUPPORTED macro, then handle SA_STREAM_ERROR_UNSUPPORTED where this is called.

@@ +292,5 @@
>  /** Normal way to open a PCM device */
>  int sa_stream_create_pcm(sa_stream_t **s, const char *client_name, sa_mode_t mode, sa_pcm_format_t format, unsigned int rate, unsigned int nchannels);
>  
> +#ifdef MOZ_WIDGET_GONK
> +/** Assign audio stream type for argument used by AudioTrack class from Android */

Leave out the Android/AudioTrack part of the comment.  Also, note that this must be called after sa_stream_create_pcm but before sa_stream_open.

::: media/libsydneyaudio/src/sydney_audio_gonk.cpp
@@ +101,5 @@
>  }
>  
> +/* Assign audio stream type for argument used by AudioTrack class */
> +int
> +sa_stream_set_stream_type(sa_stream_t *s, const char *stream_type)

This should fail if the stream is already open, since we pass streamType to the AudioTrack ctor and don't recreate it if it changes later.

@@ +106,5 @@
> +{
> +  switch(stream_type[0])
> +  {
> +    case 'M':
> +      if (!strcmp("MUSIC", stream_type)) {

switch + strcmp is overly clever, just use strcmp.

Comment 30

[Matthew Gregan [:kinetik]]

The other thing that's missing is that a set of accepted string/enum values in the nsIDOMHTMLMediaElement, which can then be mapped to the Android/Gonk/whatever values.  Right now, we're effectively exposing the Android/Gonk names directly, which I don't think is the correct approach.

Comment 31

[Marco Chen [:mchen]]

May I know who or how or procedures to define these accepted string/enum?
Thanks.

Comment 32

[Marco Chen [:mchen]]

Attachment: Part 2: Implementation

Following the comments from feedbacks.

Comment 33

[Matthew Gregan [:kinetik]]

Do we need to restrict mozAudioStreamType changes to privileged apps?  It'd be really bad if advertisement/malicious sites set mozAudioStreamType to ENFORCED_AUDIBLE or something equally high priority.

Thanks for making the changes in the patch.  Thinking about what we discussed on IRC some more, I'd prefer to see the stream types abstracted more.  So nsHTMLMediaElement::SetMozAudioStreamType accepts one of a document list of strings, and converts them to a new enum added to nsAudioStream.  Then each nsAudioStream converts that enum to a sydneyaudio/cubeb specific enum that we'll need to add to each (well, just sydneyaudio for now, since you're only touching the sydneyaudio gonk backend).

Comment 34

[Marco Chen [:mchen]]

Hi Matthew,

Thanks for your suggestion.
1. For privilege control,it should be considered but I will put it after first landing.
2. For streamType, it will be discussed here on super review process.

Comment 35

[Marco Chen [:mchen]]

Attachment: Part 1: We

Comment 36

[Marco Chen [:mchen]]

Attachment: Part 1: WebAPI - WIPv2

1. Add new attribute called - DOMString mozAudioStreamType in nsIDOMHTMLMediaElement.idl.
2. Update UUID of nsIDOMHTMLMediaElement.idl, nsIDOMHTMLAudioElement.idl and nsIDOMHTMLVideoElement.idl.

Comment 37

[Marco Chen [:mchen]]

Dear Sicking,

Thanks for your help on this case in the first.
And if you have any concerns maybe you can post here for discussion.
Or what I should do more to help you. Thanks.

Comment 38

[Chris Lee [:clee]]

Jonas, can you take a look at this when you have a chance in the next day or so?  Thanks.

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 672698 [details] [diff] [review]
Part 1: WebAPI - WIPv2

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

This looks fine, however I don't think it gets us as far as we need when it comes to audio management. I've started drafting a new API for this that I'll put up on the wiki shortly. In the meantime we should definitely take this though (though I might have named it "mozaudiochannel" instead, but it's not a big deal either way)

Comment 40

[Alex Keybl [:akeybl]]

We're marking this bug with the C1 milestone since it follows the criteria of "unfinished feature work" (see https://etherpad.mozilla.org/b2g-convergence-schedule).

If this work is not finished by Nov19, this bug will need an exception and will be called out at the upcoming Exec Review.

Comment 41

[Marco Chen [:mchen]]

Attachment: Part 1: WebAPI - WIPv3

After discussed with Jonas, Baku and Robert for bug 805333, we split a part of patch from Baku for bug 805333 then do the following stuff on this bug.

1. The attribute's name is changed to mozAudioChannelType.
2. The possible channel types are listed on idl file. And refer to link as below for detail. 
  https://etherpad.mozilla.org/audio-bug80533

Comment 42

[Marco Chen [:mchen]]

Attachment: Part 2: Implementation WIPv3

1. Following the Baku's patch from bug 805333 which transfer DOMString from new attribute to AudioChannelType enum value.

2. Transfer the AudioChannelType enum to sa_stream_type_t enum in nsAudioStream then pass into Syndey library.

3. Syndey with gonk version will transfer sa_stream_type_t to Android stream types then pass into AudioTrack class.

Comment 43

[Matthew Gregan [:kinetik]]

Comment on attachment 680956 [details] [diff] [review]
Part 1: WebAPI - WIPv3

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

r+ only if this is intended to be mozAudioChannelType.  If it's intended to be mozAudioChannel, I think we need to find a better name.

::: dom/interfaces/html/nsIDOMHTMLMediaElement.idl
@@ +110,5 @@
>    // the media element has a fragment URI for the currentSrc, otherwise
>    // it is equal to the media duration.
>    readonly attribute double mozFragmentEnd;
> +
> +   // Mozilla extension: an audio channel for media elements.

I think calling this mechanism an "audio channel" is confusing given the existing concept of audio channels in media.

I'm slight confused, because the comment associated with this attachment says that it was renamed to mozAudioChannelType, which is a better name.

@@ +142,5 @@
> +   attribute DOMString mozAudioChannel;
> +
> +   // In addition the media element has this new events:
> +   // * onmozpaused - called when the media element has been paused
> +   // * onmozunpaused - called when the media element has been unpaused

This part doesn't appear to be implemented, please remove the comment from the IDL.

Comment 44

[Matthew Gregan [:kinetik]]

Comment on attachment 680957 [details] [diff] [review]
Part 2: Implementation WIPv3

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

Where is AudioChannelCommon.h added?

My comments about mozAudioChannel vs mozAudioChannelType apply here too.

What's expected to happen if mozAudioChannelType is changed after the element/decoder is set up? Either we need to handle dynamic changes, or return an error from the setter if it's too late to change it.

::: content/html/content/public/nsHTMLMediaElement.h
@@ +323,5 @@
> +   */
> +  mozilla::dom::AudioChannelType GetMozAudioChannel()
> +  {
> +    return mMozAudioChannel;
> +  }

Drop the "Moz" part from the member function name and member variable name, the prefix is only for the Web-visible part of the API.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +3699,5 @@
> +      break;
> +    case AUDIO_CHANNEL_PUBLICNOTIFICATION:
> +      aString.AssignLiteral("publicnotification");
> +      break;
> +    case AUDIO_CHANNEL_UNKNOWN:

Remove this case.  The member variable should only be set to valid values.  The setter can return an error for invalid values.

@@ +3724,5 @@
> +  } else if (aString.EqualsASCII("telephony")) {
> +    audioChannel = AUDIO_CHANNEL_TELEPHONY;
> +  } else if (aString.EqualsASCII("publicnotification")) {
> +    audioChannel = AUDIO_CHANNEL_PUBLICNOTIFICATION;
> +  }

Set mAudioChannel directly in each of these, and return an error for unknown/invalid from an else block.

::: content/media/nsAudioStream.cpp
@@ +351,5 @@
>    return gCubebLatency;
>  }
>  #endif
>  
> +static sa_stream_type_t CovertChannelToSAType(AudioChannelType aType)

Typo: Convert.

@@ +377,5 @@
> +    break;
> +  default:
> +    NS_ERROR("The value of AudioChannelType is invalid");
> +    break;
> +  }

You can just return directly from each case, it makes the code smaller.

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +986,5 @@
>    // in nsAudioStream.h.
>    nsRefPtr<nsAudioStream> audioStream = nsAudioStream::AllocateStream();
> +  // If media element is null then there is no way to know audio stream type.
> +  // So assign it to NORMAL.
> +  if (!mDecoder->GetMediaOwner()->GetMediaElement()) {

This is not thread-safe.  The decoder must not be called without the decoder monitor held.  The element does not expect to be called from non-main threads at all.

If we're not going to support dynamic changes, rather than have GetAudioChannelType on the media element, either the element setter could call a setter on the decoder or pass the value in when the decoder is initialized.  The value can then be accessed from the decoder by the reader with the decoder monitor held.

Also, GetMediaOwner can return null.

::: dom/Makefile.in
@@ +73,5 @@
>    ipc \
>    identity \
>    workers \
>    camera \
> +  audiochannel \

This doesn't seem to belong in this patch.

::: media/libsydneyaudio/include/sydney_audio.h
@@ +307,5 @@
>  
>  /** Normal way to open a PCM device */
>  int sa_stream_create_pcm(sa_stream_t **s, const char *client_name, sa_mode_t mode, sa_pcm_format_t format, unsigned int rate, unsigned int nchannels);
>  
> +/** Assign audio stream type */

Per my last review comment, it needs to be documented that this can only be called after create and before open.

::: media/libsydneyaudio/src/sydney_audio_gonk.cpp
@@ +103,5 @@
> +/* Assign audio stream type for argument used by AudioTrack class */
> +int
> +sa_stream_set_stream_type(sa_stream_t *s,  const sa_stream_type_t stream_type)
> +{
> +  if (s->output_unit != NULL) {

What if output_unit is non-NULL but it's too late to change streamType?

Comment 45

[Marco Chen [:mchen]]

(In reply to Matthew Gregan [:kinetik] from comment #43)
> Comment on attachment 680956 [details] [diff] [review]
> Part 1: WebAPI - WIPv3
> 
> Review of attachment 680956 [details] [diff] [review]:
> -----------------------------------------------------------------
> I'm slight confused, because the comment associated with this attachment
> says that it was renamed to mozAudioChannelType, which is a better name.
> 

The comment is about adding a enum of AudioChannelType which will be converted from new new attribute. But I think the mozAudioChannelType is a better one too.

> @@ +142,5 @@
> > +   attribute DOMString mozAudioChannel;
> > +
> > +   // In addition the media element has this new events:
> > +   // * onmozpaused - called when the media element has been paused
> > +   // * onmozunpaused - called when the media element has been unpaused
> 
> This part doesn't appear to be implemented, please remove the comment from
> the IDL.

Yes, thanks for reminding.

Comment 46

[Marco Chen [:mchen]]

(In reply to Matthew Gregan [:kinetik] from comment #44)
> Comment on attachment 680957 [details] [diff] [review]
> Part 2: Implementation WIPv3
> 
> Review of attachment 680957 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Where is AudioChannelCommon.h added?

Lost the new files during "hg qrefresh". Will update them.

> 
> What's expected to happen if mozAudioChannelType is changed after the
> element/decoder is set up? Either we need to handle dynamic changes, or
> return an error from the setter if it's too late to change it.
> 

I prefer to return an error from the setter and done in next version.

> ::: content/html/content/public/nsHTMLMediaElement.h
> @@ +323,5 @@
> > +   */
> > +  mozilla::dom::AudioChannelType GetMozAudioChannel()
> > +  {
> > +    return mMozAudioChannel;
> > +  }
> 

Remove this function because will set mMozAudioChannelType to mDecoder during initilization.

> ::: content/html/content/src/nsHTMLMediaElement.cpp
> @@ +3699,5 @@
> > +      break;
> > +    case AUDIO_CHANNEL_PUBLICNOTIFICATION:
> > +      aString.AssignLiteral("publicnotification");
> > +      break;
> > +    case AUDIO_CHANNEL_UNKNOWN:
> 
> Remove this case.  The member variable should only be set to valid values. 
> The setter can return an error for invalid values.
> 

Done in next version.

> @@ +3724,5 @@
> > +  } else if (aString.EqualsASCII("telephony")) {
> > +    audioChannel = AUDIO_CHANNEL_TELEPHONY;
> > +  } else if (aString.EqualsASCII("publicnotification")) {
> > +    audioChannel = AUDIO_CHANNEL_PUBLICNOTIFICATION;
> > +  }
> 
> Set mAudioChannel directly in each of these, and return an error for
> unknown/invalid from an else block.
> 

Done in next version.

> ::: content/media/nsAudioStream.cpp
> @@ +351,5 @@
> >    return gCubebLatency;
> >  }
> >  #endif
> >  
> > +static sa_stream_type_t CovertChannelToSAType(AudioChannelType aType)
> 
> Typo: Convert.
> 

Done in next version.

> @@ +377,5 @@
> > +    break;
> > +  default:
> > +    NS_ERROR("The value of AudioChannelType is invalid");
> > +    break;
> > +  }
> 
> You can just return directly from each case, it makes the code smaller.
> 

Done in next version

> ::: content/media/nsBuiltinDecoderStateMachine.cpp
> @@ +986,5 @@
> >    // in nsAudioStream.h.
> >    nsRefPtr<nsAudioStream> audioStream = nsAudioStream::AllocateStream();
> > +  // If media element is null then there is no way to know audio stream type.
> > +  // So assign it to NORMAL.
> > +  if (!mDecoder->GetMediaOwner()->GetMediaElement()) {
> 
> This is not thread-safe.  The decoder must not be called without the decoder
> monitor held.  The element does not expect to be called from non-main
> threads at all.
> 

In next version, will protect this by monitor held and assigned audio channel type into decoder instance during it's initialization. So will not call element here.

> If we're not going to support dynamic changes, rather than have
> GetAudioChannelType on the media element, either the element setter could
> call a setter on the decoder or pass the value in when the decoder is
> initialized.  The value can then be accessed from the decoder by the reader
> with the decoder monitor held.
> 
> Also, GetMediaOwner can return null.
> 

I prefer not to support dynamic change and the same reply with comment above.

> ::: dom/Makefile.in
> @@ +73,5 @@
> >    ipc \
> >    identity \
> >    workers \
> >    camera \
> > +  audiochannel \
> 
> This doesn't seem to belong in this patch.
> 

Update in next version

> ::: media/libsydneyaudio/include/sydney_audio.h
> @@ +307,5 @@
> >  
> >  /** Normal way to open a PCM device */
> >  int sa_stream_create_pcm(sa_stream_t **s, const char *client_name, sa_mode_t mode, sa_pcm_format_t format, unsigned int rate, unsigned int nchannels);
> >  
> > +/** Assign audio stream type */
> 
> Per my last review comment, it needs to be documented that this can only be
> called after create and before open.
> 

Done in next version.

> ::: media/libsydneyaudio/src/sydney_audio_gonk.cpp
> @@ +103,5 @@
> > +/* Assign audio stream type for argument used by AudioTrack class */
> > +int
> > +sa_stream_set_stream_type(sa_stream_t *s,  const sa_stream_type_t stream_type)
> > +{
> > +  if (s->output_unit != NULL) {
> 
> What if output_unit is non-NULL but it's too late to change streamType?

The streamType MUST be assigned before calling sa_stream_open() because it will be used to create AudioTrack instance. Once AudioTrack (output_unit) is created already then sa_stream_set_stream_type() will return an error.

Comment 47

[Marco Chen [:mchen]]

Attachment: Part 1: WebAPI - WIPv4

Follow the review feedback.

Comment 48

[Marco Chen [:mchen]]

Attachment: Part 2: Implementation WIPv4

Follow the review feedback.

Comment 49

[Matthew Gregan [:kinetik]]

Comment on attachment 681408 [details] [diff] [review]
Part 2: Implementation WIPv4

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

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +3700,5 @@
> +      break;
> +    case AUDIO_CHANNEL_PUBLICNOTIFICATION:
> +      aString.AssignLiteral("publicnotification");
> +      break;
> +    default:

default case can be left out.

@@ +3708,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsHTMLMediaElement::SetMozAudioChannelType(const nsAString& aString)

The IDL mentions permissions (e.g. B2G manifest).  Is the plan to add the checks for this later, or elsewhere?

::: content/media/nsAudioStream.cpp
@@ +375,5 @@
> +      break;
> +    default:
> +      NS_ERROR("The value of AudioChannelType is invalid");
> +      return SA_STREAM_TYPE_MAX;
> +      break;

You don't need the break in all these cases, the return is sufficient.

::: dom/audiochannel/AudioChannelCommon.h
@@ +18,5 @@
> +  AUDIO_CHANNEL_NOTIFICATION,
> +  AUDIO_CHANNEL_ALARM,
> +  AUDIO_CHANNEL_TELEPHONY,
> +  AUDIO_CHANNEL_PUBLICNOTIFICATION,
> +  AUDIO_CHANNEL_UNKNOWN = -1

Remove unknown now that it's unused.

::: dom/audiochannel/Makefile.in
@@ +1,1 @@
> +# Copyright 2012 Mozilla Foundation and Mozilla contributors

Is there a plan to add more files to this directory?  If it's only ever going to contain a single item, it'd be nice to avoid a whole new dir for it--maybe use dom/media in that case?

::: media/libsydneyaudio/src/sydney_audio_gonk.cpp
@@ +141,5 @@
> +      s->streamType = AudioSystem::FM;
> +      break;
> +    default:
> +      return SA_ERROR_INVALID;
> +      break;

No need for break after return.

Comment 50

[Marco Chen [:mchen]]

Dear kinetik,

Thanks for your great help on this issue in advance.\

The answer is yes about plan of permission and I am working on it now.
Due to the new channels field in audio permission is introduced in link as below.
https://wiki.mozilla.org/WebAPI/AudioChannels
I need to modify PermissionInstaller.jsm but not only add a permission name only.

So could you give some advice about whether I can land this first then follow the permission work? Because some of bugs are waiting for this.

Thanks.

Comment 51

[Matthew Gregan [:kinetik]]

(In reply to Marco Chen [:mchen] from comment #50)
> So could you give some advice about whether I can land this first then
> follow the permission work? Because some of bugs are waiting for this.

I think that's fine, this will only affect B2G at the moment.

Comment 52

[Marco Chen [:mchen]]

(In reply to Matthew Gregan [:kinetik] from comment #49)
> Comment on attachment 681408 [details] [diff] [review]
> Part 2: Implementation WIPv4
> 
> Review of attachment 681408 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/nsHTMLMediaElement.cpp
> @@ +3700,5 @@
> > +      break;
> > +    case AUDIO_CHANNEL_PUBLICNOTIFICATION:
> > +      aString.AssignLiteral("publicnotification");
> > +      break;
> > +    default:
> 
> default case can be left out.
> 

Done in next version.

> @@ +3708,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +NS_IMETHODIMP
> > +nsHTMLMediaElement::SetMozAudioChannelType(const nsAString& aString)
> 
> The IDL mentions permissions (e.g. B2G manifest).  Is the plan to add the
> checks for this later, or elsewhere?
> 

Plan to add by another patch and it will be involved by bug 805333.
Please refer to the comment 42 from that bug. 

> ::: content/media/nsAudioStream.cpp
> @@ +375,5 @@
> > +      break;
> > +    default:
> > +      NS_ERROR("The value of AudioChannelType is invalid");
> > +      return SA_STREAM_TYPE_MAX;
> > +      break;
> 
> You don't need the break in all these cases, the return is sufficient.
> 

Done in next version.

> ::: dom/audiochannel/AudioChannelCommon.h
> @@ +18,5 @@
> > +  AUDIO_CHANNEL_NOTIFICATION,
> > +  AUDIO_CHANNEL_ALARM,
> > +  AUDIO_CHANNEL_TELEPHONY,
> > +  AUDIO_CHANNEL_PUBLICNOTIFICATION,
> > +  AUDIO_CHANNEL_UNKNOWN = -1
> 
> Remove unknown now that it's unused.
> 

Done in next version.

> ::: dom/audiochannel/Makefile.in
> @@ +1,1 @@
> > +# Copyright 2012 Mozilla Foundation and Mozilla contributors
> 
> Is there a plan to add more files to this directory?  If it's only ever
> going to contain a single item, it'd be nice to avoid a whole new dir for
> it--maybe use dom/media in that case?
> 

There will be a AudioChannelService.cpp/h introduced by bug 805333, so just keep it there.

> ::: media/libsydneyaudio/src/sydney_audio_gonk.cpp
> @@ +141,5 @@
> > +      s->streamType = AudioSystem::FM;
> > +      break;
> > +    default:
> > +      return SA_ERROR_INVALID;
> > +      break;
> 
> No need for break after return.

Done in next version.

Comment 53

[Marco Chen [:mchen]]

(In reply to Matthew Gregan [:kinetik] from comment #51)
> (In reply to Marco Chen [:mchen] from comment #50)
> > So could you give some advice about whether I can land this first then
> > follow the permission work? Because some of bugs are waiting for this.
> 
> I think that's fine, this will only affect B2G at the moment.

I discussed with Baku on Bug 805333 and from comment 42 he already do it on his patch.
So I will start to do final checking and try server then check in.
And I noticed bug 811381, it seems to effect my patch. (verifying)

Comment 54

[Marco Chen [:mchen]]

Try server result.

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

Comment 55

[Marco Chen [:mchen]]

Attachment: Part 1: WebAPI - Checkin Version

Add reviewer is kinetik and ask for checkin-needed

Comment 56

[Marco Chen [:mchen]]

Attachment: Part 2: Implementation Checkin Version

Add reviewer is Kinetik and ask for checkin-needed.

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76584699a51c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5790e9dcb69

Should this have tests?

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76584699a51c
https://hg.mozilla.org/mozilla-central/rev/f5790e9dcb69

Comment 59

[Ryan VanderMeulen [:RyanVM]]

This bug is marked blocking-basecamp+ but these patches do not apply cleanly to Aurora (due to some refactoring that happened on m-c, I believe). Please post branch-specific patches here or request uplift approval on whatever these patches depend on.

Comment 60

[Marco Chen [:mchen]]

Attachment: Part 1: WebAPI - Aurora (branch-specific patch)

Branch-specific patch of "Part 1: WebAPI" for aurora branch.

Comment 61

[Marco Chen [:mchen]]

Attachment: Part 2: Impl - Aurora (branch-specific patch)

Branch-specific version of "Part 2: Impl" for aurora branch.

Comment 62

[Marco Chen [:mchen]]

Attachment: Part 2: Impl - Aurora (branch-specific patch)

Branch-specific version of "Part 2: Impl" for aurora branch.

Comment 63

[Ryan VanderMeulen [:RyanVM]]

Thank you good sir!

https://hg.mozilla.org/releases/mozilla-aurora/rev/e146d665ab56
https://hg.mozilla.org/releases/mozilla-aurora/rev/8e00ce1788c7

Comment 64

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 682384 [details] [diff] [review]
Part 1: WebAPI - Checkin Version

FYI, This patch must not land to beta. No interface changes.
If you need to add new property to an element, you have to create a new
interface and make the element to implement it.

Comment 65

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Ryan VanderMeulen from comment #57)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/76584699a51c
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f5790e9dcb69
> 
> Should this have tests?
Yes. This must get some tests if enabled by default in m-c.

Comment 66

[Marco Chen [:mchen]]

(In reply to Olli Pettay [:smaug] from comment #64)
> Comment on attachment 682384 [details] [diff] [review]
> Part 1: WebAPI - Checkin Version
> 
> FYI, This patch must not land to beta. No interface changes.
> If you need to add new property to an element, you have to create a new
> interface and make the element to implement it.

Thanks for the information. After discussing with Andrea, he will move related codes into new interface (maybe nsIDOMHTMLMediaElement2 which inherits from nsIDOMHTMLMediaElement) and merge the current working on bug 805333.

Comment 67

[Marco Chen [:mchen]]

This really should provide the test cases and will prepare it.(In reply to Olli Pettay [:smaug] from comment #65)
> (In reply to Ryan VanderMeulen from comment #57)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/76584699a51c
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/f5790e9dcb69
> > 
> > Should this have tests?
> Yes. This must get some tests if enabled by default in m-c.

This really should provide the test cases and will prepare it.

Comment 68

[cajbir (:cajbir)]

(In reply to Marco Chen [:mchen] from comment #66)

> Thanks for the information. After discussing with Andrea, he will move
> related codes into new interface (maybe nsIDOMHTMLMediaElement2 which
> inherits from nsIDOMHTMLMediaElement) and merge the current working on bug
> 805333.

Won't that approach effectively change the interfaces of nsIDOMHTMLVideoElement and nsIDOMHTMLAudioElement since they derive from nsIDOMHTMLMediaElement and would need to change to nsIDOMHTMLMediaElement2 to pick up the new attributes?

Comment 69

[Marco Chen [:mchen]]

(In reply to Chris Double (:doublec) from comment #68)
> (In reply to Marco Chen [:mchen] from comment #66)
> Won't that approach effectively change the interfaces of
> nsIDOMHTMLVideoElement and nsIDOMHTMLAudioElement since they derive from
> nsIDOMHTMLMediaElement and would need to change to nsIDOMHTMLMediaElement2
> to pick up the new attributes?

Thanks Chris's kindly reminding here. And please give any feedbacks into bug 805333 for this because Andrea will do it on that bug. Of course I will forward this message into that bug.

Comment 70

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Olli Pettay [:smaug] from comment #64)
> Comment on attachment 682384 [details] [diff] [review]
> Part 1: WebAPI - Checkin Version
> 
> FYI, This patch must not land to beta. No interface changes.
> If you need to add new property to an element, you have to create a new
> interface and make the element to implement it.

This patch is already on beta since it was landed on aurora before we migrated aurora->beta. So we are totally fine here. The change was good.

Additionally, we're making a lot of exceptions for B2G in order to manage to ship. And we also has the option to branch B2G from beta since that will have to happen soon anyway. So please check with someone before making comments like the above.

Comment 71

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jonas Sicking (:sicking) from comment #70)
> 
> This patch is already on beta since it was landed on aurora before we
> migrated aurora->beta. So we are totally fine here. The change was good.
Argh, sorry. I totally managed to look wrong the landing date. I thought this wasn't
in beta yet. Sorry.

Comment 72

[Marco Chen [:mchen]]

The test case is already landed by bug 817043 (content/html/content/test/test_mozaudiochannel.html). Thanks for Andrea's help.