parallel image processing breaks

[ericblade]

serratus/quaggaJS#319

i suspect that this also has something to do with why the tests don't always complete with the same results.

[TomasHubelbauer]

There is a fork which supposedly has a parallel image processing fix: https://github.com/ad-m/quaggaJS/commits/parallel-process

There is also an outstanding PR in the original repo based on it: serratus/quaggaJS#319

[ericblade]

The referenced bug above from the original repo serratus/quaggaJS#183 has some code that could be adapted pretty quickly to a test-suite to allow for this to be fixed and stay fixed for good.

[ericblade]

I have finally got to validating that this is a problem.

I've just implemented a test for the qr reader, which attempts to simultaneously decode a code-128 barcode and a qr-code, and both receiving callbacks receive the results for the last decodeSingle call.

React code:

  const [barcode, setBarcode] = useState(null);
  useEffect(() => {
    Quagga.decodeSingle({
      ...qconfig,
      src: code128test,
    }, (result) => {
      console.warn('* result return 1', result.codeResult.code);
      setBarcode(result.codeResult.code);
    });
  }, []);

  const [qrcode, setQrcode] = useState('null');
  useEffect(() => {
    Quagga.decodeSingle({
      ...qconfig,
      src: qrcodetest,
    }, (result) => {
      console.warn('* result return 2', result.codeResult.code);
      setQrcode(result.codeResult.code);
    });
  }, []);

Results:

It might take me some time to determine if there's a simple change that will fix it, or if @ad-m 's solution, requiring multiple Quagga instances is best.

At least with a functional test that fails 100% of the time, someone should be able to poke at it and make a determination, which is not something I was willing to do without a test.

Also, I'm not opposed to a major version bump, if it's necessary.

• edit: i have verified that running them serially works

[ericblade]

OK, so, it's a pretty in depth task, BUT, having examined the code while looking at typescript stuff, I've picked up a lot more insight.

"Simply" converting Quagga to a class, and requiring everyone to "new" it, I don't think, is the best answer. It's heavy handed, and requires changes to an interface that I don't think really needs a change. I also have doubts that the existing changesets that implement that have really picked through it, and I'm almost certain that full testing was not performed on them.

As there are basically two different interfaces to Quagga -- Quagga.init() to startup live feed, and Quagga.decodeSingle() to decode individual items. We don't really expect Quagga.init() to happen multiple times, in multiple instances, although it should be possible to support that if it were a possible use case. decodeSingle() however, really needs to support multiple usage, and that should be pretty easy -- decodeSingle can call a callback, or return a promise (recent addition), and either way it should be able to come up with correct results -- although for the case of calling a callback, I'm thinking there's not presently a way to know for what image the callback was called for.

Just wrapping the entire Quagga export in a class definition is a relatively fast but dirty method, and I don't like fast but dirty, especially when it could expose further problems.

The solution to this, I think -- and it's not short term -- is to fix up the core of Quagga so that it can instance the pieces that need to be instanced to support multiple simultaneous decodes, without

A lot of the functions inside the main quagga.js file are presently relying on module-level variables to hold data, and that really harms the ability to properly test things, and it's also the primary cause of this parallel processing issue. Breaking several of these functions out, and accepting function parameters for the internal data, should solve the parallel processing issue, without fundamentally changing the Quagga interface .. and will improve the ability to test the entire system. And could enable things like "process one image in the background while running the camera for further input" or the "multiple cameras running multiple decodes simultaneously" theory above.

[ericblade]

I've just committed a "temporary" (har har, we all know how that goes, right?) fix to force parallel runs of decodeSingle to trigger a setTimeout and re-run the incoming request when it detects that the previous one is completed. Seems to work. Included a test set for it.

#100

so, it's not great, but it at least prevents multiple simultaneous decodeSingle calls from failing with the wrong results.

I think this will also help with a few other issues -- I've noticed in test running, that if I break a barcode reader module, that it cascades into other modules failing, and I see no dependencies on the individual modules to each other.. so my assumption is that when a module fails, something causes it to start running in parallel, which also fails.

If that's the case, then it'll be a little bit easier to convert the existing reader modules to typescript and/or ES6 due to the tests coming out deterministic.

[ericblade]

closing in favor of all-encompassing issue