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 synchronous steps #96

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

twjasa
Copy link

@twjasa twjasa commented Nov 23, 2022

Changes:

  • Step1 run after image is completely loaded
  • setTimeOut was removed from all steps

@debkbanerji
Copy link
Owner

Thanks for looking into this!

I like the idea of cleaning up the setTimeouts while possible but I do have some questions. (For context, many of these exist because browsers handles drawing of images in a way where it's difficult to detect completion. The setTimeouts are a simple-ish solution but are obviously kind of hacky)

  1. What is the purpose of the skipStep2 flag in initializeCropper? Why are we running step 2 within this and then not running this after step 1? There are no guarantees about how long the browser may take to draw stuff or how the cropper works under the hood, so this could lead to a painful race condition down the line. (in the pull request code, we're essentially running step 1 after step 2 is run in the initializeCropper function, which seems like it could cause issues)
  2. When removing all of these setTimeouts, how do we know that the browser will finish drawing the pixels to the canvases? (we're reliant on waiting a split second so we can read them back in the next steps)
  3. Overall, how did you test these changes? (making sure the right steps respond to the right setting changes, etc.)

@twjasa
Copy link
Author

twjasa commented Nov 28, 2022

Hey @debkbanerji no problem 😄 Thanks to you for this tool and for the response.

  1. I noticed that I was a bit confused about the flow of the app 😅. Now I think I have a clearer understanding of how is working, I removed all flags.
  2. I removed the call of runStep1 in handleInputImage and leave initializeCropper (which calls runStep1 -> runStep2->runStep3....) inside of inputImage.onload and that did the trick. Also because now all steps are running one after another I deleted some steps that were repeated.
  3. I basically test it in all browsers (except safari) with these steps:
    - Load image
    - check that all steps are fine
    - move the crop
    - check again
    - resize crop
    - check again
    - move resolution
    - check again
    I did not check the depth feature. After these changes I noticed that also the performance improved a lot. Check this out (left is fix-synchronous-steps and right is master):
Untitled.video.-.Made.with.Clipchamp.mp4

@twjasa
Copy link
Author

twjasa commented Jan 19, 2023

bump :(

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.

2 participants