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

Fix video backward seeking / stepping back sometimes getting stuck (in the presence of b-frames) #8053

Merged
merged 3 commits into from
Nov 11, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Nov 8, 2024

What

Our PTS -> sample search algorithm wasn't quite right. Fixed this here and added a unit test.
Unfortunately, I had to introduce a small auxiliary data structure to keep this snappy / not O(n) for n samples, but it's very simple and rather contained.

Tested on native & web. Web still suffers from

but on Chrome this works fine now!

Screen.Recording.2024-11-08.at.16.55.06.mov

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!
  • If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide

To run all checks from main, comment on the PR with @rerun-bot full-check.

@@ -104,9 +109,23 @@ impl SamplesStatistics {
.iter()
.all(|s| s.decode_timestamp == s.presentation_timestamp);

let mut biggest_pts_so_far = Time::MIN;
let has_sample_highest_pts_so_far = samples
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also a bit worried about how many times we're iterating over the samples here. I don't think LLVM will optimize this to one loop.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh I'm not sure if it's better to combine everything to a single loop or to loop several times: individual loops might vectorize better

pure speculation until someone actually measures. Maybe I should. After all we might run this with a lot of samples!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It optimizes a lot better than I thought: https://godbolt.org/z/n3Yvzff9e

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neither is in the mood for simd though. probably because everything is 64bit integer

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leaving it be as is for now

Copy link
Member

@jprochazk jprochazk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than nits above this LGTM

@Wumpf
Copy link
Member Author

Wumpf commented Nov 11, 2024

known issue on error[unmaintained]: instant is unmaintained

@Wumpf Wumpf merged commit 9b3af30 into main Nov 11, 2024
35 of 36 checks passed
@Wumpf Wumpf deleted the andreas/fix-video-step-back branch November 11, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stepping back doesn't always enqueue frames to the decoder (in the presence of b-frames?)
2 participants