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

RESOLVED FIXED

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jhlin, Assigned: jhlin)

Tracking

({stale-bug})

53 Branch
stale-bug
Points:
---

Firefox Tracking Flags

(platform-rel +, firefox-esr52 unaffected, firefox53blocking fixed, firefox54blocking fixed, firefox55+ fixed)

Details

(Whiteboard: [platform-rel-Youtube])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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
platform-rel: --- → ?
Whiteboard: [platform-rel-Youtube]
On YT's radar. Adding to plat rel +
platform-rel: ? → +
Comment hidden (mozreview-request)
Priority: -- → P1

Comment 3

2 years ago
mozreview-review
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.
Attachment #8865755 - Flags: review?(esawin) → review+
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr52: --- → unaffected
Comment hidden (mozreview-request)

Comment 5

2 years ago
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)
(Assignee)

Comment 6

2 years ago
My bad. This patch is for the release branch and shouldn't be landed through Autoland. :$
(Assignee)

Comment 7

2 years ago
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
Attachment #8865755 - Flags: approval-mozilla-release?
If this is for release, it's for 53.  But, the regressing bug you mention (bug 1319987) is only in 54.
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.
tracking-firefox53: --- → blocking
tracking-firefox54: --- → blocking
tracking-firefox55: --- → +
Can we make it very clear through testing if this affects 54 and 55, or not? Thanks.
Ioana, can your team help test? Thanks.
Flags: needinfo?(ioana.chiorean)
(Assignee)

Comment 12

2 years ago
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 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.
Attachment #8865755 - Flags: approval-mozilla-release? → approval-mozilla-release+
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?
Flags: needinfo?(jolin)
Assignee: nobody → jolin

Comment 15

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-release/rev/4a1e7645dd0c
status-firefox53: affected → fixed
(Assignee)

Comment 16

2 years ago
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.
Flags: needinfo?(jolin)
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.
Marking 54 and 55 as fixed based on comment 12.
status-firefox54: affected → fixed
status-firefox55: affected → fixed

Comment 19

2 years ago
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).
Flags: needinfo?(ioana.chiorean) → needinfo?(jolin)

Comment 20

2 years ago
John Lin - was this the same as Bug 1363276 - Android remote decoder doesn't return video frames on Android 4.2.2?
(Assignee)

Comment 21

2 years ago
(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.
Flags: needinfo?(jolin)
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.
Keywords: stale-bug
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.