Fix #5 parallel use of decodeSingle, part of #105, restructuring #171
Conversation
|
@twsouthwick If you'd like to have a look at this, it seems to, rather surpisingly, fix parallel decode. I expected that we'd also have to do some work in barcode_decoder and/or barcode_locator, but this is giving me correct results with small tests run many times. I'm feeling a bit hesitant to publish this, since it could break down in a lot of ways (especially barcode_decoder and locator modules) so if you have a use case that can put it through some stress, that'd be fantastic. |
There was a problem hiding this comment.
You may want to mark the current start/stop/etc methods as deprecated (such as https://stackoverflow.com/questions/19412660/how-should-i-mark-a-method-as-obsolete-in-js).
Are you planning on bumping the major version? If so, it may be just considered a breaking change with details on updating. Not sure how the JS community tends to do this, though. Probably better to mark as deprecated and then remove at some point later.
| QWorkers.registerReader(name, reader); | ||
| } | ||
| const instance = new Quagga(); | ||
| const _context = instance.context; |
There was a problem hiding this comment.
I'd prefer to just access everything from instance itself - keeps it easier to reason about where the data is coming from.
There was a problem hiding this comment.
If that's the only suggestion wrt to the change as is, then I feel pretty good about the change itself being good. I'll definitely take that into consideration.
|
I'm building a blazor component on top of this. I'll try it out. |
|
well, what i haven't done with this, is run it in LiveStream mode, which I think is much more likely where you'll find someone using start/stop. I don't think that's going to change much -- I don't see much use for start/stop for people who are using it to decode on command, but I don't think it should be deprecated, because it is used for LiveStream. Typical initialization goes like
and then sometime after you'll be needing to use stop to turn it off. ... now, if there are people who are controlling the decoder manually, for some reason, rather than just using decodeSingle, then i might need to expose some way to create a new interface... because this doesn't currently allow you to create multiple Quaggas, it takes care of handling that internally. also, yes, i do plan to bump major when i add this, initially, i didn't want to be on version 20.2.10 within a few weeks after i started my work on it :) but now that it's much slower, larger changes, and people are for sure using this now, i'm definitely going to bump major. If nothing else, simply because this is a bug fix that could have wide effects on people using it presently -- fixing decodeSingle to do what it should do, after a few years of it not being right, could really break some people's code. |
Update snyk to the latest version 🚀
Update snyk to the latest version 🚀
… class - after this commit, there should be no functional change, but the bulk of the code outside the static interface has been moved to a class, instead of global variables. An instance of that class is created at the top as a global.
- this appears to actually work correctly, all tests are passing - move stop functionality into the instance - init accepts an instance parameter to determine which instance to init - update bound to instance - add Quagga::setReaders and Quagga::registerReader - setupInputStream defaults to LiveStream per the TypeScript def for inputStream config - InputStreamType back to optional in input stream config
- surprisingly, this seems to work
|
@twsouthwick and anyone else who might want to test this out .. i've just validated that LiveStream works on it after fixing a small error. Not sure if you're wanting to use that or not, but if anyone can validate that this doesn't break anything for them, I think I'm ready to go to 1.0 with it. |
Change Log for @ericblade/quagga2 (0.0.21 → 1.0.0)Breaking Changes
Refactorings
Bug Fixes
|
No description provided.