Yes, since the jxl_decoder_process_data consumes the buffer by incrementing the pointer and decrementing the length, it will become 0 when this buffer is used up.

jxl_decoder_process_data will repeatedly populate the buffer until it's done. This call is only called once - after this we other fail out or return TerminateSuccess, right?

I'll add a comment.

When the buffer is empty we will ask for more data to continue decoding?

Yes, but first we need to land some changes in jxl-rs, so that I can test without having it referring to my local checkout :P

So this means that after jxl_decoder_process_data returns Ok and there is a valid basic info struct it can only return Error or NeedsMoreData until it has the complete image? Is that an api constraint we want? Seems a little limiting.

This mult could overflow?

What else would we need to do?

It's easy to update the code if we need to (like I do in the next two patches, for animation and CMS support).

Okay, just something I noticed that felt a little unexpected.

This is not called anywhere, and why do we need a separate init function instead of doing it in the constructor? This doesn't seem to ever fail, or is the intention to make it fallible? Still jxl_decoder_new is infallible...

InitInternal is the virtual version of the non-virtual Decoder::Init, which it gets called from.

Ah. I missed override. Sorry and thanks.

I think I'd prefer to error in case of overflow, to prevent mismatches of size expectations potentially causing bugs/security issues.

"repeatedly populate buffer" meaning the buffer will be written with intermediate decoding result multiple times?

I'm actually not certain if jxl-rs will wait until it can populate it all, or will populate a bit at a time. I _think_ it's the former.

The important point is that when we have received the frame header we don't know how much data jxl-rs needs to produce a frame - we just know it will happen at some point, and how big the buffer needs to be - so from then on we need to have the buffer available.

And storing a C++ buffer on the Rust side seemed like an unnecessary amount of hassle, so sending it each call across the FFI seemed simpler.

Sending it sounds good to me, I just have concern that writing it repeatedly may negatively affect the performance.

Say,

1. The browser gets 1 MB of binary data immediately
2. Somehow the first rendering is available at 200 kB (random chosen number by me)
3. The decoder writes that to the buffer
4. The decoder does the same on 400 kB, 800 kB and then on 1 MB
5. In the end there was three full buffer write even though it could happen just once, as the user would never even notice the intermediate results

(but yeah dealing with that doesn't have to happen in this patch IMO)

I have now talked with Luca about it - and current jxl-rs will render a group once neighboring groups are available. Future render pipelines will render the internal of a group once the group is available, and the rest once neighboring groups are.

It's not a full buffer write - jxl-rs only renders to the sections of memory where we get new pixels.

It was used in a later patch - moved it to that patch now.

It seems like we wouldn't expect to get here with has_output_buffer == true? Then we would return Ok(false) below which seems wrong. I think it works because we never get here with has_output_buffer == true because the cpp code will pass an output buffer after we hit this and the subsequent process call above will fill the pixel buffer and then we'll toggle frame_ready and return Ok(true)? Is my understaning correct?

This isn't about guarding against metadata decodes having buffers, this is about the interaction between decoder.rs and jxl-rs - we must not have a buffer when we get frame metadata, because we are allocating based on the frame metadata, so if we already have a buffer something is wrong.