1200111 - [Aries] AudioOffloadPlayer cannot be used

Status: RESOLVED WONTFIX

(Core :: Audio/Video: Playback, defect, P2)

Description

[Blake Wu [:bwu][:blakewu]]

This bug is created to fix the AudioOffloadPlayer from Bug 1166758 comment 36. 
Initial analysis is in Bug 1166758 comment 34.

Comment 1

[Blake Wu [:bwu][:blakewu]]

From the log on my Aries, 
it looks like Aries doesn't support Offload audio and that's why we failed to create AudioTrack with AUDIO_OUTPUT_FLAG_COMPRESS_OFFLOAD. 

W/AudioFlinger(10879): Thread AudioOut_2 cannot connect to the power manager service
W/AudioFlinger(10879): Thread AudioOut_3 cannot connect to the power manager service
W/AudioFlinger(10879): Thread AudioOut_3 cannot connect to the power manager service
E/AudioFlinger(10879): no wake lock to update!
W/AudioFlinger(10879): Thread AudioOut_2 cannot connect to the power manager service
E/AudioFlinger(10879): no wake lock to update!
D/audio_hw_extn(10879): audio_extn_get_anc_enabled: anc_enabled:0
D/sony_audio_effect(10879): sony_update_effect_param: Same out device. No need to update
"
E/audio_hw_primary(10879): adev_open_output_stream: Unsupported Offload information
"
D/audio_hw_primary(10879): adev_open_output_stream: exit: ret -22
E/AudioTrack(11406): Could not get audio output for stream type 3

This bug may need partner's help to look into. I am still going to check what else I can find.

Comment 2

[Juan Gomez [:_AtilA_] (CET/CEST)]

I have fix. It's more a hack because the problem is inside vendor libraries so we cannot fix it, and it's pretty ugly but it works and it's very simple. Alexandre told me that we should test playing a track without offloading support to ensure that all cases are working, the point is that we don't really know how to test such a thing... any guide would be appreciated :)

Comment 3

[Blake Wu [:bwu][:blakewu]]

Juan, 
Good job! Could you share us how you fixed it?

Offloading only works if the condition[1] is true.
You can simply play a song from website, which will not trigger offloading.  

[1]https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/MediaOmxCommonReader.cpp?case=true&from=MediaOmxCommonReader.cpp#70

Comment 4

[Juan Gomez [:_AtilA_] (CET/CEST)]

Attachment: Hack on audio.primary.msm8974.so to fix the audio offloading

As you can see in the patch, I'm restoring the first 4 bytes of the structure (first two fields: version and size), because the structure has been shifted 4 bytes to the left by Sony's libmedia. The good part is that I can figure out those 4 bytes because they are fixed, so I did.
With this patch, offloading works again and no glitches can be hear while playing whatever format uses it (like mp3).

Comment 5

[Juan Gomez [:_AtilA_] (CET/CEST)]

For landing this patch, we need to create a new branch in https://github.com/mozilla-b2g/hardware_qcom_audio and change the manifest file accordingly. So do not try to land it, it's here just for the review.

Comment 6

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8709387 [details] [diff] [review]
Hack on audio.primary.msm8974.so to fix the audio offloading

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

It is really hacky to me, but I am glad to see there is a way to make it work :-) 
As you mentioned in comment 5, this would not be good to be landed. 
Forward this review to mwu to have his comment as well.

Comment 7

[Michael Wu [:mwu]]

Comment on attachment 8709387 [details] [diff] [review]
Hack on audio.primary.msm8974.so to fix the audio offloading

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

I'm ok with this. Probably should remove some of the debugging spew though.

::: hal_mpq/audio_stream_out.c
@@ +2712,5 @@
>  
>      if (out->flags & AUDIO_OUTPUT_FLAG_COMPRESS_OFFLOAD) {
> +        /* Bug 1200111: This ugly hack is needed because of Sony Z3 Compact audio blobs, based on KitKat. It looks like
> +         * there's some buffer overrun on Sony's blobs, so we need to mitigate it by restoring the data which was messed up
> +         * by the blobs */

Hah, wow. Ok. I don't think this is an overrun as much as the Sony blobs using a different definition of audio_offload_info_t. I wonder what info the first 4 bytes store..

@@ +2720,5 @@
> +            ALOGE("%s: Before restoring: config->offload_info.version = %d .size = %d .sample_rate = %d .channel_mask = %d",
> +                   __func__, config->offload_info.version, config->offload_info.size, config->offload_info.sample_rate,
> +                   config->offload_info.channel_mask);
> +            memcpy(((char *)(&tmp))+4, &config->offload_info, sizeof(tmp)-4);
> +            memcpy(&config->offload_info, &tmp, sizeof(config->offload_info));

I think you can memmove this data instead of copying it to a separate buffer first.

@@ +2730,4 @@
>          ALOGE("%s: Usecase is OFFLOAD", __func__);
>          if (config->offload_info.version != AUDIO_INFO_INITIALIZER.version ||
>              config->offload_info.size != AUDIO_INFO_INITIALIZER.size) {
> +                ALOGE("%s: Unsupported Offload information. version = %d, (expected %d) size = %d (expected = %d)", __func__,

nit: bad indentation level

Comment 8

[Juan Gomez [:_AtilA_] (CET/CEST)]

Thanks Michael!
Yes, it looks like they are using different headers, and this is why I sent them an email few weeks ago so they could provide us the modified audio.h (where audio_offload_info_t resides), anyway I should update the comment to be more precise :)
I'd rather keep the debugging texts so we can make clear that we are hitting the hacky path in logcat, but it'll be probably better if they are ALOGI instead of ALOGE :)

From comment 99 bug 1166758, looks like fixing this issue causes problems with the sound for the rest of the system, so I'm not going to land anything until we can fix that too.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Closing as we are not working on Firefox OS anymore.