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

Code 128 Subtype is Changed #244

Open
gschwaer opened this issue Jun 23, 2021 · 13 comments
Open

Code 128 Subtype is Changed #244

gschwaer opened this issue Jun 23, 2021 · 13 comments
Labels
common: occasional Affects or can be seen by some users regularly or most users rarely severity: major Severely degrades major functionality or product features, with no satisfactory workaround type: bug Something isn't working

Comments

@gschwaer
Copy link

gschwaer commented Jun 23, 2021

Hi,

When scanning a Code 128 bar code, the displayed bar code does not match. The code below contains the "0013002994" as Code 128 (Subtype B). When scanning, saving, and then displaying the bar code, it's shown in Code 128 (Subtype C) format. It seems the subtype is not saved.

Steps to reproduce:

  1. Scan this code (Code 128B):
    code84004618b9c0e431027589c7de43b2cb
  2. Save it and display it
  3. It will look like this code (Code 128C):
    codeb7a2ba53d0843ce645f1d9b859e14f15

The same behavior is also present in Loyalty Card Keychain.

I used this online generator to create these pictures, which is also how I found out the sub-types of the original and the displayed bar code.

I have no experience with Android programming, but browsing through the code, it looks like new MultiFormatReader().decode(binaryBitmap).getBarcodeFormat().name() is used here to get the bar code type as string ("CODE_128" in this case). It, however, does not contain the subtype. Maybe this is somewhere in com.google.zxing.ResultMetadataType?

Possibly related (though other bar code type): #44

@TheLastProject TheLastProject added the type: bug Something isn't working label Jun 24, 2021
@gschwaer
Copy link
Author

gschwaer commented Jun 24, 2021

Hi,
I actually dug a little deeper and I think the problem is a bit more annoying than initially thought:

  1. com.google.zxing.oned.Code128Reader doesn't seem to expose the detected subtype.
  2. com.google.zxing.oned.Code128Writer doesn't seem to accept a subtype, but always infers it from the content.
  3. The GS1 standard specifying Code-128 allows for subtype change within a bar code (see 5.4.3.2). So bar codes may not be pure subtype A, B, or C.

I see three options how this could be handled:

  1. (Notification of limitation) Let the user know, when Code-128 is detected, that it may differ from the original. Maybe something like: "For Code-128 the generated bar code can deviate. Please compare the generated code with the original. More information."
  2. (Semi-automatic) Kindly ask the zxing developers to add support for an option which forces a specific subtype in Code128Writer, such that chooseCode(..) returns it (if given). I don't know the details on how the MultiFormatWriter is hooked to the Code128Writer, but the MultiFormatWriter provides some Map<EncodeHintType,?> hints that could contain the forced subtype. Then in Catima let the user choose which subtype to use.
  3. (Fully-automatic) Same as 2. but also ask for a hint from the Code128Reader if the code was enum { pure_subtype_A, pure_subtype_B, pure_subtype_C, mixed_subtypes }. Then in Catima save the subtype hint additional to the code and use it when generating the output bar code.

I guess the first option is the easiest to implement. For the other two, I don't know how much effort that would be and how libraries on Android are bundled. Maybe feedback from the zxing developers would help. What do you think?

And @TheLastProject: Thank you so much for your active efforts on this and other projects! You are a blessing! 🙇

@TheLastProject
Copy link
Member

Thanks for the deep research! This is very helpful info.

I am thinking what would possibly be the simplest, also for the ZXING developers, is to just add these subtypes to BarcodeFormat.

That way, we could just save the barcode as CODE_128_A or CODE_128_B or so in the code after we read it (which xzing would return) and we could pass the CODE_128_A or CODE_128_B as BarcodeType to explicitly tell zxing to generate that subtype.

What do you think? Would that be worth suggesting to the zxing developers? If you don't mind, I feel it would be best for you to file such an issue with them as you seem to understand the details better than I do.

@gschwaer
Copy link
Author

gschwaer commented Jun 25, 2021

I think they will not like that because the subtype can be changed at any point in the barcode, so the solution with CODE_128_A, .. would only apply if the subtype is not changed. Basically you would need a subtype per character (or per two characters in case of subtype C), which is not nice to handle. Changing the enum will probably break to much code that uses this library. I will write and issue an ask the zxing developers about their opinion on this matter.
Edit: Here is the question in the zxing group.

@gschwaer
Copy link
Author

I realized that we can actually figure out the code set (subtype) of the scanned bar code from the raw bytes, using a few lines of code in Catima. But there is currently no way to use the code set when generating a new bar code (see my description in the zxing group).

@gschwaer
Copy link
Author

gschwaer commented Jun 29, 2021

In the zxing group it was suggested to modify the code (for example to accept a hint to force the encoding).
@TheLastProject Is changing zxing code viable? I just saw this comment in the build.gradle:

// Do not upgrade past 3.3.3! Causes a crash on versions before Android Nougat
//noinspection GradleDependency
implementation 'com.google.zxing:core:3.3.3'

Currently zxing is at 3.4.1.

@TheLastProject
Copy link
Member

The zxing team made a small change (not even a functionality change) that broke everything, see #147 and zxing/zxing#1170.

Right now, I just haven't updated because there isn't much new really worth it. But I can try to figure out how to maintain a fork of zxing if there are meaningful updates in zxing. Maybe I can work together with @markusfisch there, given he seems to have a similar opinion to me: markusfisch/BinaryEye#190 there.

So don't worry, I'll make sure we can upgrade zxing if a meaningful patch is introduced there.

@TheLastProject
Copy link
Member

Hmm, it seems the mailing list kinda... went quiet. Maybe you should just make that PR to zxing and see what they say?

@gschwaer
Copy link
Author

I ported the change to the master branch and opened a PR: zxing/zxing#1411

@TheLastProject
Copy link
Member

I noticed a new FAQ entry in the zxing FAQ which allows me to upgrade the zxing version used without dropping support for old devices, so I did that in cbcf1bc.

So Catima is already halfway ready for when your patch gets merged (just need to add support for saving the code set and rendering a specific code set). Let's hope they will accept your patch :)

@gschwaer
Copy link
Author

My change was merged into zxing master yesterday. 🎉

I think the points left to implement in Catima are:

  1. When adding a new code in the app and Code-128 was detected, check if the raw bytes contain any of the code switching symbols:
    private static final int CODE_CODE_A = 101;
    private static final int CODE_CODE_B = 100;
    private static final int CODE_CODE_C = 99;
    
    If so then the barcode uses mixed types. (Then we generate the barcode the usual way. It is still possible that the code set switching is performed differently by the generator than the original. But to cope with that is out of scope for this issue IMO.)
    If none of the switching codes are present in the raw bytes, then the code set can be determined by checking the first byte, which should be one of
    private static final int CODE_START_A = 103;
    private static final int CODE_START_B = 104;
    private static final int CODE_START_C = 105;
    
    This we safe as the code set with the content in the database.
  2. When generating a barcode we check if the code set is set and if so pass it as the hint FORCE_CODE_SET to the writer.

Does this make sense?

@TheLastProject
Copy link
Member

Kinda, although the variables you name are all listed as "private" so I feel that is probably not the cleanest way to do things.

Anyway, after the new zxing release is out I will:

  1. Update
  2. Write an unit test to ensure your original example in the issue works
  3. Write code to make sure it works

So, for now, let's wait for zxing to do a new release. Thank you for all your help so far!

@TheLastProject TheLastProject added the state: blocked by upstream Can't currently be done due to a limitation of Android or another upstream label Jul 30, 2021
@Altonss
Copy link
Contributor

Altonss commented May 1, 2022

A new version of zxing has been released integrating the changes 👍

@TheLastProject
Copy link
Member

#506 (comment)

@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 TheLastProject added severity: major Severely degrades major functionality or product features, with no satisfactory workaround common: occasional Affects or can be seen by some users regularly or most users rarely labels Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common: occasional Affects or can be seen by some users regularly or most users rarely severity: major Severely degrades major functionality or product features, with no satisfactory workaround type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants