Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Console log frame decoding timestamps and out-of-order errors #734

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marcello3d
Copy link

To help diagnose out-of-order decoding on Safari I added this logging. context: #55 (comment)

@chrisn
Copy link
Member

chrisn commented Feb 8, 2024

@youennf PTAL, is this indicating a difference between Safari and other browsers? Do we have a WPT test case for out of order decoding?

@youennf
Copy link
Contributor

youennf commented Feb 8, 2024

VideoToolbox is outputting video frames in decoding order not presentation order.
We fixed this issue in Safari and wrote some tests. They should probably be upstreamed to WPT (https://github.com/WebKit/WebKit/blob/main/LayoutTests/http/tests/webcodecs/h264-reordering.html and https://github.com/WebKit/WebKit/blob/main/LayoutTests/http/tests/webcodecs/hevc-reordering.html)

@chrisn
Copy link
Member

chrisn commented Feb 8, 2024

Thanks! That sounds good. I'll leave it to you, @sandersdan and @aboba on whether to merge this PR. (I'd suggest it shouldn't log for every frame, only those detected as out of order.)

@marcello3d
Copy link
Author

@youennf did this fix ship? Because I'm still seeing out of order failures on Safari and Safari Technology Preview and haven't seen it mentioned on any of the STP release notes. I've posted on bugzilla but gotten no response: https://bugs.webkit.org/show_bug.cgi?id=263901

Here is my repro:

Expected:

  • no error logs, everything decodes in ascending order

Actual:

  • alternating decode and decode out of order logs:
[Log] Decoded frame 66666 µs (worker.js, line 78)
[Log] Decoded frame 100000 µs (worker.js, line 78)
[Log] Decoded frame 133333 µs (worker.js, line 78)
[Log] Decoded frame 200000 µs (worker.js, line 78)
[Error] Decoded frame 166666 µs out of order, last frame was 200000 µs
	output (worker.js:76)
[Log] Decoded frame 233333 µs (worker.js, line 78)
[Log] Decoded frame 366666 µs (worker.js, line 78)
[Error] Decoded frame 300000 µs out of order, last frame was 366666 µs
	output (worker.js:76)
[Error] Decoded frame 266666 µs out of order, last frame was 300000 µs
	output (worker.js:76)
[Log] Decoded frame 333333 µs (worker.js, line 78)
[Log] Decoded frame 500000 µs (worker.js, line 78)
[Error] Decoded frame 433333 µs out of order, last frame was 500000 µs
	output (worker.js:76)

...etc

@youennf
Copy link
Contributor

youennf commented Feb 8, 2024

@youennf did this fix ship?

It should be in STP. Maybe there is still a bug in the queue size computation.
Let's continue in bugzilla.

@tidoust
Copy link
Member

tidoust commented Feb 8, 2024

+1 to add tests to WPT.

I don't think samples should document implementation bugs though, or at least they should be very explicit about it and link to the tracking issue in bug trackers. Otherwise, the code could confuse readers with issues that will hopefully disappear once the spec, test suite and implementations become more stable.

@marcello3d
Copy link
Author

For context: this PR wasn't intended to be merged, just was the smallest/easiest way to make a test case for the bug. I'm not sure how to make a WPT but happy to contribute one if someone can point me in the right direction!

@youennf
Copy link
Contributor

youennf commented Feb 8, 2024

@chrisn, do you know whether we can use https://w3c.github.io/webcodecs/samples/data/bbb_video_avc_frag.mp4 for WPT tests?

@chrisn
Copy link
Member

chrisn commented Feb 8, 2024

From a licensing perspective I think we can. The website says it's released under the Creative Commons Attribution 3.0 license, which means we can copy it for any purpose but we must credit Blender Foundation (www.blender.org) and link to the license. I guess we could add a README file to the relevant WPT folder to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants