1105452 - [gonk-l] Need to use new Audio system APIs for audio offload playback

Status: RESOLVED FIXED

(Firefox OS Graveyard :: AudioChannel, defect)

Description

[Jay]

Attachment: PATCH_81751_Resolve-the-build-failure-caused-by-API-changes_20141126.tar.gz

There are changes on accessing the session ID through Audio system API and canOffloadStream() interface in L-release.

In addition, syscall 256 is added to sandbox filter. Without it, the music app crashes due to seccomp violation.

Attached file has the suggested change for this.

Comment 1

[Jay]

Attachment: 0001-Resolve-the-build-failure-caused-by-API-changes.patch

Comment 2

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

Comment on attachment 8529272 [details] [diff] [review]
0001-Resolve-the-build-failure-caused-by-API-changes.patch

Hi roc, 
Could you review this?
Thanks.

Comment 3

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

Comment on attachment 8529272 [details] [diff] [review]
0001-Resolve-the-build-failure-caused-by-API-changes.patch

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

r+ on the media change.

The sandbox changes need review from someone else.

Comment 4

[Boris Chiou [:boris]]

Comment on attachment 8529272 [details] [diff] [review]
0001-Resolve-the-build-failure-caused-by-API-changes.patch

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

::: dom/media/omx/MediaOmxCommonReader.cpp
@@ +71,5 @@
>        isNotStreaming, mAudioChannel));
>  
>    if ((meta.get()) && hasNoVideo && isNotStreaming && isTypeMusic &&
> +#if ANDROID_VERSION >= 21
> +      canOffloadStream(meta, false, NULL, false, AUDIO_STREAM_MUSIC)) {

I have a problem about this API. I check the AOSP (5.0.0_r2): 
http://androidxref.com/5.0.0_r2/xref/frameworks/av/include/media/stagefright/Utils.h#63

This API only has 4 arguments. Which one should we use? Looks like b2g-l branch use 4 arguments now.

Comment 5

[Michael Vines [:m1] [:evilmachines]]

(In reply to Boris Chiou [:boris] from comment #4)
> I check the AOSP (5.0.0_r2): 
> http://androidxref.com/5.0.0_r2/xref/frameworks/av/include/media/stagefright/
> Utils.h#63
> 
> This API only has 4 arguments. Which one should we use? Looks like b2g-l
> branch use 4 arguments now.

Thanks for catching this.  For now we should use the AOSP version so this patch needs a minor update before landing.

We removed the 5th argument via https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/commit/?id=af0ae5a245a45ee039d76396f16b0c52fca37eb6 from the CAF K b2g branch, but have not yet done the same for L.

Comment 6

[Michael Vines [:m1] [:evilmachines]]

Hey Greg, can you please upload a new version of the patch with non-AOSP 5th argument to canOffloadStream removed as per comment 4 and comment 5 so we can get this landed.

Comment 7

[Greg Grisco]

Attachment: 0001-Bug-1105452-Need-to-use-new-Audio-system-APIs-for-au.patch

Comment 8

[Carsten Book [:Tomcat]]

Hi,

Could you provide a try run for this change ? thanks!

Comment 9

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

try link:
https://hg.mozilla.org/try/rev/be894dc6f092

Comment 10

[Michael Vines [:m1] [:evilmachines]]

Thanks Randy, was the try run green? I don't see the results from that link, just the commit.

Comment 11

[StevenLee[:slee]]

hi m1,

Here is the link, https://tbpl.mozilla.org/?tree=Try&rev=be894dc6f092

Comment 12

[Michael Vines [:m1] [:evilmachines]]

Thanks, seems greenish enough given there's no L build on try for this to break ;)
Restoring checkin-needed

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/92bd6caa14da

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/92bd6caa14da