Enabled bounds check for gif frames

[fseegraeber]

Fixes #1238

Reading a gif that contained frames exceeding its "Logical Screen" dimensions would panic.
This PR simply enables the check that is provided by the underlying decoder.

Feel free to reject or comment if you want to handle out of bounds frames differently.

[fseegraeber]

On a note: I just discovered that, when running all test targets, the check_references & render_images tests fail with the tests\images\gif\anim\oob.gif. Not suprisingly I guess.

Image viewers I tested display that image regardless (interestingly Windows, IrfanView and VSCode all seem to display it differently). If this should be supported it must be defined how i guess.

[HeroicKatora]

The common interpretation is to discard data outside the bounds and only blend the pixels that are inbounds. I'm quite worried why the CI does not show failure despite images failing to render?

[fseegraeber]

To me it looks like if --no-default-features overrides the --features "gif".

[HeroicKatora]

@fseegraeber You're right. The reference tests need both gif and png enabled as any unsupported image formats are skipped and png is required to load the references. It seems that no configuration has all features enabled.

[fseegraeber]

The common interpretation is to discard data outside the bounds and only blend the pixels that are inbounds.

Enabling the bounds check is not the way to go then.

What you describe seems to be the current behavior. Problematic are only frames that exceed the logical screen in size. These currently lead to a panic.

Should we

• allocate the frame in an extra buffer and only copy the inbound pixels, or
• return with an error

Ideally the underlying gif decoder would directly support that scenario (transfering a frame rectangle into a target rectangle) making additional allocations unneccesary, but I'm not sure if that is desired.

[HeroicKatora]

Should we [..] allocate the frame in an extra buffer and only copy the inbound pixels

I believe the code already has the intention of doing that:

image/src/codecs/gif.rs

Lines 264 to 277 in b77d4f3

	ImageBuffer::from_fn(self.width, self.height, |x, y| {
	let frame_x = x.wrapping_sub(left);
	let frame_y = y.wrapping_sub(top);
	let previous_pixel = self.non_disposed_frame.get_pixel_mut(x, y);
	if frame_x < frame_buffer.width() && frame_y < frame_buffer.height() {
	let mut pixel = *frame_buffer.get_pixel(frame_x, frame_y);
	blend_and_dispose_pixel(dispose, previous_pixel, &mut pixel);
	pixel
	} else {
	// out of bounds, return pixel from previous frame
	*previous_pixel
	}
	})

If there are any remaining panics (now I'm no longer sure) then I think we should fix those while keeping that same intention.

[fseegraeber]

You'r right the Iterator implementation does it that way. Allocating frame + logical screen if they dont match.
The problem is in read_image:

image/src/codecs/gif.rs

Line 110 in b77d4f3

self.reader.read_into_buffer(buf).map_err(ImageError::from_decoding)?;

self.reader.read_into_buffer(buf) expects buf to be atleast of frame size, but it is of size self.total_bytes() which uses the logical screen size.

It probably makes sense to consolidate those two methods.

[HeroicKatora]

Well spotted. Yes, that seems like the right way to handle it.

[fseegraeber]

GifDecoder::read_image does now handle frames bigger than the logical screen size by allocating it into an extra buffer and inserting the frame into the logical screen image at the correct position. This is basically what next() was doing.

I refactored read_image to better align with GifFrameIterator::next and got rid of one copy_from_slice.

The functionality is pretty similar, yet there are some differences, that made it hard to really consolidate them. read_image does not need to blend between different frames, while next needs to. I experimented with a

fn read_next_frame(blend: Option<F>, ...) where F: Fn(...) {...

but couldn't get it to compile in the end😄 . I doubt it would really have improved readability.

I refactored some of the shared functionality into a helper struct, not sure how much that imroves it though.

[HeroicKatora]

That all looks good to me, including helper struct. Doing the infallible conversions to our datatypes in one place instead of interrupting code flow and assigning to previous declared stack slots improves readability.