Closed
Bug 870682
Opened 12 years ago
Closed 12 years ago
[gonk-jb] To add basic support for audio control API.
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mchen, Assigned: mchen)
References
Details
Attachments
(3 files, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
mchen
:
review+
|
Details | Diff | Splinter Review |
The audio control API would include 1. Routing 2. Volume Control 3. Phone State Change 4. Headset detection.
Assignee | ||
Comment 1•12 years ago
|
||
Temporary hack on JB412 version. https://github.com/marcofreda527/jb412gecko/commit/6f268e3b99274431b61a2ac6ea6f106b155d4470
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 5•12 years ago
|
||
(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?
Assignee | ||
Comment 8•12 years ago
|
||
Yes, the audio playback is still going though AudioFlinger. And actually OpenSL ES is a wrapper on AudioTrack.
Assignee | ||
Comment 9•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 11•12 years ago
|
||
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)
Comment 12•12 years ago
|
||
(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.
Assignee | ||
Comment 13•12 years ago
|
||
(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•12 years ago
|
||
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.
Assignee | ||
Comment 15•12 years ago
|
||
Rebase it to m-c.
Attachment #750329 -
Attachment is obsolete: true
Attachment #750329 -
Flags: review?(mwu)
Attachment #750880 -
Flags: review?(mwu)
Assignee | ||
Comment 16•12 years ago
|
||
To fix the wrong variable name on setPhoneState.
Attachment #750880 -
Attachment is obsolete: true
Attachment #750880 -
Flags: review?(mwu)
Attachment #750950 -
Flags: review?(mwu)
Comment 17•12 years ago
|
||
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.
Assignee | ||
Comment 18•12 years ago
|
||
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)
Assignee | ||
Comment 19•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #750957 -
Attachment description: Patch on m-c v3 → Patch on m-c v4
Attachment #750957 -
Attachment is patch: true
Assignee | ||
Updated•12 years ago
|
Attachment #750957 -
Flags: review?(mwu)
Assignee | ||
Updated•12 years ago
|
Attachment #750956 -
Attachment is obsolete: true
Attachment #750956 -
Flags: review?(mwu)
Comment 20•12 years ago
|
||
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+
Assignee | ||
Comment 21•12 years ago
|
||
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.
Assignee | ||
Comment 24•12 years ago
|
||
Add reviewer name.
Attachment #750957 -
Attachment is obsolete: true
Attachment #751587 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 25•12 years ago
|
||
https://hg.mozilla.org/projects/birch/rev/81c7af2e48d9
Keywords: checkin-needed
Comment 26•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/81c7af2e48d9
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•