PNG: extend roundtrip fuzzing to low bit depths

[Shnatsel]

Right now roundtrip fuzzing in PNG is disabled for bit depths lower than 8:

zune-image/zune-png/fuzz/fuzz_targets/roundtrip.rs

Line 17 in 008f02a

if bit_depth < 8 { return None } // TODO: remove this hack

As noted here, zune-png expands the low bit depths up to 8 bits, but the fuzz target doesn't account for that.

[Shnatsel]

I've taken a stab at this, but the expansion function is tied to the rest of the decoding in not entirely trivial ways, so it's not just a matter of adding a wrapper that's only exposed when fuzzing as I hoped:

#[cfg(fuzzing)]
pub fn some_function() {
    some_function()
}

[etemesi254]

It's tied in only one way that makes it annoying to handle small bit depths.

It's decode_interlaced part. Those passes expect it to be 8 bit.

And another annoying thing is that the spec contradicts itself

Images with lower bit depths < 8 and PLTE chunks exists for which the spec says we use entries from 0-(2^bitdepth)-1
Which makes sense, but then each PLTE chunk is 8 bits/1 byte.

Png with small bit depths look like, assuming a bit depth of 4, look like z= [AAAA | BBBB] with AAAA being the first pixel and BBBB being the second.

So in order to read pLTE chunks correctly, we have to shift AAAA, load it's plTE chunk, get the value, and pack it back into z.

That is a lot of work, and I'd prefer that we have the slogan correct and fast, with priority as specified.

I'd prefer I take this one on since you mentioned it is tied to the rest of the steps, it may not be easy and I may punt up and write a expansion to reduction pass instead, will close this when done.

[Shnatsel]

FWIW so far I'm looking at fuzzing the images with low bit depths but without a palette; e.g. 1-bit black-and-white ones. I haven't considered paletted images yet.

Although another contributor has opened a PR for the png crate adding paletted roundtrip too, perhaps the palette generation could be reused for fuzzing zune-png.

[etemesi254]

I've been slowly working on this and it dawned to me that there is no fast way to do this and 8 bit/16 bit jpegs

I'll add a pass to convert 8 bit images to low bit depths after I finish the alloc friendly png decoder

[Lokathor]

Just reading this thread a bit, you may be misunderstanding how the PLTE chunk works. When bit depth is lower there's fewer entries but each entry is stil r8g8b8. Lower bit depths affect how you get each index, but once you have the index it's just image.palette.get(index).unwrap_or_default()

[etemesi254]

st reading this thread a bit, you may be misunderstanding how the PLTE chunk works. When bit depth is lower there's fewer entries but each entry is stil r8g8b8.

That was the initial confusion I had , but I did come to learn about it as you did

But still, the problem again is that if you get the information from the ihdr chunk it reports < 8 bits, but then the PLTE chunk uses 8 bits.

So then meaning the headers would report < 8bpp but the image would be 8 bpp for you to get any meaningful image.

Lower bit depths affect how you get each index, but once you have the index it's just image.palette.get(index).unwrap_or_default()

No need for unwraping btw, bad for speed, just set it to black also allowed,

the crate does it like

zune-image/zune-png/src/decoder.rs

Lines 1126 to 1163 in a30c3e1

	fn expand_palette(&mut self, components: usize)
	{
	if components == 0
	{
	return;
	}
	// this is safe because we resized palette to be 256
	// in self.parse_plte()
	let palette: &[PLTEEntry; 256] = &self.palette[0..256].try_into().unwrap();
	if components == 3
	{
	for px in self.out.chunks_exact_mut(3)
	{
	// the & 255 may be removed as the compiler can see u8 can never be
	// above 255, but for safety
	let entry = palette[usize::from(px[0]) & 255];
	px[0] = entry.red;
	px[1] = entry.green;
	px[2] = entry.blue;
	}
	}
	else if components == 4
	{
	for px in self.out.chunks_exact_mut(4)
	{
	let entry = palette[usize::from(px[0]) & 255];
	px[0] = entry.red;
	px[1] = entry.green;
	px[2] = entry.blue;
	px[3] = entry.alpha;
	}
	}
	}
	}

[Lokathor]

But still, the problem again is that if you get the information from the ihdr chunk it reports < 8 bits, but then the PLTE chunk uses 8 bits.

So then meaning the headers would report < 8bpp but the image would be 8 bpp for you to get any meaningful image.

That's just a natural part of all image format decoding. It's the same thing with BMP files. More specifically it's how all paletted image systems work. "bits per pixel" with paletted color means "bits per palette index" (because each pixel is one index), and the actual palette entries are essentially always going to be some format that's bigger than the index itself (because that's the point).

EDIT: the thing with the palette is neat, I'll try using that soon.

[etemesi254]

That's just a natural part of all image format decoding. It's the same thing with BMP files. More specifically it's how all paletted image systems work. "bits per pixel" with paletted color means "bits per palette index" (because each pixel is one index), and the actual palette entries are essentially always going to be some format that's bigger than the index itself (because that's the point).

Cool, didn't know bmp works like that too.

But again I don't see a use-case for supporting < 8bpp images since they make other parts slower as they have to take into consideration that hence why I explicitly expand to 8bpp.

EDIT: the thing with the palette is neat, I'll try using that soon.

Another cool feature of it is that the palette is 4 entries, which is 4*u8 = u32, hence for four component output, theoretically the compiler should optimize it to a single load and store but it fails on it

https://godbolt.org/z/WbEeMhPEq

[Lokathor]

I'd agree that, in general, the in-memory version of an image doesn't need to be less than 8 bits per channel. You really only need to support less than 8 bits per channel for storage on disk (either encoding or decoding).

(the exception here would be for systems that actually use data that's less than 8 bits per channel directly, but those are relatively rare these days)