870682 - [gonk-jb] To add basic support for audio control API.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Marco Chen [:mchen]]

The audio control API would include
  1. Routing
  2. Volume Control
  3. Phone State Change
  4. Headset detection.

Comment 1

[Marco Chen [:mchen]]

Temporary hack on JB412 version.

https://github.com/marcofreda527/jb412gecko/commit/6f268e3b99274431b61a2ac6ea6f106b155d4470

Comment 2

[Michael Wu [:mwu]]

Is anyone working on this? This is the last thing that prevents gecko from building on JB so I'd like to either fix this bug or just disable audio configuration on JB so we can get things compiling. If this won't take too long to fix, we can wait, but if not, we should put in a few things to make things build until we can fix it properly.

Comment 3

[cheng.luo]

Attachment: AudioManager adapt for JB

Comment 4

[cheng.luo]

Attachment: libsydneyaudio for JB

Comment 5

[Marco Chen [:mchen]]

(In reply to cheng.luo from comment #4)
> Created attachment 750221 [details] [diff] [review]
> libsydneyaudio for JB

Hi Cheng,

In first, thanks for your contribution on this bug.
And for patch of "libsydneyaudio for JB", we already drop libsydney in our main stream (Mozilla-Central). It is replaced by libCubeb + OpenSL ES (not libsydney + AudioTrack). And it seems to work well on JB 4.2.

For AudioManager, we will have a review on it. Thanks.

Comment 6

[cheng.luo]

(In reply to Michael Wu [:mwu] from comment #2)
> Is anyone working on this? 
I had try it.I think it it not very difficult just only basic support such as adjust volume.How to support ICS and JB on same version is a more bigger headache. what you suggestion?

Comment 7

[cheng.luo]

(In reply to Marco Chen [:mchen] from comment #5)
> (In reply to cheng.luo from comment #4)
> for JB", we already drop libsydney in our main stream (Mozilla-Central). It
> is replaced by libCubeb + OpenSL ES (not libsydney + AudioTrack). And it
> seems to work well on JB 4.2.

Will B2G use mediaserver process on JB?

Comment 8

[Marco Chen [:mchen]]

Yes, the audio playback is still going though AudioFlinger. And actually OpenSL ES is a wrapper on AudioTrack.

Comment 9

[Marco Chen [:mchen]]

Hi Michael,

I will try to give a version merged from comment 1 & 3 today (Taipei time).

Hi Cheng,

Would you mind I rebase you patch & mine from comment 1 for AudioManager to gecko version on our JB branch?

Thanks.

Comment 10

[cheng.luo]

Would you mind I rebase you patch & mine from
> comment 1 for AudioManager to gecko version on our JB branch?

OK,That is my wish.

Comment 11

[Marco Chen [:mchen]]

Attachment: Patch AudioManager JB v1

To adapt some functions to JB version.
  setStreamVolumeIndex() 
  getStreamVolumeIndex
  setPhoneState()

Have a reference to comment 3 from cheng & comment 1.

Comment 12

[cheng.luo]

(In reply to Marco Chen [:mchen] from comment #11)
> Created attachment 750329 [details] [diff] [review]
1、I think the third argument of setStreamVolumeIndex ought not to be always AUDIO_DEVICE_OUT_SPEAKER.That will cause audio device route change fail.
2、Android 4.1 which ANDROID_VERSION = 16 also called JB will running on branch of ANDROID_VERSION < 17 in the patch. The results could be disastrous.I think 16 is more reasonable.

Comment 13

[Marco Chen [:mchen]]

(In reply to cheng.luo from comment #12)
> (In reply to Marco Chen [:mchen] from comment #11)
> > Created attachment 750329 [details] [diff] [review]
> 1、I think the third argument of setStreamVolumeIndex ought not to be always
> AUDIO_DEVICE_OUT_SPEAKER.That will cause audio device route change fail.

Hi Cheng,

Thanks for your follow up and this is the patch for basic support only.
And we really need to plan how to adapt it well and full.

No matter we set volume index to speaker only or current device that are no correct solution right now. So I just keep it simple to have a basic support only.

> 2、Android 4.1 which ANDROID_VERSION = 16 also called JB will running on
> branch of ANDROID_VERSION < 17 in the patch. The results could be
> disastrous.I think 16 is more reasonable.

Currently, this bug is used to support "Bug 784227 - (gonk-jb) JB Gonk Support (Android 4.2)". so we only consider whether Android version is JB 4.2 or not.

Comment 14

[Michael Wu [:mwu]]

Comment on attachment 750329 [details] [diff] [review]
Patch AudioManager JB v1

Can you make this patch against mozilla-central? It looks like it's against my jb fork, but most of the patches there have landed except for an AudioManager fix.

Comment 15

[Marco Chen [:mchen]]

Attachment: Patch on m-c v1

Rebase it to m-c.

Comment 16

[Marco Chen [:mchen]]

Attachment: Patch on m-c v2

To fix the wrong variable name on setPhoneState.

Comment 17

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Macro, 
The function parameter's line break should aline the left bracket.
Extra line between 
+    AUDIO_MODE_IN_COMMUNICATION = 3,
+
+    AUDIO_MODE_CNT,
BTW, It seems we have also find out the solution for setStreamVolumeIndex on device.

Comment 18

[Marco Chen [:mchen]]

Attachment: Patch on m-c v3

Thanks for Randy's reminding about nits issue.

And as your comment, we really need to fire a follow up bug for solving the new API which can support to adjust volume on different device.

Comment 19

[Marco Chen [:mchen]]

Attachment: Patch on m-c v4

Comment 20

[Michael Wu [:mwu]]

Comment on attachment 750957 [details] [diff] [review]
Patch on m-c v4

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

Thanks!

::: dom/system/gonk/AudioManager.cpp
@@ +269,5 @@
> +  AudioSystem::setStreamVolumeIndex(
> +    static_cast<audio_stream_type_t>(AUDIO_STREAM_ENFORCED_AUDIBLE),
> +    sMaxStreamVolumeTbl[AUDIO_STREAM_ENFORCED_AUDIBLE],
> +    AUDIO_DEVICE_OUT_SPEAKER);
> +#endif

It looks like we do this frequently enough that we'll want some sort of wrapper function rather than putting ifdefs all over the code. We can do this in the follow up to properly support JB audio though.

Comment 21

[Marco Chen [:mchen]]

Got it. And actually I want to fire a bug to refine the AudioManager.cpp. The target will be 
  1. to remove GB code.
  2. to remove AudioManager::setFMVolume (which is not a standard API on Android ICS but chip vendor's API). 
  3. To re-naming the AudioManager's API, the current names are too dependent on Android but general for FxOS.
  4. Discuss how to utilize out device Based API for volume control.

Comment 22

[Michael Wu [:mwu]]

Sounds good to me.

Comment 23

[Marco Chen [:mchen]]

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

Comment 24

[Marco Chen [:mchen]]

Attachment: Patch on m-c Checkin Version

Add reviewer name.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/81c7af2e48d9

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/81c7af2e48d9