FourMcOutputStream.compress doesn't handle incompressible data correctly about 4mc HOT 9 CLOSED

Thanks for spotting this. Your workaround is smart but what about fixing it by a compressor reset() performed in that specific case when block is better uncompressed?

        if (compressor.getBytesRead() <= compressor.getBytesWritten()) {
            // write uncompressed data block
            byte[] uncompressed = ((Lz4Compressor) compressor).uncompressedBytes();
            rawWriteInt(uncompressed.length);
            int checksum = Lz4Compressor.xxhash32(uncompressed, 0, uncompressed.length, 0);
            rawWriteInt(checksum);
            out.write(uncompressed, 0, uncompressed.length);

            compressor.reset(); // reset compressor buffers
          }

Last line should work just fine. If you have such corner case reproducible, would you mind trying that?

from 4mc.

Hi, when calling compressor.reset(), you also reset finish to false, which will also lead compressor.finished() to false. A call to compressor.finish() should also be made after compressor.reset();

My current fix: use maxInputSize instead of FOURMC_BLOCK_SIZE

// maxInputSize for feeding to compressor.
// Called MAX_INPUT_SIZE in other CompressorStream, change to maxInputSize for style check pass.
    final int maxInputSize;

// Use max input size instead of FourMcCodec.FOURMC_BLOCK_SIZE.
int overhead = Lz4Compressor.compressBound(bufferSize) - bufferSize;
maxInputSize = bufferSize - overhead;

       if (len + limlen > maxInputSize && limlen > 0) {
            finish();
            compressor.reset();
        }

        if (len > maxInputSize) {
            do {
                int bufLen = Math.min(len, maxInputSize);
                // It's essential that we make sure input length(bufLen) <= FOUR_BLOCK_SIZE. Otherwise, we will got
                // a wrong decision whether we use uncompressed data directly or not, which will also trigger a
                // BufferOverflow exception if we read too much uncompressed data.
                compressor.setInput(b, off, bufLen);
                finish();
                compressor.reset();
                off += bufLen;
                len -= bufLen;
            } while (len > 0);
            return;
        }

I will try your fix and report back.

from 4mc.

And yes, this fixes the problem. You can use testCodec to reproduce this case

  if (compressor.getBytesRead() <= compressor.getBytesWritten()) {
            // write uncompressed data block
            byte[] uncompressed = ((Lz4Compressor) compressor).uncompressedBytes();
            rawWriteInt(uncompressed.length);
            int checksum = Lz4Compressor.xxhash32(uncompressed, 0, uncompressed.length, 0);
            rawWriteInt(checksum);
            out.write(uncompressed, 0, uncompressed.length);

            compressor.reset(); // reset compressor buffers
            compressor.finish(); // set compressor to be finished.
          }

from 4mc.

Perfect, I'll commit our fix as well for upcoming release. Thanks again.

from 4mc.

Ah, I forgot to mention there is another case in the code.
createCompressor in FourMcCodec.java should use a fixed directBufferSize(4MB) rather than a user configurable size. (I already used a fixed size when testing)

If the configured block size is 1MB, there will be 4 calls of FourMcOutputStream.compress() for 4MB input.
For incompressible data, the (compressor.getBytesRead() <= compressor.getBytesWritten) will always be false in the first 3 calls, writes compressed data larger than original data(unexpected). The last call will try to read all the input(4MB), which also raises a BufferUnderFlowException (The uncompressedBuf has only 1MB capacity, and bytesRead is 4MB).

And if the configured block size is near 4MB and incompressible and lz4_compressBound(configured_block_size) >= 4MB, then the fix we proposed above will lose (4MB-configured_block_size) incompressible data.

from 4mc.

@carlomedas ping

from 4mc.

OK so the fix to this would just be to remove the read from configuration and hardcode it to the max size of 4MB?
Sounds reasonable because anyways that size is best for big-data.
TBH I'm not able to understand 100% the issue you reported because it's long time I didn't look at that code, so doing a proper fix for this use case would require much longer time and introduce non trivial risk...
That's why your idea of fix sounds very good. Let me know and I'll commit that as well for upcoming release of 4mc with embedded libraries).
TIA.

from 4mc.

OK so the fix to this would just be to remove the read from configuration and hardcode it to the max size of 4MB?
Yes, that's the fix, createCompressor in FourMcCodec uses a hardcoded maximum 4MB size.
It's indeed a corner case.

from 4mc.

Done in 4mc 1.4.0

from 4mc.