Skip to content
This repository was archived by the owner on Apr 22, 2020. It is now read-only.

Conversation

@rdblue
Copy link
Contributor

@rdblue rdblue commented May 17, 2016

The JNI wrapper in C++ currently assumes that Brotli consumes all of the available bytes in the input buffer, but this isn't always the case. Brotli will sometimes leave the in_available value less than the number of bytes available, which causes jbrotli to discard some unprocessed input bytes and fail decompression.

This patch adds BrotliStreamDeCompressorResult to return the error code, bytes consumed, and bytes produced by the stream decompressor and correctly updates the ByteBuffer handling in Java to correctly track what of the input buffer was consumed (so it can be passed into Brotli again to consume the rest). Using a Java class also avoids packing and unpacking the return value.

@nitram509
Copy link
Contributor

Ryan, thank you very much for pointing this out and making this PR.
It looks great and I will merge it.

Regarding some of your changes, I just have some minor comments.
Which means no show stopper, but e.g. I don't want to have a 'todo' in the code base ;-)

@@ -0,0 +1,13 @@
package org.meteogroup.jbrotli;

public class BrotliStreamDeCompressorResult {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good idea to replace the bit-twiddling error handling with such an Object.

@nitram509 nitram509 merged commit 4bf9039 into MeteoGroup:master May 18, 2016
@rdblue
Copy link
Contributor Author

rdblue commented May 18, 2016

Thanks for merging this! I've also opened #4 because we can't currently use the published jar on either dev or prod systems because of the version of C++ used. Using 4.8 will make this more portable and would be good to switch to for the next release with this PR included. Thanks @nitram509!

@rdblue rdblue mentioned this pull request Jun 13, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants