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

RESOLVED FIXED in 2.2 S2 (19dec)

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaywang, Assigned: jaywang)

Tracking

unspecified
2.2 S2 (19dec)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

5 years ago
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.
Assignee

Comment 1

5 years ago
Attachment #8529224 - Attachment is obsolete: true
Comment on attachment 8529272 [details] [diff] [review]
0001-Resolve-the-build-failure-caused-by-API-changes.patch

Hi roc, 
Could you review this?
Thanks.
Attachment #8529272 - Flags: review?(roc)
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.
Attachment #8529272 - Flags: review?(roc)
Attachment #8529272 - Flags: review?(jld)
Attachment #8529272 - Flags: review+
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.
(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.
Attachment #8529272 - Flags: review?(jld) → review+
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.
Flags: needinfo?(ggrisco)

Comment 7

5 years ago
Attachment #8529272 - Attachment is obsolete: true
Flags: needinfo?(ggrisco)
Attachment #8531387 - Flags: review+

Updated

5 years ago
Keywords: checkin-needed
Hi,

Could you provide a try run for this change ? thanks!
Flags: needinfo?(ggrisco)
Keywords: checkin-needed
Blocks: 1098970
Thanks Randy, was the try run green? I don't see the results from that link, just the commit.
Flags: needinfo?(rlin)

Comment 11

5 years ago
hi m1,

Here is the link, https://tbpl.mozilla.org/?tree=Try&rev=be894dc6f092
Flags: needinfo?(rlin)
Thanks, seems greenish enough given there's no L build on try for this to break ;)
Restoring checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/92bd6caa14da
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S2 (19dec)
You need to log in before you can comment on or make changes to this bug.