zxing-js / library

Multi-format 1D/2D barcode image processing library, usable in JavaScript ecosystem.
https://zxing-js.github.io/library/
Apache License 2.0
2.48k stars 547 forks source link

NotFoundException slowing this down by lots... #323

Open Salketer opened 4 years ago

Salketer commented 4 years ago

Hello!

I've worked pretty hard on speeding up my barcode scanner as much as I could. You can see a snippet of my code to crop the video feed, this has divided by 3 the processing time here!

https://github.com/zxing-js/library/issues/305

Another big point seems to be the NotFoundException which is a ts-custom-error. Using chrome inspection, I have took a sample of 3128 ms while scanning. Of those 3128 ms, 2241ms are using for creating those exceptions.

1026ms by the NotFoundException constructor 1185ms by the fixStack method called by that constructor.

I am not sure if native Errors would fix the issue, make it worse or better. But I am sure there is something better to do in this field. One would argue that errors shouldn't be flying at high velocity like they are in this library. They are meant to be used when something went wrong, while here we simply expect it to happen quite often and does not mean something went wrong.

My guess is that using native Errors would help as Error.captureStackTrace would not be used every time (I doubt this is very useful for this kind of exception).

Another approach (that feels a bit hacky) would be to keep sending the same instance of NotFoundException. The idea is that the call stack would be the same anyways since it is called from a setTimeout, the stack is new. The only one that has a better stack is the first one thrown as it starts from the setup promise fulfill. Throwing the same exception again and again poses no real risk, we could freeze it just in case. This method would benefit from saving the time to create the new Error object which is very time consuming when done that many times.

My third suggestion would be to get rid of the throwing mechanic to prefer returning a null/false/empty value when a code is not found.

In case you wonder: I was on Chrome 83, with a 640x480 webcam displaying on a 375x812 sized debug screen. And this setup code:

      const config = new Map();
      config.set(ZXing.DecodeHintType.TRY_HARDER, true);
      config.set(ZXing.DecodeHintType.POSSIBLE_FORMATS, [/* ZXing.BarcodeFormat.EAN_8, */ZXing.BarcodeFormat.EAN_13]);

      this.codeReader = new ZXing.BrowserMultiFormatReader(config);
      this.codeReader.timeBetweenDecodingAttempts = 10;
hyansuper commented 4 years ago

I prefer returning null when there's no barcode/qrcode in the image

github-actions[bot] commented 4 years ago

Stale issue message

odahcam commented 4 years ago

These exceptions are a recurrent discussion here and I do think they will continue for a while, as we are just a port from the Java source at this point. My biggest concern would be: how to stop throwing this odd exceptions and at the same time keep the library working as you would expect the Java version to work?

kenlyon commented 3 years ago

I think that the amount of exception logging here is way over the top. It might be useful in some kind of verbose troubleshooting mode, but for normal production use, I don't want my console being flooded with "exceptions" just because the program hasn't yet detected a barcode in the camera image.

Could we consider changing this to some kind of optional setting? I would suggest that all five places where you use console.log() within a catch block should be conditional.

If you're concerned about backwards compatibility, you could make the default behaviour the current and have to opt in to hide the logging. (Although my preference would be the opposite. I can't imagine those errors are useful.)

odahcam commented 3 years ago

We are simply porting from Java, which behaves this way.

While I agree with you, there are/were more important things on or way than changing the original behavior of the library without enough knowledge for that. Working on these exceptions is in our roadmap now so, with patience, this will be solved.

PRs welcome.

Discussions actually pointing ways to solve this are also welcome.

kenlyon commented 3 years ago

@odahcam Thanks for your reply. Is the main goal of this project to port the Java version to JavaScript without changing any functionality? I agree that it would be useful to know why the exceptions are happening to begin with rather than just silencing them. However, as a consumer of this package I just want to be able to use it. I'm not really able to invest the time in learning how it works or why it's happening.

odahcam commented 3 years ago

Is the main goal of this project to port the Java version to JavaScript without changing any functionality?

Yes!

On top of that we add extra functionalities, like the browser layer, that is becoming a new package now and some other stuff we're creating. We do that as extensions, not changes in the ported source.

gremmil commented 3 years ago

Hello everyone, I am not very good with English, but I have the same problem. What I discovered is that these errors are thrown when the BarcodeFormat.RSS_EXPANDED is included in the scan formats. Use the @ zxing / ngx-scanner library, which uses @ zxing / library and @ zxing / browser

cmawhorter commented 2 years ago

the most important part of OPs post was kinda glossed over:

ts-custom-error [...] My guess is that using native Errors would help as Error.captureStackTrace would not be used every time (I doubt this is very useful for this kind of exception).

FWICT, it'd be pretty simple to remove capturing stack from the ts-custom-error file and just copy that file here sans fixStack call. nothing fancy is happening there.