Closed Bug 1919683 Opened 2 years ago Closed 2 years ago

Incorrect final video dimensions or final presented video frame when decoding lags

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
132 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- wontfix
firefox-esr128 --- wontfix
firefox130 --- wontfix
firefox131 --- wontfix
firefox132 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

(Regression)

Details

(Keywords: regression)

Attachments

(6 files, 1 obsolete file)

Bug 1919683 move big endian 32-bit integer helper to MediaSourceUtil r?#media-playback-reviewers
48 bytes, text/x-phabricator-request
Details | Review
Bug 1919683 add a description to an optional assert r?#media-playback-reviewers
48 bytes, text/x-phabricator-request
Details | Review
Bug 1919683 correct documentation of media.ruin-av-sync.enabled r?#media-playback-reviewers
48 bytes, text/x-phabricator-request
Details | Review
Bug 1919683 keep the most recent video frame for presentation at video end or pause r?#media-playback-reviewers
48 bytes, text/x-phabricator-request
Details | Review
Bug 1919683 remove optional parameters from RenderVideoFrames() r?alwu
48 bytes, text/x-phabricator-request
Details | Review
Bug 1919683 change MediaDecoderOwner::AbstractMainThread() from instance to class method r?#media-playback-reviewers
48 bytes, text/x-phabricator-request
Details | Review

Even the last video frame of media can be dropped in the VideoSink without being sent to the compositor, so that the video element never shows the correct final frame.

Before https://hg.mozilla.org/mozilla-central/rev/4f39f2e1324b#l2.38, RenderVideoFrames() would have had the last frame still in the queue and so would not have returned early but would have changed the frames and dimensions in the video container before the end of playback.

https://pernos.co/debug/E80n6oQztoIc865zC3H0DA/index.html records a run of a test written for bug 1909750. (Even without a fix for that bug) the last frame has dimensions of 640x480 in the first subtest, 'overlap key frame and dependent frame'. The last frame is passed to the compositor in this case, but the dimensions passed to the VideoFrameContainer are those of the first frame from the VideoQueue().
The last frame of the media is never the first frame passed to the VideoFrameContainer because it is dropped before this can happen.

https://pernos.co/debug/Oexl8Ph29CX0oF6xU476ig/index.html is a run with an optimized build that passes the subtest 'overlap key frame and dependent frame' because the last frame can be rendered on time.

Set release status flags based on info from the regressing bug 1258870

status-firefox130: --- → affected
status-firefox131: --- → affected
status-firefox132: --- → affected
status-firefox-esr115: --- → affected
status-firefox-esr128: --- → affected

Is there a user-facing impact from this bug?

status-firefox130: affected → wontfix
status-firefox131: affected → wontfix
status-firefox-esr115: affected → wontfix
Flags: needinfo?(karlt)

There is an intermittent impact of the user seeing the wrong frame at the end of video playback if decoding is slow.
On most sites that won't matter too much, but a video may intentionally have a banner or similar in the last frame.

Flags: needinfo?(karlt)

The documentation implied that a minimum queue size of 0 would remedy A/V sync
but this actually just changes the effect to a playback stall, as reported in
bug 1316571 and others.

A subsequent patch will restore the minimum queue size of 1, but still
drop frames while the video is playing.

ClearFutureFrames() is not required in MaybeResolveEndPromise() when the
VideoQueue() is empty, because the queue is now not emptied until end of
playback. An empty queue before end of playback means no frames have been set
in the VideoFrameContainer.

This no longer needs to be an instance method since
https://hg.mozilla.org/mozilla-central/rev/324cc7d374d1860a1743fa7f8a33a782e3bd4177#l50.524

Another mock MediaDecoderOwner implementation will be added for GTests and
having the single AbstractMainThread() implementation saves having to add
another.

Maintaining the method on MediaDecoderOwner retains the clarification that the
different clients are using the same event queue mechanism.

Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9b9da983e745 move big endian 32-bit integer helper to MediaSourceUtil r=media-playback-reviewers,padenot https://hg.mozilla.org/integration/autoland/rev/ae4b8df5ca28 add a description to an optional assert r=media-playback-reviewers,padenot https://hg.mozilla.org/integration/autoland/rev/93492262ee85 correct documentation of media.ruin-av-sync.enabled r=media-playback-reviewers,padenot
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/48359 for changes under testing/web-platform/tests
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/180cb1869213 remove optional parameters from RenderVideoFrames() r=padenot
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/13dc2bb30a54 change MediaDecoderOwner::AbstractMainThread() from instance to class method r=media-playback-reviewers,padenot
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/899312c8f860 keep the most recent video frame for presentation at video end or pause r=media-playback-reviewers,padenot
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4b3ea81e0245 Release references to unused frames and update dimensions even when decoded frames are late in VideoSink r=media-playback-reviewers,padenot

Backed out changeset 4b3ea81e0245 (bug 1919683) for causing multiple android Gtest failures.

[task 2024-09-27T18:41:06.490Z] 18:41:06     INFO -  TEST-START | PacerTestIntTenMsDuplication.SingleDuplication
[task 2024-09-27T18:41:26.649Z] 18:41:26     INFO -  gtest INFO | gtest | wait for org.mozilla.geckoview.test_runner complete; top activity=com.android.launcher3
[task 2024-09-27T18:41:26.954Z] 18:41:26     INFO -  mozcrash checking /tmp/tmpja_m6k2g for minidumps...
[task 2024-09-27T18:41:27.385Z] 18:41:27     INFO - Return code: 0
[task 2024-09-27T18:41:27.386Z] 18:41:27    ERROR - No tests run or test summary not found
[task 2024-09-27T18:41:27.386Z] 18:41:27     INFO - TinderboxPrint: gtest<br/><em class="testfail">T-FAIL</em>
[task 2024-09-27T18:41:27.386Z] 18:41:27     INFO - ##### gtest log ends
[task 2024-09-27T18:41:27.386Z] 18:41:27  WARNING - setting return code to 1
[task 2024-09-27T18:41:27.386Z] 18:41:27     INFO - The gtest suite: gtest ran with return status: WARNING
[task 2024-09-27T18:41:27.386Z] 18:41:27     INFO - Running post-action listener: _package_coverage_data
[task 2024-09-27T18:41:27.386Z] 18:41:27     INFO - Running post-action listener: _resource_record_post_action
[task 2024-09-27T18:41:27.386Z] 18:41:27     INFO - Running post-action listener: process_java_coverage_data
[task 2024-09-27T18:41:27.386Z] 18:41:27     INFO - Running post-action listener: stop_device
[task 2024-09-27T18:41:27.789Z] 18:41:27     INFO - /data/tombstones/tombstone_00 deleted
[task 2024-09-27T18:41:27.867Z] 18:41:27     INFO - /data/tombstones/tombstone_01 deleted
[task 2024-09-27T18:41:27.945Z] 18:41:27     INFO - /data/tombstones/tombstone_02 deleted
[task 2024-09-27T18:41:28.026Z] 18:41:28     INFO - /data/tombstones/tombstone_03 deleted
[task 2024-09-27T18:41:28.026Z] 18:41:28     INFO - Killing logcat pid 1378.
[task 2024-09-27T18:41:28.026Z] 18:41:28     INFO - Killing every process called qemu-system-x86_64
[task 2024-09-27T18:41:28.031Z] 18:41:28     INFO - [mozharness: 2024-09-27 18:41:28.031677Z] Finished run-tests step (success)
[task 2024-09-27T18:41:28.031Z] 18:41:28     INFO - Running post-run listener: _resource_record_post_run
[task 2024-09-27T18:41:28.209Z] 18:41:28     INFO - Total resource usage - Wall time: 445s; CPU: 13%; Read bytes: 4096; Write bytes: 708587520; Read time: 36; Write time: 674452
[task 2024-09-27T18:41:28.209Z] 18:41:28     INFO - TinderboxPrint: CPU usage<br/>13.1%
[task 2024-09-27T18:41:28.209Z] 18:41:28     INFO - TinderboxPrint: I/O read bytes / time<br/>4,096 / 36
[task 2024-09-27T18:41:28.209Z] 18:41:28     INFO - TinderboxPrint: I/O write bytes / time<br/>708,587,520 / 674,452
[task 2024-09-27T18:41:28.209Z] 18:41:28     INFO - TinderboxPrint: CPU guest<br/>267.0 (7.0%)
[task 2024-09-27T18:41:28.209Z] 18:41:28     INFO - TinderboxPrint: CPU idle<br/>3,072.2 (80.6%)
[task 2024-09-27T18:41:28.209Z] 18:41:28     INFO - TinderboxPrint: CPU system<br/>163.6 (4.3%)
[task 2024-09-27T18:41:28.209Z] 18:41:28     INFO - TinderboxPrint: CPU user<br/>304.7 (8.0%)
[task 2024-09-27T18:41:28.209Z] 18:41:28     INFO - TinderboxPrint: Swap in / out<br/>0 / 0
[task 2024-09-27T18:41:28.210Z] 18:41:28     INFO - start-emulator - Wall time: 0s; CPU: Can't collect data; Read bytes: 0; Write bytes: 0; Read time: 0; Write time: 0
[task 2024-09-27T18:41:28.214Z] 18:41:28     INFO - verify-device - Wall time: 30s; CPU: 23%; Read bytes: 0; Write bytes: 14495744; Read time: 0; Write time: 2856
[task 2024-09-27T18:41:28.215Z] 18:41:28     INFO - install - Wall time: 11s; CPU: 34%; Read bytes: 0; Write bytes: 372948992; Read time: 0; Write time: 366756
[task 2024-09-27T18:41:28.248Z] 18:41:28     INFO - run-tests - Wall time: 403s; CPU: 12%; Read bytes: 4096; Write bytes: 308867072; Read time: 36; Write time: 291320
[task 2024-09-27T18:41:29.180Z] 18:41:29  WARNING - returning nonzero exit status 1
[task 2024-09-27T18:41:29.248Z] cleanup
[task 2024-09-27T18:41:29.248Z] + cleanup
[task 2024-09-27T18:41:29.248Z] + local rv=1
[task 2024-09-27T18:41:29.248Z] + [[ -s /builds/worker/.xsession-errors ]]
[task 2024-09-27T18:41:29.248Z] + cp /builds/worker/.xsession-errors /builds/worker/artifacts/public/xsession-errors.log
[task 2024-09-27T18:41:29.249Z] + '[' ']'
[task 2024-09-27T18:41:29.249Z] + true
[task 2024-09-27T18:41:29.249Z] + cleanup_xvfb
[task 2024-09-27T18:41:29.249Z] ++ pidof Xvfb
[task 2024-09-27T18:41:29.252Z] + local xvfb_pid=58
[task 2024-09-27T18:41:29.252Z] + local vnc=false
[task 2024-09-27T18:41:29.252Z] + local interactive=false
[task 2024-09-27T18:41:29.252Z] + '[' -n 58 ']'
[task 2024-09-27T18:41:29.252Z] + [[ false == false ]]
[task 2024-09-27T18:41:29.252Z] + [[ false == false ]]
[task 2024-09-27T18:41:29.252Z] + kill 58
[task 2024-09-27T18:41:29.252Z] + screen -XS xvfb quit
[task 2024-09-27T18:41:29.257Z] + exit 1
[taskcluster 2024-09-27 18:41:29.824Z] === Task Finished ===
[taskcluster 2024-09-27 18:41:32.144Z] Unsuccessful task run with exit code: 1 completed in 2415.583 seconds
Flags: needinfo?(karlt)
Backout by imoraru@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/881c8e72b246 Backed out changeset 4b3ea81e0245 for causing multiple android Gtest failures.
Upstream PR merged by moz-wptsync-bot

Comment on attachment 9427351 [details]
Bug 1919683 Release references to unused frames and update dimensions even when decoded frames are late in VideoSink r?#media-playback-reviewers

Revision D223697 was moved to bug 1316571. Setting attachment 9427351 [details] to obsolete.

Attachment #9427351 - Attachment is obsolete: true
Attachment #9427351 - Attachment is obsolete: false
Flags: needinfo?(karlt)

Comment on attachment 9427351 [details]
Bug 1919683 Release references to unused frames and update dimensions even when decoded frames are late in VideoSink r?#media-playback-reviewers

Revision D223697 was moved to bug 1316571. Setting attachment 9427351 [details] to obsolete.

Attachment #9427351 - Attachment is obsolete: true
Blocks: 1316571
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: