Closed Bug 870682 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mchen, Assigned: mchen)

References

Details

Attachments

(3 files, 5 obsolete files)

The audio control API would include
  1. Routing
  2. Volume Control
  3. Phone State Change
  4. Headset detection.
Blocks: gonk-jb
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.
(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.
(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?
(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?
Yes, the audio playback is still going though AudioFlinger. And actually OpenSL ES is a wrapper on AudioTrack.
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.
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.
Attached patch Patch AudioManager JB v1 (obsolete) — Splinter Review
To adapt some functions to JB version.
  setStreamVolumeIndex() 
  getStreamVolumeIndex
  setPhoneState()

Have a reference to comment 3 from cheng & comment 1.
Assignee: nobody → mchen
Attachment #750329 - Flags: review?(mwu)
(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.
(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 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.
Attached patch Patch on m-c v1 (obsolete) — Splinter Review
Rebase it to m-c.
Attachment #750329 - Attachment is obsolete: true
Attachment #750329 - Flags: review?(mwu)
Attachment #750880 - Flags: review?(mwu)
Attached patch Patch on m-c v2 (obsolete) — Splinter Review
To fix the wrong variable name on setPhoneState.
Attachment #750880 - Attachment is obsolete: true
Attachment #750880 - Flags: review?(mwu)
Attachment #750950 - Flags: review?(mwu)
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.
Attached patch Patch on m-c v3 (obsolete) — Splinter Review
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.
Attachment #750950 - Attachment is obsolete: true
Attachment #750950 - Flags: review?(mwu)
Attachment #750956 - Flags: review?(mwu)
Attached patch Patch on m-c v4 (obsolete) — Splinter Review
Attachment #750957 - Attachment description: Patch on m-c v3 → Patch on m-c v4
Attachment #750957 - Attachment is patch: true
Attachment #750957 - Flags: review?(mwu)
Attachment #750956 - Attachment is obsolete: true
Attachment #750956 - Flags: review?(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.
Attachment #750957 - Flags: review?(mwu) → review+
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.
Sounds good to me.
Add reviewer name.
Attachment #750957 - Attachment is obsolete: true
Attachment #751587 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/81c7af2e48d9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.