1362969 - MediaCodecDataDecoder::Flush() hangs when playing Youtube video on Android 4.2.2

Status: RESOLVED FIXED

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

Description

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

On 4.2.2 ACodec has a bug that when flush() could hang when called immediately after start(). It's fixed in 4.3 [1]

[1] https://android.googlesource.com/platform/frameworks/av/+/6463e76d41430f9b03a79b221de84255f2475658

Comment 1

[Thomas Elin [:relaas]]

On YT's radar. Adding to plat rel +

Comment 2

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

Attachment: Bug 1362969 - Flush MediaCodec only when necessary.

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

Comment 3

[Eugen Sawin [:esawin]]

Comment on attachment 8865755 [details]
Bug 1362969 - Flush MediaCodec only when necessary.

https://reviewboard.mozilla.org/r/137364/#review140578

::: dom/media/platforms/android/MediaCodecDataDecoder.cpp:471
(Diff revision 1)
>  {
>    MonitorAutoLock lock(mMonitor);
>  
>    if (mState == ModuleState::kFlushing) {
> +    if (!mDurations.empty()) {
> -    mDecoder->Flush();
> +      mDecoder->Flush();

Please add a comment stating the reason for the check.

Comment 4

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

Comment on attachment 8865755 [details]
Bug 1362969 - Flush MediaCodec only when necessary.

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

Comment 5

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7a57b6cf4f93 -d 62ff7ac07712: rebasing 395103:7a57b6cf4f93 "Bug 1362969 - Flush MediaCodec only when necessary. r=esawin" (tip)
other [source] changed dom/media/platforms/android/MediaCodecDataDecoder.cpp which local [dest] deleted
use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 6

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

My bad. This patch is for the release branch and shouldn't be landed through Autoland. :$

Comment 7

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

Comment on attachment 8865755 [details]
Bug 1362969 - Flush MediaCodec only when necessary.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1319987
[User impact if declined]: youtube video won't play
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no. Only version before 53 has this problem.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: it removes non-necessary operation.
[String changes made/needed]: none

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

If this is for release, it's for 53.  But, the regressing bug you mention (bug 1319987) is only in 54.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Is this broken on all Android hardware using 4.2.2 and previous? Or just specific devices?

If this really broken for all 4.2.2 and below, that is a significant amount of our user base, so we should fix this in a dot release rather than waiting a month.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Can we make it very clear through testing if this affects 54 and 55, or not? Thanks.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Ioana, can your team help test? Thanks.

Comment 12

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

Liz,

 My apologies. I incorrectly used blame on the central branch to find the bug#. This is not really a regression in our code (which was introduced in bug 1221991).
 
 The ACodec bug exists in Android 4.1~4.2.2 bug and is a timing issue, which shouldn't affect 54 and after (where OOP decoding is enabled).

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8865755 [details]
Bug 1362969 - Flush MediaCodec only when necessary.

Let's land this on m-r. I would like us to have the fix verified before we do a 53.0.3 fennec release, though.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

I still have questions, though.  If this is broken for Android 4.1-4.2.2, since bug 1221991, then it's been broken for a year and a half. If that's true, how is this the first we know about it? And, why the urgency now, if it isn't a recent regression? 

Is this happening for all YouTube videos, on all hardware with those versions of Android, or is the problem more limited?

We have to do a lot of work across several teams to do a dot release.  So, we need to know the user impact to judge whether it's worth it to do, or whether we should wait till the next release. 

Can you assign yourself to this bug - since you are submitting the patch?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/4a1e7645dd0c

Comment 16

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

I did some quick testing on Nexus 4 running Android 4.2.2. It looks like this ACodec bug didn't affect us until 52. (51.0.4 passed and 52.0b1 failed)
It could be some code change between those 2 versions that either adds a call to flush(), or makes existing call sooner and unleashes the ACodec bug.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, from email, this is not 100% reproducible and is only happening for a fairly small set of the user base, intermittently, and isn't a new regression in 53 (been around since 52). 

>>
It happens on on older android versions between 4.1 and 4.2.2, so that maps to Android SDK 16 and 17. 
That means 2.18% and 3.76% respectively across the whole Fennec user base. 
This issue is also a timing-wise bug to reproduce, so doesn’t 100% reproducible.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking 54 and 55 as fixed based on comment 12.

Comment 19

[Ioana Chiorean]

We were not able to reproduce it on 4.1.2 Android Devices. John Lin  do u see this still?
If needs more investigation, I can manually install 4.2.2 if not to check on some device (as we don't have one anymore).

Comment 20

[Ioana Chiorean]

John Lin - was this the same as Bug 1363276 - Android remote decoder doesn't return video frames on Android 4.2.2?

Comment 21

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

(In reply to Ioana Chiorean from comment #19)
> We were not able to reproduce it on 4.1.2 Android Devices. John Lin  do u
> see this still?
> If needs more investigation, I can manually install 4.2.2 if not to check on
> some device (as we don't have one anymore).

 Hi Ioana, I don't think further investigation is needed since current release is 54, which is not affected by this bug.

> John Lin - was this the same as Bug 1363276 - Android remote decoder doesn't return video frames on Android 4.2.2?

 No, bug 1363276 is for another Android 4.2.2 (and before) bug that release 54 was affected.

Comment 22

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.