search

zxing-cpp / zxing-cpp

C++ port of ZXing
Apache License 2.0
1.29k stars 407 forks source link

Inconsistent naming of the "tryHarder" feature #104

Open axxel opened 5 years ago

axxel commented 5 years ago

I noticed that there seems to be a divergent use of names for the DecodeHints::setShouldTryHarder() functionality: @huycn called this flag 'fast' / 'fastMode' in the scan_image example @timrae followed that example in his python wrapper @dabrain34 used the term 'TRY_FASTER' in his gstreamer plugin @swex sticked to 'tryHarder' in his Qt wrapper

I would like to suggest to come to an agreement about how we'd like to call that flag and name it consistently (before it is "too late"). As I see it, the basic difference between the terms tryHarder and fastMode is that they imply a different default value for the underlying functionality. I believe the upstream term tryHarder (next to the similar tryRotate) is the more appropriate one, as the word implies both (better results but also longer execution time) where as a simple 'fastMode' sounds as if you would always want that (fast == good, right?).

Ideally, we would not need either of those flags if the code were simply fast enough with both enabled.

However we end up calling it, I'd like to remove the word "should" from the DecodeHints methods. I believe it is unnecessarily verbose and also misleading: as if the computer would have a say in whether or not he will actually do what he is asked to :).

Anyone who has an opinion about the matter, please let me know.

timrae commented 5 years ago

I agree that it's mainly a matter of what the default is. IMHO if the default is accuracy over performance (which I vote for) then call it fast mode and set to off by default. If the default is performance over accuracy then call it try harder and set to off by default.

axxel commented 5 years ago

I thought a bit more about what this whole thing is actually good for and what exactly it is doing. And that is: it is bailing out early (before it tried to look at "every pixel"). Then the question is: why would you want to do that? I think the only application is in real-time (think mobile app) situations where you want to limit the time spent on one frame because you already have the next frame and that might be more likely to contain something good. Then what you really want is something like this:

That could also subsume the tryRotate option in the sense that it could search both rotations in parallel.

My suggestion would then be to replace tryHarder with setTimeout(int ms) which would default to 0 (meaning no timeout == tryHarder). As long as the underlying code is not changed, yet, we could simply interpret it such that anything other than 0 means !tryHarder. This way, the interface would work for future improvements and the default case would be as preferred by @timrae, @dabrain34 (and me).

huycn commented 5 years ago

I believe a setTimeout() would be harder to use than two flags tryHarder and tryRotate. Most of times, you don't know exactly what is the exact frame rates you are going to get. It's more likely you known your exact usecase, thus deciding tryHarder and tryRotate would be obvious (given of course you understand what these flags do).

Also, trying to respect the given timeout will not be as simple as it may appears and will likely introduce some overheads, both in runtime performance and code maintenance.

I would prefer the core library offers as many as possible options to tune its behavior and any "smart" feature would be implemented in layers on-top of it.

Thus, for the core library, I would avoid rename the flags to avoid confusion and remain compatible with upstream ZXing. For the layer above, I find it's correct (and preferable) to rename the flag when it implies different default.

axxel commented 5 years ago

I did not mean to suggest that the timeout would have to be followed exactly for this to be useful. Hence, you don't need to know your exact framerate anyway. Neither currently visible versions (tryHarder / fastMode) tell you anything about the 'speedup' you'd get, nor is it documented somewhere (nor do I know it, even though I spent a considerable time on improving various performance aspects). So you have an option that changes the behavior of the lib in an unknown way (for anyone who did not study the source code). I fail to see how it would be obvious to choose the best setting as things are right now. The code comment Spend more time to try to find a barcode; optimize for accuracy, not speed. is not really helpful there either.

And in case you really needed that "smart" feature, you can't implement that on top of the current API. You said, you wanted 'as many as possible options'. Such a timeout option would be a superset of the tryHarder bool. You could remove the timing-aspect from it and call it something like 'speedup' or 'skipFactor' (as in skipping lines, which is what it does) and allow you to have a more fine grained control (instead of just two levels).

Anyway, maybe it is best to wait and see if my other speedup experiments are going to be fruitful and if the tryHarder version is then fast enough. Then there would be no point in having the option (which I believe would obviously be the most user friendly version).

Lastly: I can follow your argument for having the sample program call that option something else if that fits the purpose as it is a higher level abstraction. But if you argue that the core lib should stay consistent with upstream, then I believe all wrappers (python, Qt, gstreamer) should stick with the same naming as they are conceptually the same thing (providing a library API for the upstream functionality). Interestingly, everyone in this discussion seems to agree that the correct default value should be tryHarder==true, meaning all agree that the upstream naming choice is 'wrong'?

Apart from all that: what about my suggestion to drop the 'should' fill word from the method names? (They are not part of the upstream names, either).

huycn commented 5 years ago

I agree that there is not enough information on what kind of thing the flag changes. Maybe we should provide more documentation on how to use the API. Is there any volunteer? :)

For the timeout, if we don't follow the provided value, maybe it's better to stick with a flag? I understand that opens the door for extension in the future, but maybe I can add it when we are ready to do that, since it's a behavior change anyway.

You are right, we cannot implement timeout on-top of current API. We need to provide kind-of callback, mutable boolean or something similar. But from my point of view as user, if I have a time constraint, I would go with multiple instances of readers, each configured for detect one kind of barcode only, and run them in parallel. This way, I have total control instead of letting this library to decide which codes to scan, which codes to give up when time's up. If it still takes too much time for scan one type of of barcode in one image, the problem should be something else, captured image is too big for example, in which case no option can help.

For the flag name, I don't have too much opinion on what name we should take in wrapper, as it would depend on what default value you want it to be, based on "common usage" of the wrapper. The upstream ZXing project was created for an app running on Android, thus the default value tryHarder==false is a logical choice. For a python wrapper, tryHarder==true would be more logical since python users don't care about performance ;) (just kidding of course, but you get my point).

I totally agree with you to drop of word 'should'. I have no memory how it was introduced, but it's obviously superfluous.

enzo-simone commented 5 years ago

Hello all, I would like to express my humble opinion.

I would keep the library as simple as possible, so no support out-of-the-box for timeout (or thread safety). The library should be easy to use just for what it was designed for, without worries about the 'environment' in which it will be working. It would be up to users to wrap it according to the environment, i.e. using parallel and/or time-constrained 'tasks' (or locks for multi-threaded access).

For example, concerning a barcode reader, I would personally like to be able to include a single header file (see #102) and create an instance of an object which does its best, i.e. tryHarder==true and tryRotate==true; to say it all, I currently wrapped the library in an object that by default has all setPossibleFormats. If time would be a constraint I would enable only needed decoders and split decoding in different tasks.

axxel commented 5 years ago

Thanks for all your input.

I'll drop the timeout idea then (which was mainly supposed to be a 'clear/obvious' solution to the question of how to name the API in a self-explanatory way). I still hope I made a convincing case for the idea to keep the naming of the same thing consistent across the different wrappers but will not change any of those myself.

I will remove the 'should' string, though.

axxel commented 4 years ago

I'd like to revisit this subject once more. The discussion above clearly showed the majority of people who expressed an opinion believe the default setting for the decoding should be "try your very best to get me a valid result" (i.e. tryHarder(true) and tryRotate(true)). I strongly believe that those should be the default. Setting them to false is only good to increase performance. The other related options setFormat and the new setIsPure and setBinarizer are also there to increase speed while limiting the search space but their default is "try everything".

If you don't agree, please let me know.

axxel commented 2 years ago

In the context of #333 I'm currently trying to 'cleanup up' the API as much as possible before releasing a 2.0 version. As you can see, the tryHarder flag has been annoying me for years. This flag only really makes any sense in mobile contexts. Therefore I'd like to ask @markusfisch @parallaxe and @vkrause: are you using the DecodeHints::tryHarder flag in your applications? If so, do you let the user choose or simply set it to false or maybe use some auto-tuning based on your hardware performance?

markusfisch commented 2 years ago

Yes, I'm using tryHarder as a preference setting so a user can choose to activate it.

This makes sense for very large 2D barcodes with many very small modules (QR Code, PDF417). There, tryHarder can make a noticeable difference in my experience, especially when the camera isn't very good.

vkrause commented 2 years ago

are you using the DecodeHints::tryHarder flag in your applications? If so, do you let the user choose or simply set it to false or maybe use some auto-tuning based on your hardware performance?

What does this actually do? :) I think we are not touching this at all, even on mobile performance seems sufficient to run this at the full frame rate here.

axxel commented 2 years ago

@markusfisch so tryHarder = false is your default and sometimes you or the user find it helpful/necessary to set it to true? The question is: do you find it necessary to set it to false to meet your performance requirements?

@vkrause It basically limits the search space by skipping more lines and doing an implicit crop to the centered half of the input. The price is an overall lower success rate. This was especially useful in the upstream code. To illustrate: with our own android demo app where I can switch between the Java and C++ version I see a runtime per full frame on my Pixel3 of

markusfisch commented 2 years ago

so tryHarder = false is your default and sometimes you or the user find it helpful/necessary to set it to true?

Correct.

The question is: do you find it necessary to set it to false to meet your performance requirements?

Yes. As your tests on a Pixel3 show, for the Java upstream version, this can make all the difference between useable and not. 250ms is really slow - and this is a really good device. Now, the C++ fork is a lot faster, of course, but there are a lot devices that are far weaker than a Pixel 3.

5ms doesn't sound like much, but we also have applications that do image processing after a QR Code (for example) was found. On the same camera frame. So 5ms are quite valuable there. And on a bad device, it can be more than these 5ms.

When we are processing a live camera stream on a range of differently performing devices, speed is really important.