brianchirls / Seriously.js

A real-time, node-based video effects compositor for the web built with HTML5, Javascript and WebGL
MIT License
3.88k stars 354 forks source link

videoElement.currentTime is incorrect #101

Open tencircles opened 9 years ago

tencircles commented 9 years ago

Hey there, I've done a bit of work with interactive video on several projects. I'm starting a project in the near future and considering seriously.js for blending various clips together. The issue I'm seeing in the source is that the current time being used is almost always and necessarily incorrect. Please refer to the media element spec on current playback position.

When seriously accesses the currentTime, it does so at an arbitrary point in time determined by requestAnimationFrame timing. Which will return the official playback position which is an approximation of the actual playback position of the video. The only reliable method for determining the actual current time of the video is to access the currentTime property exactly during a timeupdate event received from the video element.

Since it seems that the checkDirty method of a Node is only called during a requestAnimationFrame, the currentTime property of the video will almost always be incorrect. The correct implementation of this should actually calculate the currentTime of the video based on the time elapsed since the last known current playback position of the video element which occurred during a timeupdate event.

I'm experiencing some dropped frames in a few tests especially at 1080p video and above and I suspect this to be the cause. Specifically, even if the canvas is running at 60fps playing a video which only has 24fps, we still experience dropped frames.

I'm not personally familiar enough with the seriously js source to submit a pull request just yet, but I just though I'd bring it up and get your thoughts on the matter.

Thanks for your time!

brianchirls commented 9 years ago

Thanks for filing this. Unfortunately this is kind of a deceptively complex problem, and there may not be a solution without some compromises. I should explain the purpose of this check...

Seriously.js only uses currentTime to check if it's necessary to refresh the video texture (i.e. upload the latest frame to the GPU). Since it's not used for synchronizing animation or sound with the video (at least not yet), strict accuracy is neither necessary nor helpful.

Uploading a video frame to the GPU can be quite slow, especially for HD video, and is often the frame rate bottleneck. So we really want to avoid updating the texture more often than we have to. Ideally the browser would accurately report the current frame or at least give a frame rate, which would allow us to avoid updating on requestAnimationFrame calls between frames, but it does neither of those things. So the best we can do is to at least try not to update twice in a given render pass, in case a .render() method gets called twice or if a video source is passed into multiple effect nodes. The hope here is that currentTime will at best avoid some of these unnecessary calls, but maybe not all of them.

Checking against the clock won't help us here at all, since a call to checkDirty will always be a little bit later than the call before that. Though I could possibly set a very small clock-based threshold of maybe 2-4 ms. I could remove the check altogether, but then we risk piling up multiple unnecessary texture updates, potentially making the problem worse. If it takes 40 ms to update a HD video texture, doing it twice in one frame would cut the frame rate down to about 13fps.

It would be helpful if you could show some kind of test that demonstrated that the current code is actually skipping frames and that it's caused by Seriously.js/webgl and not by the browser's decoding. Perhaps a 60fps screen capture of the canvas next to the source video element. That way we'll know that this is actually a problem in practice and we'll have something to test a possible solution against.