Always show video frame at or before timestamp - #1531
Conversation
yannik131
changed the title
Since my free plan can barely handle 2-3 reviews, from now on I'll just request a review from you and you can request an AI review if your budget allows it, I'll revisit the PR on my own if you do that. |
mzur
left a comment
mzur
left a comment
There was a problem hiding this comment.
This looks very promising and decoding with Mediabunny seems as fast as the old "seek to currentTime on pause" trick. I hope that we can close all the issues related to frame-accuracy as part of this PR. The warning message for Firefox should be removed as part of this PR, too.
Did you test for correctness with the artificial videos again?
When I reload the page and the video is seeked to a previous time, I sometimes get the first video frame instead. Looks like an async race issue:
Screencast.From.2026-09-02.14-56-14.webm
More comments below.
| this.videoContext.drawImage(this.video, 0, 0, this.videoCanvas.width, this.videoCanvas.height); | ||
| this.videoSource.changed(); |
| if (this.video.paused) { | ||
| await this.renderPausedFrame(); | ||
| } else { | ||
| this.renderVideo(true); |
There was a problem hiding this comment.
Is the else case still needed for anything? The old workaround was to seek to currentTime to force the browser to jump to the frame that will be displayed at that time (see setPausedAndSeek()). This was done to make sure the same frame will be shown next time based on the timestamp that was stored for an annotation.
This didn't fix the cross-browser and decoding inconsistencies that we want to fix with this PR. But maybe the currentTime = currentTime trick is no longer required with this PR, too? If we remove this we have to call renderPausedFrame directly on the pause event. Also we should make the process of decoding with Mediabunny visible in the UI somehow (currently the seeking state does this).
Ideally we can close #433 as part of this PR too.
| try { | ||
| this.mediabunnyInput = new Input({ | ||
| source: new UrlSource(this.video.src), | ||
| formats: ALL_FORMATS | ||
| }); | ||
| const track = await this.mediabunnyInput.getPrimaryVideoTrack(); | ||
| this.mediabunnySink = new VideoSampleSink(track); | ||
| } catch (e) { | ||
| this.useMediabunnyFallback = false; | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
When can this fail? I'd rather fail hard if Mediabunny is not available than to display wrong frames again as fallback (i.e. Mediabunny should be the default method, not the fallback). What do you think?
| const roundedTime = Math.round(time * 100) / 100; | ||
| if (this.cachedTime === roundedTime && this.cachedBitmap) { | ||
| this.drawBitmap(this.cachedBitmap); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This rounds to 10s of ms, right? If decoding is fast, shouldn't we skip rounding/caching and use the raw timestamp for the sake of correctness instead?
| protected function getVideoFrame(Video $video, float $time, int $trySeek = 60) | ||
| protected function getVideoFrame(string $sourcePath, float $time, int $trySeek = 60) | ||
| { | ||
| $fps = max($this->getVideoFps($sourcePath), 0.0001); |
There was a problem hiding this comment.
This should be done outside the loop in processAnnotationChunk and maybe even before the chunking in handleFile. The fps can be cached once for the whole job.
|
|
||
| $buffer = File::get($outputPath); | ||
| } catch (Throwable $e) { | ||
| $buffer = ''; |
There was a problem hiding this comment.
Should this really be handled as empty buffer instead of failing immediately? If there is something wrong with the video, the job retries 60 times before giving up.
| this.video.addEventListener('loadedmetadata', () => { | ||
| this.initMediabunny(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Since the same video object is reused and only src is updated, it should be enough to add the listener once in created().
| // 1.5M annotations on 16k videos). | ||
| $time = max(0, $time - 0.033333333); | ||
| } while (empty($buffer) && $trySeek > 0); | ||
| $time = max(0, $time - (1 / 30.0)); |
There was a problem hiding this comment.
We now know the actual framerate so we can use exact steps.
2 participants
Closes #1492