Fix entropy range encoding in octree-based pointcloud compression #3579
Conversation
This file contains 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
|
@nicolas-cadart Thanks for the fix! Is it possible to provide a test case too? It'll prevent regressions. The tests are located in test/io/test_range_coder.cpp Maybe reorder the commits so that the tests come before the fix. |
I modified the test to deal with a problematic size. This test fails without the fix, and is OK with it. |
kunaltyagi
reviewed
Jan 21, 2020
Size of frequency table elements was wrongly computed when frequency was a power of 2. This led to faulty frequency table encoding with 1 missing byte. During decoding, the frequency was decoded to 0 as 1 byte is missing, leading to division by 0 and thus error. The frequencyTableByteSize is now correctly computed.
nicolas-cadart
force-pushed
the
fix/octreeCompressionEncoding
branch
from
2cabb11 to
0aaa6ac
Compare
Done ! ;) |
taketwo
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Error description
Octree Pointcloud compression encoding was faulty when pointcloud had 2^i - 3 points (problem was spotted with 253 points), leading to program crash when decoding the wrongly encoded compressed data.
Error origin
The crash comes from a division by zero in
pcl::StaticRangeCoder::decodeStreamToIntVector(), when frequency element value is read as 0 instead of 2^i. In fact, this frequency value is badly written inpcl::StaticRangeCoder::encodeIntVectorToStream()because bytes size of each frequency element is wrongly computed.For example, with n = 253 points, last frequency element is equal to 256. This value needs 2 bytes to be encoded. However,
frequencyTableByteSizewas set to 1 byte for this border case. As a result, only the 'null' byte was written to compressed data, missing the most significant one : this frequency element value is written as 0 instead of 256.Proposed correction
The frequencyTableByteSize is now corrected to consider this border case. No change occurs for other pointclouds sizes.