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

QR code generated by Catima cannot be read by Verify Ontario #506

Open
toolstack opened this issue Oct 17, 2021 · 20 comments
Open

QR code generated by Catima cannot be read by Verify Ontario #506

toolstack opened this issue Oct 17, 2021 · 20 comments
Labels
common: uncommon Most users are unlikely to come across this or unexpected workflow severity: minor Impairs non-critical functionality or suitable workarounds exist subject: store quirk Something non-standard related to a specific store type: bug Something isn't working

Comments

@toolstack
Copy link

Smart Health Cards standard for COVID vaccination QR receipts are used in several Canadian provinces, but Catima generates a QR code that looks very different from the source QR code and cannot be read by the Verify Ontario app.

For example here is a sample QR code that looks similar to the official QR codes created by Ontario:

sample-qr-code

And what Catima generates after reading in the data:

Screenshot_20211017-133542

Note that this is valid QR code, and can be read by the shc-extractor script from which the sample data is taken, but Verify Ontario cannot read it.

It looks like the official QR code is a 4x4 pattern while Catima generates a 6x6 code. Catima is also not alone in generating this 6x6 style QR code from the data, another QR app I tried does the same thing.

@TheLastProject
Copy link
Member

I first thought Catima might use another error correction level, but that isn't it. They're both L.

This issue sounds extremely similar to #244.

Looking at this commit, it seems that we may be able to fix this after next ZXING release: zxing/zxing@1287751

While honestly I believe it is a bug in the Verify Ontaria app if it can't read this, Catima should indeed still try to output the same as was given as input and that currently isn't happening.

@TheLastProject TheLastProject added state: blocked by upstream Can't currently be done due to a limitation of Android or another upstream type: bug Something isn't working labels Oct 17, 2021
@toolstack
Copy link
Author

While honestly I believe it is a bug in the Verify Ontaria app if it can't read this, Catima should indeed still try to output the same as was given as input and that currently isn't happening.

Agreed on both points 😄

It looks like zxing is in matinanace mode, no "active" development, so it may be a while before a release is done (though that commit was just done today, so mabye?). If they don't release something shortly any thoughts on pulling in the patched version before the release is complete?

@toolstack
Copy link
Author

If they don't release something shortly any thoughts on pulling in the patched version before the release is complete?

I can confirm that using the current snapshot of ZXing, along with adding the QR_COMPACT hint to the Catima code, that the QR code is generated in a 4x4 matrix and is read correctly by the Verify Ontario app.

@TheLastProject
Copy link
Member

I don't really want to use a snapshot version, because zxing is extremely important and we can't risk breakage there, but I would accept a PR that starts implementing recognizing the type of scanned code and saves that to generate the exact same code type.

But make sure to talk to @gschwaer if you want to take that on, because this month is Hacktoberfest so I don't want to risk passing up on the person who found such an issue first.

I probably won't merge anything until zxing releases something new, but I would mark it hacktoberfest-accepted as it is useful preparation work

@toolstack
Copy link
Author

toolstack commented Oct 18, 2021

Would there be any case in which you don't want the smallest possible QR code?

If not, just adding the QR_COMPACT hint to all QR code generation would seem like the easiest fix. It's always going to return a valid QR code.

I don't see any obvious API from xzing to return what kind of QR code is decoded, just the data seems to be returned.

The only other thought I'd have if using it as default isn't tenable, would be to add a new barcode type to Catima, something like "QR Code Compact" which would use the hint, but be user selectable.

@TheLastProject
Copy link
Member

Would there be any case in which you don't want the smallest possible QR code?

Yes, I don't want to suddenly start using a different type of QR code because it will confuse users.

Besides that, I have no clue if this compacting does anything for the error correction level.

I don't see any obvious API from xzing to return what kind of QR code is decoded, just the data seems to be returned.

I thought @gschwaer implemented this for Code128 in zxing/zxing#1411, but it seems now like they only implemented the creating part, not the returning-what-was-scanned part, unless I'm misunderstanding the code :/

The only other thought I'd have if using it as default isn't tenable, would be to add a new barcode type to Catima, something like "QR Code Compact" which would use the hint, but be user selectable.

I do want something like this in general, but standardized because we already identified both QR and CODE 128 has multiple valid variants. In general (with proper readers), it also shouldn't matter, so this should perhaps be less in-your-face than regular different code types to not overload the user with options.

@toolstack
Copy link
Author

I do want something like this in general, but standardized because we already identified both QR and CODE 128 has multiple valid variants. In general

Perhaps a third drop down on the "Barcode" tab when editing called "Subtype" or "Barcode Options". For QR it could be "Normal"/"None" (aka current behaviour) and "Compact".

@TheLastProject
Copy link
Member

Okay, hearing this discussed more... lets just go for a regular list entry after all

So "QR Code (Compact)". And also we will use "Code 128 (A)" and "Code 128 (B)" and stuff. Simple and clear it's a subtype.

Making these selectable in the dropdown would be a good first step, probably with a new DB field called "BARCODE_SUBTYPE" which is null by default, for easier compat with zxing internals

@gschwaer
Copy link

gschwaer commented Oct 27, 2021

I thought @gschwaer implemented this for Code128 in zxing/zxing#1411, but it seems now like they only implemented the creating part, not the returning-what-was-scanned part, unless I'm misunderstanding the code :/

Yes. If I remember correctly, the issue is that zxing has no support built in for returning more than the decoded data, the main QR code type, and the raw data. Also Code-128 can actually use mixed encoding, so not just A or B, but for example A and then at letter 6 switch to B and then after letter 10 back to A. So to find a way to express this is already complicated. But since for Catima, the actual subcode is of interest, we can parse the raw bytes and thus find out, if a single subtype was used and if so which one. For the QR code generation we can then force the selected subtype via the patch I implemented. That is the plan.

So "QR Code (Compact)". And also we will use "Code 128 (A)" and "Code 128 (B)" and stuff. Simple and clear it's a subtype.

Sounds good. Just keep in mind that there should maybe be a fallback type in case a mixed encoding is encountered.

@TheLastProject
Copy link
Member

zxing has no support built in for returning more than the decoded data, the main QR code type, and the raw data [...] But since for Catima, the actual subcode is of interest, we can parse the raw bytes and thus find out, if a single subtype was used and if so which one.

Okay, that makes sense. What is this "raw data" like?

@gschwaer
Copy link

If I remember correctly you call OneDReader.decode(bitmap) at some point. This should return a Result, which contains a member called byte[] rawBytes. Those raw bytes can be checked for the codes here which specify the subcode. If more than one of them are present it is of mixed subtype. Not sure if it is possible that none is present.
Something like (pseudo python code):

CODE_C = 99
CODE_B = 100
CODE_A = 101
MIXED = 42

result = OneDReader.decode()
subcode = None
for byte in result.rawBytes:
    if byte in (CODE_A, CODE_B, CODE_C):
        if subcode is not None:
            subcode = MIXED
            break
        subcode = byte
if subcode == None:
    raise MaybeSomeError('No subcode type found.')

@TheLastProject
Copy link
Member

Hmm, I wonder if we can move this code into zxing, as it seems weird for Catima to read internals itself instead of ask zxing. But thank you for the info, that helps :)

@gschwaer
Copy link

That is true, but it is also kind of a special case that is only really interesting when you want to restore the QR code 1-to-1 and use Code128. I think the chances of the zxing maintainers accepting such a feature are slim considering zxing is in maintenance mode only.

@TheLastProject
Copy link
Member

Well, of course we would want a generic method for Code128/QR/whatever. But I do realize getting the other code in already took quite a while yeah

@TheLastProject TheLastProject added common: uncommon Most users are unlikely to come across this or unexpected workflow severity: minor Impairs non-critical functionality or suitable workarounds exist subject: store quirk Something non-standard related to a specific store labels Feb 5, 2022
@Altonss
Copy link
Contributor

Altonss commented May 1, 2022

A new version of zxing has been released integrating the desired changes :)

@TheLastProject
Copy link
Member

Okay, now we just need to:

  1. Create a new database table to store the EncodingHints in (as there may be more than 1 EncodingHints per barcode)
  2. Save the relevant EncodingHints during scanning
  3. Load the relevant EncodingHints during DB load of a barcode

Still gonna be a fair bit of code, but should be doable.

@TheLastProject TheLastProject removed the state: blocked by upstream Can't currently be done due to a limitation of Android or another upstream label May 4, 2022
@TheLastProject
Copy link
Member

TheLastProject commented May 11, 2022

Thinking about this more, it probably makes more sense to just implement only the EncodingHints we know what to do with as new colums in the LoyaltyCardDbIds table. They should then be nullable (null = unset).

@Altonss
Copy link
Contributor

Altonss commented Oct 18, 2022

Thinking about this more, it probably makes more sense to just implement only the EncodingHints we know what to do with as new colums in the LoyaltyCardDbIds table. They should then be nullable (null = unset).

So which encoding hints would be relevant to add? I'm thinking about picking up this task as I already have some experience with writing database changes in Catima, but this might be a bit of work ^^

Edit: If I understood correctly, the both relevant encoding hints for the moment would be FORCE_CODE_SET and QR_COMPACT, am I right?

@TheLastProject
Copy link
Member

For this specific issue only QR_COMPACT seems necessary, but we'll generally need to support more hints.

I think this will be easier to do if we refactor the whole CatimaBarcode class first. I started with this in the fix/barcodeRefactor branch but I haven't had time to work more on this

@Altonss
Copy link
Contributor

Altonss commented Oct 18, 2022

For this specific issue only QR_COMPACT seems necessary, but we'll generally need to support more hints.

I think this will be easier to do if we refactor the whole CatimaBarcode class first. I started with this in the fix/barcodeRefactor branch but I haven't had time to work more on this

Thanks, I will have a look at this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common: uncommon Most users are unlikely to come across this or unexpected workflow severity: minor Impairs non-critical functionality or suitable workarounds exist subject: store quirk Something non-standard related to a specific store type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants