Fix input_offset resetting in stream

[myl7]

Close #22, close #23

The incorrect input_offset resetting causes that, while the stream outputs are still decompress-able, when the input is long enough, the output would become incorrect (means decompression can not output the original input). The PR fixes it.

[pimterry]

Amazing @myl7! Thanks for investigating this.

Do you know how long the input has to be to trigger this issue? Is it possible to write a test to reproduce it, just to check this fix and to ensure we don't run into the same issue again in future?

[myl7]

A ~570KB file is enough. How about to add a test with a 600KB file?

[pimterry]

Yes, that sounds good to me. I'm happy with either committing a 600KB file or just generating it at runtime (we can use getRandomValues in a loop for 65K at a time) assuming that's fairly quick.

We need to remember to confirm that any new test for this does definitely fail in the previous implementation without this fix 😄

[pimterry]

Oops, sorry, that was closed automatically due to merging #26.

[myl7]

My fault😂. I mistakenly used the term like "the fix # 25" in that.

I checked the processing of input_offset.
The problem looks more like a design choice rather than logical error.

When the instance requires more input and returns NeedsMoreInput, input_offset is previously set to input.len(), which is fair since for this chunk all bytes have been consumed.
However, since in the TransformStream we use do {} while {} and go to the next chunk immediately after getting NeedMoreInput, input_offset should be 0 for the next chunk but incorrectly kept input.len() which is for the previous chunk.
It causes the problem.

In the tests since input_offset is manually managed so the problem does occur.
The above mistakes seem to cause the instance keeps some chunks so the output is still decompressable but can not be recovered to the original input.

Adding an extra step in the TransformStream helper to manually reset input_offset seems also OK.
But I personally slightly prefer resetting input_offset in the library like the fix because it is easier for users to use.
How about your preference?

[pimterry]

I see, thanks for the explanation, I think I understand the issue much more clearly.

What's really going on here is that within brotli-wasm we're tracking state that tightly depends on how it's used in the stream wrapper - it's not really part of our Brotli streaming implementation at all. It's breaking here not because it's wrong, but because it doesn't match details of how that other implementation works. The fixed logic in this PR to update input_offset makes sense in the current suggested implementation, but might still be wrong in other implementations - for example reading from a Buffer that is being appended to elsewhere (i.e. where the input offset doesn't change between chunks), something like this:

const buffer = /* ... some fixed buffer that's being written to elsewhere */ 
let inputOffset = 0;

while (true) {
    const nextInput = buffer.slice(inputOffset, endOfBuffer);
    output.append(this.stream.compress(nextInput, outputSize));

    if (this.stream.result() === brotliWasm.BrotliStreamResult.NeedsMoreInput) {
        // Wait for data from elsewhere to update endOfBuffer
        await waitForMoreData();

        // Last_input_offset will often be wrong here - it really should be endOfBuffer
        // now, but in fact it will be 0, because it's assuming that buffer is reset.
    }
}

(Very rough code, but you get the idea)

Even when it's correct, it's a bit 'too clever' and I think moving that into the stream wrapper (but making it easy to implement there) would be simpler & clearer.

What would happen if we remove last_input_offset completely, and instead make compress and decompress return the details of what was consumed each time. Then it's up to the stream wrapper to handle that and track input offset itself, depending on how the input actually works. In the web stream case, the stream implementation would be something like this:

let inputOffset = 0;
do {
    const input = chunk.slice(inputOffset);
    const { output, consumedInputLength } = this.stream.compress(input, this.outputSize);
    inputOffset += consumedInputLength;
    controller.enqueue(output);
} while (this.stream.result() === brotliWasm.BrotliStreamResult.NeedsMoreOutput)

What do you think?

[myl7]

True. Returning input_offset other than keeping it in the stream looks better.

I have updated the stream Rust code with the tests.
If OK, I will then also update the stream decompression example PR #27 .

[pimterry]

Looks great, thanks @myl7! I think this is definitely better and should make stream usage generally a little clearer, and fix the original bug en route too.

I won't merge #27 for now, if you could add the matching changes in there that would be great, thank you! I'll do a release once that's all done.