HDR Decoder: float by default

[johannesvollmer]

I suggest we replace the default color type of the hdr format. It was rgb8, but instead we should consider making it f32, to avoid loosing information.

I also thought about whether it was possible to implement Pixel for Rgbe8, which looks like the optimal solution, since it saves memory but retains the float precision and infinite range. However, there is no color type associated with that, and we cannot directly return a pixel buffer, so that seems impossible.

Also adds some tests for round-trip checks and (soon) pixel checks ns across image formats.

• decode from file to f32 buffers
• fix tests

[johannesvollmer]

running the tests currently does not trigger this panic. some ideas why this is not triggered?

[johannesvollmer]

the benchmarks trigger this panic

[fintelia]

Anyone remember why this stalled? The changes look reasonable, modulo figuring out whether they include any API-breaking changes.

[johannesvollmer]

I believe the following happened:

1. I changed the code until I was pretty confident it might work

2. Add some tests for further confidence

3. Notice that an unconditional panic never is detected by the tests

   In detail: To make the tests fail, I added a simple unconditional panic, here:

   fn read_image_data_f32(&mut self, image_bytes: &mut [u8]) -> ImageResult<()> {
        panic!("dsfowsidjw"); // was never triggered when running any of the tests (existing and new tests)

Maybe you can check it out and try to investigate @fintelia? Maybe it already works but only the tests need some tweaking

[johannesvollmer]

Note: This PR only handles decoding, not encoding yet.

The encoder itself is already full f32, and only needs to be called correctly somewhere else in the library.

image/src/codecs/hdr/encoder.rs

Lines 12 to 20 in 183e74e

	impl<W: Write> HdrEncoder<W> {
	/// Creates encoder
	pub fn new(w: W) -> HdrEncoder<W> {
	HdrEncoder { w }
	}
	/// Encodes the image ```data```
	/// that has dimensions ```width``` and ```height```
	pub fn encode(mut self, data: &[Rgb<f32>], width: usize, height: usize) -> ImageResult<()> {

It seems like it is never called at all yet? Probably because it does not implement the standard ImageEncoder trait yet. This probably needs to be added. Fortunately, this means the encoder will be a non-breaking change :)

It was planned to be called here

image/src/io/free_functions.rs

Line 182 in 9ed41d7

// image::ImageFormat::Hdr => hdr::HdrEncoder::new(fout).encode(&[Rgb<f32>], width, height), // usize

But we should rather put it here I guess.

image/src/io/free_functions.rs

Lines 244 to 246 in 9ed41d7

	}
	image::ImageOutputFormat::Unsupported(msg) => Err(ImageError::Unsupported(

[johannesvollmer]

Anyways, do we consider changing the returned ColorType to be a breaking change?

Some users might expect the returned type to always be Rgb8, even if the library never made such a guarantee. This would mean that with the new version, their code no longer works, failing at runtime.

[HeroicKatora]

The short answer: not really, but we don't 100% know.

Downstream could rely on matching DynamicImage to get results and panic on mismatching code paths. Code would break even though to the type system it isn't a SemVer major change. Note also, DynamicImage::from_decoder(…).into_rgba8(self) would continue to work fine.

Some changes to color types had been made previously but most from rgba to rgb and not to the sample type. Potentially, there could be some code that requires opt-in to th changes in image::io::Reader (by specifying some 'support-level') but making a promise to keep color types stable is a significant maintenance burden.

I'd say it's okay, although we may seek to clarify this through the Reader or decoder at a future point. With jpeg (CMYK) and avif (Yuv) there are certainly potentially going to be even more drastic color type changes happening in the future.

[johannesvollmer]

revisiting this:

• reduced pr scope to exclude the hdr encoder, do that in another pr, this pr is only the decoder
• removed some roundtrip testing because it seemed of little value

now, the reference tests fail. first, this was because the png format doesn't support float. added the workaround that all hdr images are converted from rgb32f to rgb8 before comparing with png. now, it fails because there is a bug in the pr somewhere, the tests do their job.

current state: it seems to be on the right way, but some channels are apparently not handled correctly. you can see this when comparing the rendered png reference image with the hdr image, it doesn't match.

[johannesvollmer]

left: the original hdr image, as displayed in affinity photo
right: result of decoding the hdr image using the new decoder, converting that to rgb8, and writing it out as png

maybe the problem is that we must do gamma correction for the output to match? the old decoder did gamma correction (when reading the float values from the image and converting them to u8, because it calls Rgbe8Pixel::to_ldr_scale_gamma()). the new decoder returns the float values without gamma correction, because it calls Rgbe8Pixel::to_hdr().

edit: success

all tests pass when the new decoder uses the gamma technique of the previous implementation pow(x, 2.2). But this doesn't seem desirable. we should probably always return linear floats from the hdr decoder, and instead update the reference image for the tests.

opinions? how would i do that?

[johannesvollmer]

added one commit that implements the gamma correction. but, as i said, i think there is a better way: output linear values and update the tested reference images. i'll revert that commit if you agree

[fintelia]

Could you take a look at #1937, I think it is doing something very similar?

[johannesvollmer]

Yes indeed. That pr achieves the same with less code changes, but

• their tests shouldn't work? I can run the tests for that pr to check it
• this pr does more: it removes some of the unneeded buffer copying

oh, the debug statements should be removed from this pr

[johannesvollmer]

the tests need to be adjusted either pr, because changing non-linear u8 to linear f32 should result in failing tests.

which is another breaking change. we should probably provide functionality to obtain the old behavior without much of a hassle

[johannesvollmer]

note: i checked out that PR and indeed the tests fail, as they should.

[fintelia]

Superseded by #2150