Skip to content
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

Updated tests to detect BarcodeFormat & Barcode data #219

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

maheshdev305
Copy link

The existing tests was just reporting the Decoded barcode data alone.
So, updating the tests to Decode barcode data as well as the Barcode Type/Format(i.e. QR_CODE , CODE_128 and so on)

maheshdev305 pushed a commit to maheshdev305/qzxing-test-resources that referenced this pull request Jun 2, 2022
@maheshdev305
Copy link
Author

Updated resources for this tests. So the resources PR-1 needs to be merged as well.

@ftylitak
Copy link
Owner

ftylitak commented Jun 26, 2022

Hello @maheshdev305,

i have two comments:

  • In Qt 5.x it compiles successfully though for in Qt 6 it has compilation errors. Could you check them?
  • The test results (success rate) is far off the previous results. Do you know the reason?
    • Using the old test mechanism: Total: [ 1110 ] , successful: [ 851 ] , failed: [ 237 ]
    • Using your solution : Total: [ 1192 ] , successful: [ 371 ] , failed: [ 821 ]

@maheshdev305
Copy link
Author

@ftylitak

Hello @maheshdev305,

i have two comments:

  • In Qt 5.x it compiles successfully though for in Qt 6 it has compilation errors. Could you check them?

Qt6 compilation errors are present without this PR changes as well. Please check from your side as well.

  • The test results (success rate) is far off the previous results. Do you know the reason?

    • Using the old test mechanism: Total: [ 1110 ] , successful: [ 851 ] , failed: [ 237 ]
    • Using your solution : Total: [ 1192 ] , successful: [ 371 ] , failed: [ 821 ]

Not sure why it was a huge difference at your side. Below is the result counts from my machine.

Total Files :  1192
Decode Success count :  807
Decode Failure count :  385

Attached below is the complete output from my machine. Please do review and let me know.

NewTestResults.txt

@ftylitak
Copy link
Owner

ftylitak commented Jul 11, 2022

a) regarding the compilation errors, I was facing them when building for Qt 6.0. When compiling with Qt 6.2 it was successful.

b) the reason why I am getting low success rate in the tests is because in both Ubuntu and Windows that I have tested, around 500 of the images can not be read for some reason. This resulted the QImage.isNull to be true after this line:

QImage tmpImage = QImage(imageUrl.toLocalFile());
.

Strange enough, the previous implementation of the tests work fine. So, this is the reason why this PR is not yet merged.

I gave it a try though I wasn't able to understand why this happens. Now, since this PR regards the tests and not the core library, I will consider it low priority. This means that even though I am interested in merging it, I currently do not have time to debug why I get this strange behavior of the Null images. If anyone has any suggestion, I would be more than willing to test it ASAP.

@maheshdev305
Copy link
Author

maheshdev305 commented Jul 20, 2022

b) the reason why I am getting low success rate in the tests is because in both Ubuntu and Windows that I have tested, around 500 of the images can not be read for some reason. This resulted the QImage.isNull to be true after this line:

QImage tmpImage = QImage(imageUrl.toLocalFile());

.
Strange enough, the previous implementation of the tests work fine. So, this is the reason why this PR is not yet merged.

I gave it a try though I wasn't able to understand why this happens. Now, since this PR regards the tests and not the core library, I will consider it low priority. This means that even though I am interested in merging it, I currently do not have time to debug why I get this strange behavior of the Null images. If anyone has any suggestion, I would be more than willing to test it ASAP.

Below are my observations...

  • Qt Documentation of QUrl::toLocalFile() says that

    Note: if the path component of this URL contains a non-UTF-8 binary sequence (such as %80), the behaviour of this function is undefined.

    So, Could you validate the file path and check?

  • and below code strips the file:// string in the given path twice!

    qzxing/src/QZXing.cpp

    Lines 574 to 578 in 6ea2b31

    const QString header = "file://";
    QString filePath = imageFilePath;
    if(imageFilePath.startsWith(header))
    filePath = imageFilePath.right(imageFilePath.size() - header.size());

    Below lines adds the file:// string & strips again.

    qzxing/src/QZXing.cpp

    Lines 580 to 581 in 6ea2b31

    QUrl imageUrl = QUrl::fromLocalFile(filePath);
    QImage tmpImage = QImage(imageUrl.toLocalFile());

Also, requesting you to consider this as a little higher priority and let me know the results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants