search

etro-js / etro

Typescript video-editing framework for the browser
https://etrojs.dev
GNU General Public License v3.0
899 stars 86 forks source link

Wait for resources to load before refreshing #191

Closed clabe45 closed 1 year ago

clabe45 commented 1 year ago

Wait for the _whenReady method to resolve before rendering in Movie#refresh

OmegaHawkeye commented 1 year ago

Hi @clabe45, I wanted to contribute but after some code changes and running the linter I tried to run the test but without much success since the canvas library is missing. Is this a know issue or my fault even though I have basically new system of node js installed.

clabe45 commented 1 year ago

Hi @OmegaHawkeye, thanks for contributing! Can you paste the error? Which os and version of node are you using? After running npm install and the other steps in the contributing guide, does the issue persist? I can't seem to reproduce it on my end.

leanjunio commented 1 year ago

Hi @clabe45, I wanna give this a shot if it hasn't been taken yet. Do we have info on how we came across this bug? Was it a failed test by any chance that I can run on my end? Just not sure how to test if my fix works.

clabe45 commented 1 year ago

Hi everyone, the async Movie._whenReady() method was recently added. The method resolves when all its layers' and effects' _whenReady() methods resolve, which happens when all resources are loaded and ready to be used for playback.

I opened this issue because play() currently waits for _whenReady() to resolve before beginning playback. However, after looking into this more, it's not really necessary to call _whenReady() in either play() or refresh(). All the logic to wait until the movie is ready happens in the render loop (_render()), so we don't need to call _whenReady() at the beginning of play(). In the render loop, the movie already constantly checks if it is ready and responds accordingly (in case an input video stream buffers). refresh() is just for refreshing the screen, and it also uses the render loop. All this to say that this change isn't necessary, but if anyone is already working on it, I'll review your PR.

leanjunio commented 1 year ago

Gotcha, I don't have one myself so all good from my end.