1306483 - Audio "skips" when using out-of-process decoder

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect, P2)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

I've been running Nightly with the remote media codec enabled (media.android-remote-codec.enabled = true), and sometimes I have a problem with audio. It sounds like it's skipping or missing samples. It's fairly easy to reproduce, but it doesn't seem to always happen. I don't have a consistent test URL at the moment, but if I find one I'll attach to this bug.

Comment 1

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

Once bug 1295106 is fixed, it should be able to fix this one as well.

Comment 2

[John Lin [:jhlin][:jolin]]

Unfortunately bug 1295106 patches mitigates but not completely eliminates audio skips in my own tests. Will continue to investigate the problem.

Comment 3

[John Lin [:jhlin][:jolin]]

The methods of CallbackSender[1] call sendMessage()[2][3] to queue buffer notifications in its looper. Since Codec doesn't supply a looper for it, the BufferPoller looper is shared by both and messages for polling will delay the delivery of output buffers for AudioStream to consume. Sending notification immediately instead of queuing solves the problem.

Unfortunately, I found that on my Nexus 5, after playing video for a while (10+ min), the CPU governor will shutdown all cores but one and it starts to stutter very, very seriously. Next I will investigate what triggers CPU governor.

[1] http://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/media/JellyBeanAsyncCodec.java#71
[2] http://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/media/JellyBeanAsyncCodec.java#90
[3] http://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/media/JellyBeanAsyncCodec.java#100

Comment 4

[John Lin [:jhlin][:jolin]]

From kernel log, it looks like the cores were shut down by thermal regulator. :(

<6>[83408.055426] msm_thermal: Set Offline: CPU3 Temp: 81
<6>[83408.305412] msm_thermal: Set Offline: CPU2 Temp: 80
<6>[83408.555443] msm_thermal: Set Offline: CPU1 Temp: 81

Comment 5

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1306483 - Part 1: use concurrent queue to fix race condition.

Review commit: https://reviewboard.mozilla.org/r/87558/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87558/

Comment 6

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1306483 - Part 2: send notification immediately when caller sender shares looper with buffer poller.

Review commit: https://reviewboard.mozilla.org/r/87560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87560/

Comment 7

[John Lin [:jhlin][:jolin]]

Comment on attachment 8803260 [details]
Bug 1306483 - Part 1: use concurrent queue to fix race condition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87558/diff/1-2/

Comment 8

[John Lin [:jhlin][:jolin]]

Comment on attachment 8803261 [details]
Bug 1306483 - Part 2: send notification immediately when caller sender shares looper with buffer poller.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87560/diff/1-2/

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8803260 [details]
Bug 1306483 - Part 1: use concurrent queue to fix race condition.

https://reviewboard.mozilla.org/r/87558/#review87068

Comment 10

[Eugen Sawin [:esawin]]

Comment on attachment 8803261 [details]
Bug 1306483 - Part 2: send notification immediately when caller sender shares looper with buffer poller.

https://reviewboard.mozilla.org/r/87560/#review87076

Decoding audio shouldn't cause thermal throttling, there must be an issue somewhere else.

::: mobile/android/base/java/org/mozilla/gecko/media/JellyBeanAsyncCodec.java:97
(Diff revision 2)
> +
> +        private void processMessage(Message msg) {
> +            if (Looper.myLooper() == getLooper()) {
> +                handleMessage(msg);
> +            } else {
> -            sendMessage(msg);
> +                sendMessage(msg);

Remove ">>>>".

::: mobile/android/base/java/org/mozilla/gecko/media/JellyBeanAsyncCodec.java:98
(Diff revision 2)
> +        private void processMessage(Message msg) {
> +            if (Looper.myLooper() == getLooper()) {
> +                handleMessage(msg);
> +            } else {
> -            sendMessage(msg);
> +                sendMessage(msg);
> -        }
> +            }

Remove ">>>>".

Comment 11

[Eugen Sawin [:esawin]]

Weird, review board showed ">>>>" explicitly, bug?

Comment 12

[John Lin [:jhlin][:jolin]]

Comment on attachment 8803261 [details]
Bug 1306483 - Part 2: send notification immediately when caller sender shares looper with buffer poller.

https://reviewboard.mozilla.org/r/87560/#review87076

It takes longer but happens when pref is off too. Doesn't happen on Z3C or Nexus 6. Maybe Nexus 5 thermal engine has more aggressive settings?s

> Remove ">>>>".

Downloaded diff/patch file and couldn't see '>>>>'. I guess it's the Reviewboard diff viewer showing added whitespaces.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77c515e8350a
Part 1: use concurrent queue to fix race condition. r=snorp
https://hg.mozilla.org/integration/autoland/rev/9f2f66e3762f
Part 2: send notification immediately when caller sender shares looper with buffer poller. r=esawin

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/77c515e8350a
https://hg.mozilla.org/mozilla-central/rev/9f2f66e3762f