Repository navigation
Datamatrix GS1 fixes - #99
Merged
Merged
Conversation
Upstream zxing excludes the sentinel with an upper bound of 999, which was lost in the port.
The minimal encoder prepends the FNC1 codeword of a GS1 symbol once the solution has been assembled, so every edge computed its size one codeword short. When the message without the FNC1 exactly filled a symbol, the encoder concluded that the trailing C40, Text or X12 run needed no unlatch. The FNC1 then pushed the message into the next symbol size and the padding of that symbol was decoded as text: "01012345678901281720010110ABC123" decoded as "01012345678901281720010110ABC123GR2u". Upstream zxing has the same defect.
The C40 versus X12 tie break in the Data Matrix lookahead searches for an X12 terminator that comes before a character that X12 cannot encode. It iterated the message from index 0 instead of from the position the lookahead stopped at, so it answered the question for an unrelated part of the input, and the position variable it computed was never read. Upstream zxing scans from startpos + charsProcessed + 1. Over a corpus of 11260 inputs the two disagree on 570 of them: the corrected version selects a smaller symbol for 67 and a larger one for 40, the heuristic in the specification is not optimal.
The minimal encoder sliced the macro header and trailer off the message with character counts used as byte offsets. A macro message holding any non ASCII character either sliced inside a character, which panics, or lost trailing bytes. Both affixes are ASCII, so their byte lengths are also their character lengths.
hschimke
approved these changes
Sep 16, 2026
hschimke
left a comment
hschimke
left a comment
Collaborator
There was a problem hiding this comment.
looks like all tests pass and changes look good, approving for merge.
Collaborator
|
Looking through these I see exactly how I messed them up while porting. I had a huge set of very similar issues that I found because they broke things more obviously when I first started integration testing. Thank you for this patch. I'm going to bundle this and a bunch of other fixes up and do a release before the end of the week. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This condition on minimal ECI input got lost in the port:
https://github.com/zxing/zxing/blob/33dfdefcb35576612841e614d44ba9edc9aee2b5/core/src/main/java/com/google/zxing/common/MinimalECIInput.java#L162
FNC1 characters aren't accounted for in determining datamatrix edge sizing, so some content could get cut short.
Fixes a couple differences compared to the java implementation