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

Add multi-eci decoding for PDF417 #1507

Merged
merged 3 commits into from Mar 9, 2022

Conversation

AlexGeller1
Copy link
Contributor

@AlexGeller1 AlexGeller1 commented Mar 7, 2022

Also fixed issue that some multi-eci encoded PDF417 codes were missing ECIs
The unit test now also decodes the multi-eci encoding tests and verifies that the result is identical with the input.

- Fixed issue that some multi-eci encoded PDF417 codes were missing ECIs
Copy link
Contributor

@srowen srowen left a comment

Minor comments.
Seems OK if this strictly increases the number of symbols that can be decoded

@AlexGeller1
Copy link
Contributor Author

AlexGeller1 commented Mar 8, 2022

Seems OK if this strictly increases the number of symbols that can be decoded

Great. Yes, it does just that and it also fixes a bug in the encoding. Regarding the risks of this PR, I have the following thoughts:

  • Regarding the encoding bug fix. The fix is in the function PDF417HighLevelEncoder.encodeMultiECIBinary(). This function is only called if the encoding hint PDF417_AUTO_ECI is set so that the default behavior is not affected.
  • DecodedBitStreamParser line 105 .. 113: The removed code was newly introduced in the previous PR (Add support for multi-eci encoding for PDF417 #1506) to fix the issue that the decoder was failing to decode a single ECI code with no leading latch. The fix is no longer needed since the method textCompaction now handles ECIs. In other words, the change reverts the code to the previous state.
  • Changes to DecodedBitStreamParser.textCompaction(): The function now handles ECI_CHARSET in the switch case. There is no other behavior change. Before the change the ECI was ignored. As long as the new treatment doesn't crash, it may in the worse case replaces one buggy behavior with another.
  • Changes to DecodedBitStreamParser.decodeTextCompaction(): There is no behavioral change. The method can now be instructed to start in a particular state. In the default case (no ECI) it starts with the original state Mode.ALPHA (see line 286). The method now returns the last state that it latched to.

@AlexGeller1
Copy link
Contributor Author

AlexGeller1 commented Mar 8, 2022

Please don't merge it yet. The spec says that ECIs may occur in the middle of 901 data (less than 6 bytes) and at the boundaries of 924 blocks (every 5 codewords) without requiring a new latch. The encoder never creates that but I am making sure that the decoder can handle it.

…ocations in binary encoded data as specified in section 5.5.3.2 of the spec

- Added verifying unit test
@AlexGeller1
Copy link
Contributor Author

AlexGeller1 commented Mar 8, 2022

I condensed the code of the function DecodedBitStreamParser.byteCompaction() so that it is about half as long as before.
I think that it is pretty straightforward now. The previous code didn't lend itself well to add the extension.

srowen
srowen approved these changes Mar 8, 2022
@srowen srowen merged commit ce1a1a5 into zxing:master Mar 9, 2022
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants