-
Notifications
You must be signed in to change notification settings - Fork 979
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 parallel image processing #319
base: master
Are you sure you want to change the base?
Conversation
This looks to be a really massive changeset -- is there some kind of formatting change that was committed unintentionally here, or is it really that large of a change? (not just in the built library, but the source code changes are large enough that github refuses to display them to me, telling me to look at them offline) Also, is this substantially the same as the solution posted in #135 ? @TomasHubelbauer do you have time to have a look at this? I've just had a very brief look over the original issue, and the discussion makes sense, but I think we need to have a good look over the proposed solution, especially if the changeset is legit 1000-ish lines That said, @serratus did at least in theory greenlight it, but there's not an indication that he properly reviewed it. Thanks for the contribution, @ad-m |
@ericblade , see https://github.com/serratus/quaggaJS/pull/319/files?utf8=%E2%9C%93&diff=unified&w=1 (ignore whitespace). There is only changes in |
a...ha :-D that makes more sense out of it. So long as it doesn't change the external interface, I'm good with it. Any opinion @TomasHubelbauer ? |
@ericblade , it change external inerface, becuase then you have use library like that: - Quagga.init({
+ Quagga().init({
inputStream : { |
ahh. ok, see, I'm new to this code repo, so my head is spinning a little bit trying to wrap my head around it all. :-) If it changes the interface, then we have to do a major version bump, and go through that. I'm not opposed to it, but I would like more eyes on it than just me, if we're going to do that (and I'm not sure that we've got the publishing details hammered out yet?). It looks like, then, this would also require changes in (at least): example/file_input.js, example/live_w_locator.js, example/node-test-with-buffer.js, example/node-test.js, example/static_images.js, possibly the parts in quagga.js that reference Quagga, README.md, and test/integration/integration.spec.js After rebasing my patch to get testing working using PhantomJS, I was able to run all tests after fixing integration.spec.js, and they pass. #303 On the bright side, this seems? to fix at least one of the test cases being non-deterministic, although i'd need to get both patches together and test them on a travis builder or another machine to validate that 100% . . but it looks like this somehow solves the problem with EAN-8 image-003.jpg failing tests. |
@ad-m It would be great if you could update this to completely fix the other files, as detailed above. Once that's done, I'll try to figure out how to manage a major version change release. |
@ericblade Do you have any visibility into why we keep the |
@ad-m The link you sent above to ignore white space changes does not seem to work for me (same amount of changes). Is this still supported by GitHub? |
@TomasHubelbauer hmm. with hide whitespace changes ticked, it shows without looks like maybe they've changed the algorithms some :-S As to the question about keeping the 'dist' in the repo, i'd prefer not to, if that's an option. It definitely clutters up history. |
@ad-m I've forked this project at https://github.com/ericblade/quagga2 and would love to have your contributions there! |
See also ericblade/quagga2#171 |
Fix:
I do not have much experience in JS, so please do not beat (too much).