-
Notifications
You must be signed in to change notification settings - Fork 703
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
Scanner improvements - added the logic from original zxing android project to set the best camera settings #789
base: dev
Are you sure you want to change the base?
Conversation
Improvement for scanning
Scanner improvement for auto focus. Setting the auto focus to rectangle
Updated credits for the file
Update credits
Your PR states the what but not the why. Why are these changes needed? What do they accomplish? |
The issue is with the scanner performance on certain chemical resistant papers. It takes a significant amount of the time (~10 - 30 secs) to scan a barcode. This is reproducible all the time. This degraded behavior makes app unusable. I compared the google's zxing - play store app vs app using Redth/zxing library, zxing's app is scanning on the mark while my app using Redth/zxing library is below par and sometimes not scanning the barcode at all, which makes app unusable. |
@vkalyanapu that's a much better description! can you add it to the commit messages too? otherwise in the future only the what would be discovered by |
foreach (var fpsRange in parameters.SupportedPreviewFpsRange) | ||
{ | ||
if (fpsRange[0] <= selectedFps[0] && fpsRange[1] > selectedFps[1]) | ||
if (fpsRange[1] > selectedFps[1] || fpsRange[1] == selectedFps[1] && fpsRange[0] < selectedFps[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you walk through the logic here please? I'm thinking it might help to use some more parenthesis and add some comments in the code here to explain it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied the suggestion from - #395 (comment)
I agree that the line 306 needs an improvement -
Basically the issue with old line - 253 is that it is selecting the bottom value in the range.
New line should be something like =>
if (fpsRange[1] >= selectedFps[1] && fpsRange[0] < selectedFps[0])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or
if (fpsRange[0] < selectedFps[0] && fpsRange[1] >= selectedFps[1])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is kind of interesting. Probably this issue is right and should pick something with a range from the lowest to the highest.
I think your idea for the logic is probably ok:
if (fpsRange[1] >= selectedFps[1] && fpsRange[0] < selectedFps[0])
This should loop through and keep looking for the widest range... In the case of #395 this logic would end up choosing the 15000,30000 which I think is a good result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the general ideas here, but I'm a bit hesitant to make the center framing the default. I'm not sure in practice if this is ideal or not. I lean towards yes, but there may be some who expect scanning to work outside of this middle frame. I'm also wondering if the ratios and min/max settings are ideal for this or not.
I'm thinking we should at least have a setting to disable the scanning only inside the frame.
I agree, there should be a configuration parameter to set the range. And default will scan entire screen. |
@vkalyanapu are you able to fix that one line of logic as discussed as well as create some sort of configuration parameter(s) for the frame size/range? |
I need to work on the configuration parameter yet. |
Trying this pull request and the scanning performance in Android is much better. |
else if (Build.VERSION.SdkInt >= BuildVersionCodes.IceCreamSandwich && | ||
supportedFocusModes.Contains(Camera.Parameters.FocusModeContinuousPicture)) | ||
parameters.FocusMode = Camera.Parameters.FocusModeContinuousPicture; | ||
else if (supportedFocusModes.Contains(Camera.Parameters.FocusModeContinuousVideo)) | ||
parameters.FocusMode = Camera.Parameters.FocusModeContinuousVideo; | ||
else if (supportedFocusModes.Contains(Camera.Parameters.FocusModeAuto)) | ||
parameters.FocusMode = Camera.Parameters.FocusModeAuto; | ||
else if (supportedFocusModes.Contains(Camera.Parameters.FocusModeFixed)) | ||
parameters.FocusMode = Camera.Parameters.FocusModeFixed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing order of focus selection has completely broke scanner autofocus on my device (Vernee thor).
I added improvements for scanner for android library