1552607 - seeking intermittently failed on Android

Status: RESOLVED FIXED

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

Description

[Alastor Wu [:alwu]]

Form from bug1548446 comment17, there are two different seeking issue on Android, which would cause intermittent failures.

This bug is used to investigate why seeking would fail on Android.

Comment 1

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

Attachment: Bug 1552607 - drop out-of-range outputs. r?jya

In bug 1540748/D28167 the demuxed sample timestamp was used to judge
whether audio decoder returns invalid output timestamp and reports
error for them. Unfortunately, Android may return buffers prior to
flush() and cause false alarm.

To make sure only requested buffers are reported, keep track of input
intervals and discard those that are outside the intervals rather
than compare with demux timestamp. Also log out-of-range buffers to
help debugging.

Comment 2

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

Attachment: Bug 1552607 - p1: filter out invalid input buffers. r?jya

Comment 3

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

Attachment: Bug 1552607 - p2: check sample session ID when processing buffers. r?jya

ProcessOutput() for buffers prior to Flush() could be scheduled to run
after the flush promise is resolved because MediaCodec buffer callback
runs asynchronously in both remote decoder process and content process.
To help check the validness of buffer, a session ID increased by flush
is added to both RemoteDataDecoder and remote codec service and will be
passed through IPC. If the passed ID doesn't agree with current session
ID, it means the buffer doesn't belong to current session and should be
discard.

Depends on D36381

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6385b8616d54
p1: filter out invalid input buffers. r=jya
https://hg.mozilla.org/integration/autoland/rev/53a39e402451
p2: check sample session ID when processing buffers. r=jya

Comment 5

[Alexandru Michis [:malexandru]]

Backed out 2 changesets for causing checkstyle failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/3de1b396b7177de25726848b8f62872eeb6072c8

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=53a39e4024511c051187db2a6c501c56c23a77ea&searchStr=checkstyle&selectedJob=254341852

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=254341852&repo=autoland&lineNumber=3045

[task 2019-07-01T23:59:25.147Z] 23:59:25 INFO - TEST-UNEXPECTED-FAIL | android-checkstyle | Checkstyle rule violations were found. See the report at: https://queue.taskcluster.net/v1/task/cju4VesKR2aF7nM7NCHpiA/runs/0/artifacts/public/android/checkstyle
[task 2019-07-01T23:59:25.147Z] 23:59:25 INFO - TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/Sample.java
[task 2019-07-01T23:59:25.148Z] 23:59:25 INFO - TEST-UNEXPECTED-FAIL | <error column="75" line="76" message="Parameter session should be final." severity="error" source="com.puppycrawl.tools.checkstyle.checks.FinalParametersCheck" />

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6b0141f7b1f
p1: filter out invalid input buffers. r=jya
https://hg.mozilla.org/integration/autoland/rev/f2d9515c3843
p2: check sample session ID when processing buffers. r=jya

Comment 7

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/f6b0141f7b1f
https://hg.mozilla.org/mozilla-central/rev/f2d9515c3843

Comment 8

[Chris Peterson [:cpeterson]]

John, should we uplift this seeking fix to Fennec ESR 68? Can this bug affect real users or just our automated tests (like bug 1548446)?

There will be no Fennec 69 release. Fennec users will be switched to the ESR 68 channel for the next 6-12 months, so the only way to release a fix to Fennec users will be to uplift to ESR.

Comment 9

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

(In reply to Chris Peterson [:cpeterson] from comment #8)

John, should we uplift this seeking fix to Fennec ESR 68? Can this bug affect real users or just our automated tests (like bug 1548446)?

There will be no Fennec 69 release. Fennec users will be switched to the ESR 68 channel for the next 6-12 months, so the only way to release a fix to Fennec users will be to uplift to ESR.

It could affect real users. I made a mistake here and caused bug 1563734, so I'll request uplift for both after the later is landed and verified in central.

Comment 10

[Chris Peterson [:cpeterson]]

There will be no Fennec 69 release. Fennec users will be switched to the ESR 68 channel for the next 6-12 months, so the only way to release a fix to Fennec users will be to uplift to ESR.

It could affect real users. I made a mistake here and caused bug 1563734, so I'll request uplift for both after the later is landed and verified in central.

Thanks! I'll update the bugs' status flags so to track for Fennec ESR 68 uplift.

[Tracking Requested - why for this release]:

John says this video seeking bug could affect real Fennec users and we should uplift the fix to Fennec ESR 68.0.x or 68.1. We will also want to uplift the fix for bug 1563734, which was a regression from this bug.(In reply to John Lin [:jhlin][:jolin] from comment #9)

Comment 11

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

Comment on attachment 9074874 [details]
Bug 1552607 - p1: filter out invalid input buffers. r?jya

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: The patch of bug 1540748 included in 68 causes a seeking failure regression that could be annoying to users. Please note that bug 1563734 fixed a regression by this patch and should also be uplifted.
• User impact if declined: Random MP4 video playback errors after seeking.
• Fix Landed on Version: 69
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch eliminates a racing issue.
• String or UUID changes made by this patch:

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9074874 [details]
Bug 1552607 - p1: filter out invalid input buffers. r?jya

Fixes a new regression in 68 causing problems when seeking. Approved for Fennec 68.1b3.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/53679e9046db
https://hg.mozilla.org/releases/mozilla-esr68/rev/57ae9ad9b574

Comment 14

[Diana Rus]

Hi, I tried to reproduce the issue, in order to test it, with Sony Xperia Z5 (Android 7.0) and Samsung Galaxy S9 (Android 8.0.0) Firefox Release 68.0.
The scenarios with which I tried to reproduce:

• Youtube:
  • play video and after pause the video and playing again the video
  • play video, move to a different position for the video/audio
• Twitter:
  • play video from a tweet, after the video was paused than continue playing
  • play video, move to a different position for the video/audio

@John Lin - Could extra information be provided in order to be sure what the issue was?

Note:

• Firefox Release 68.0 - An affected build as per Comment10 it says that the fix would go on either 68.0.x or 68.1.
• On Samsung Galaxy S9 (Android 8.0.0) as per the see also ticket that contained extra info: 1548446.
• Looked in logcat for both Youtube and Twitter platform to see whether there is extra info about the seeking process, but didn't found any extra detail.