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

wasmWorker example: Add option to receive empty response when QR not found #87

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

Conversation

JL102
Copy link

@JL102 JL102 commented Nov 9, 2023

Issue #, if available:
N/A

Description of changes:
In my own app, I was unable to send messages to the worker too frequently, lest I send messages while it's still working, leading to the worker stalling over a pile of work that it can never catch up on, delaying the time it takes to actually register a successful scan.

In my use case, I was able to remedy this by having the worker respond with data/type undefined, and then once the app receives word that the worker is no longer busy, it's free to send another snapshot from the camera. This significantly improved the performance of my scanner, because I can essentially always keep the worker busy & not be afraid of piling it with more work than it can handle.

I added it under the optional event.data.alwaysRespond, because this is a change in the API behavior, and therefore the programmer must update the way they call the worker in order to take advantage of this change. (i.e., if you request alwaysRespond: true, then you can no longer assume that data is present when receiving a message from the worker. So it makes sense to me to have this an "opt-in" change.

Side note: I noticed that public/jsQrWorker.js has a console.log statement where wasmWorker doesn't. I didn't include a change to it in this PR cuz it's outs of the scope of this issue, but I thought I'd mention it if you didn't realize it was there.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@JL102
Copy link
Author

JL102 commented Nov 9, 2023

Commit 1122728 was because I saw a diff on the last line of jsQrWorker.js, so I thought it was because i accidentally deleted a newline at the end of the file, but the diff is still there. Not sure what the deal is with that.

@maslick
Copy link
Owner

maslick commented Nov 9, 2023

Hey @JL102, why can't you adjust the scan rate (see https://github.com/maslick/koder/blob/master/src/index.js#L8) instead? It's set to 250 ms by default, resulting in 4 images per second sent to the Web worker. In my experience, especially with iPhones 11 Pro, 13 Pro, and 14 Plus, koder can handle image processing at 10 ms (100 images per second) and below without any problem. I personally find 250 ms optimal for balancing performance and usability. Anything below 100 ms seems like a bit of a resource overkill. What are your thoughts on adjusting the scan rate?

@JL102
Copy link
Author

JL102 commented Nov 10, 2023

The app I'm working on may be run on a wide variety of devices, including older and slower phones. Also, sometimes the device/browser may give a video stream that's a bit higher resolution than expected, which takes longer to parse. For me personally, "blindly" adjusting the scan rate without the main thread receiving any concrete feedback on how long the parsing really takes makes me really uncomfortable. When I was playing with different video resolutions, I was encountering issues where the web worker was overloaded with requests as described above, and I didn't want that to happen again, so I turned it up to 500 ms but then found that the scanning experience was terrible and inconsistent. For me, being able to know for sure when the web worker has stopped scanning the last frame gives me the peace of mind necessary to push it further with more frames.

That being said, you're probably right about it being a bit overkill to crank up the frequency too high, so I might cap the scan rate anyways instead of requesting a new scan as soon as it's possible. I'll do some tests to see where the point of diminishing returns is, and cap it there.

About the PR: I've copied the WASM and JS into the project's source (with my suggested changes) so I'm not dependent on the PR being accepted & uploaded to a new version on NPM. I just figured I'd submit it as PR because it's an upgrade IMO, in case others would benefit from it too. If you don't want to merge it, that's alright!

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.

3 participants