From 657f75b63a9c1d05a6d8dd2307c371ab304f968a Mon Sep 17 00:00:00 2001 From: Lukasz Anforowicz Date: Thu, 3 Oct 2024 19:52:22 +0000 Subject: [PATCH 104/113] Simplify how `Reader` tracks how many frames remain to be decoded. In an earlier commit 278b1d4fc1da9cebdeb511d8b106d239155c35cc we stopped using `Reader.next_frame` for deciding whether to call `read_until_image_data` from `next_frame`. This commit goes one step further and removes the `next_frame` field entirely. There are no known cases where the previous code would result in incorrect behavior, but this commit still seems desirable: * It simplifies the code * It ensures that a single "end-of-IDAT/fdAT-sequence" event kind controls both 1) `consumed_and_flushed` state, and 2) counting remaining frames. (Before this commit `next_frame` would also be mutated after a separate "fcTL-encountered" event. And also after "end-of-IDAT/fdAT-sequence" but only from `next_frame` and not from `next_row`.) --- src/decoder/mod.rs | 80 +++++++++++++++++----------------------------- 1 file changed, 29 insertions(+), 51 deletions(-) diff --git a/third_party/rust/chromium_crates_io/vendor/png-0.17.14/src/decoder/mod.rs b/third_party/rust/chromium_crates_io/vendor/png-0.17.14/src/decoder/mod.rs index 56570b3..d0a88e8 100644 --- a/third_party/rust/chromium_crates_io/vendor/png-0.17.14/src/decoder/mod.rs +++ b/third_party/rust/chromium_crates_io/vendor/png-0.17.14/src/decoder/mod.rs @@ -205,8 +205,7 @@ impl Decoder { decoder: self.read_decoder, bpp: BytesPerPixel::One, subframe: SubframeInfo::not_yet_init(), - fctl_read: 0, - next_frame: SubframeIdx::Initial, + remaining_frames: 0, // Temporary value - fixed below after reading `acTL` and `fcTL`. data_stream: Vec::new(), prev_start: 0, current_start: 0, @@ -234,6 +233,19 @@ impl Decoder { } reader.read_until_image_data()?; + + reader.remaining_frames = match reader.info().animation_control.as_ref() { + None => 1, // No `acTL` => only expecting `IDAT` frame. + Some(animation) => { + let mut num_frames = animation.num_frames as usize; + if reader.info().frame_control.is_none() { + // No `fcTL` before `IDAT` => `IDAT` is not part of the animation, but + // represents an *extra*, default frame for non-APNG-aware decoders. + num_frames += 1; + } + num_frames + } + }; Ok(reader) } @@ -349,11 +361,8 @@ pub struct Reader { decoder: ReadDecoder, bpp: BytesPerPixel, subframe: SubframeInfo, - /// Number of frame control chunks read. - /// By the APNG specification the total number must equal the count specified in the animation - /// control chunk. The IDAT image _may_ have such a chunk applying to it. - fctl_read: u32, - next_frame: SubframeIdx, + /// How many frames remain to be decoded. Decremented after each `IDAT` or `fdAT` sequence. + remaining_frames: usize, /// Vec containing the uncompressed image data currently being processed. data_stream: Vec, /// Index in `data_stream` where the previous row starts. @@ -386,19 +395,6 @@ struct SubframeInfo { consumed_and_flushed: bool, } -/// Denote a frame as given by sequence numbers. -#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] -enum SubframeIdx { - /// The initial frame in an IDAT chunk without fcTL chunk applying to it. - /// Note that this variant precedes `Some` as IDAT frames precede fdAT frames and all fdAT - /// frames must have a fcTL applying to it. - Initial, - /// An IDAT frame with fcTL or an fdAT frame. - Some(u32), - /// The past-the-end index. - End, -} - impl Reader { /// Reads all meta data until the next frame data starts. /// Requires IHDR before the IDAT and fcTL before fdAT. @@ -414,14 +410,6 @@ impl Reader { match state { Some(Decoded::ChunkBegin(_, chunk::IDAT)) | Some(Decoded::ChunkBegin(_, chunk::fdAT)) => break, - Some(Decoded::FrameControl(_)) => { - // The next frame is the one to which this chunk applies. - self.next_frame = SubframeIdx::Some(self.fctl_read); - // TODO: what about overflow here? That would imply there are more fctl chunks - // than can be specified in the animation control but also that we have read - // several gigabytes of data. - self.fctl_read += 1; - } None => { return Err(DecodingError::Format( FormatErrorInner::MissingImageData.into(), @@ -473,7 +461,7 @@ impl Reader { /// Output lines will be written in row-major, packed matrix with width and height of the read /// frame (or subframe), all samples are in big endian byte order where this matters. pub fn next_frame(&mut self, buf: &mut [u8]) -> Result { - if self.next_frame == SubframeIdx::End { + if self.remaining_frames == 0 { return Err(DecodingError::Parameter( ParameterErrorKind::PolledAfterEndOfImage.into(), )); @@ -534,37 +522,27 @@ impl Reader { // Advance over the rest of data for this (sub-)frame. self.finish_decoding()?; - // Advance our state to expect the next frame. - let past_end_subframe = self - .info() - .animation_control() - .map(|ac| ac.num_frames) - .unwrap_or(0); - self.next_frame = match self.next_frame { - SubframeIdx::End => unreachable!("Next frame called when already at image end"), - // Reached the end of non-animated image. - SubframeIdx::Initial if past_end_subframe == 0 => SubframeIdx::End, - // An animated image, expecting first subframe. - SubframeIdx::Initial => SubframeIdx::Some(0), - // This was the last subframe, slightly fuzzy condition in case of programmer error. - SubframeIdx::Some(idx) if past_end_subframe <= idx + 1 => SubframeIdx::End, - // Expecting next subframe. - SubframeIdx::Some(idx) => SubframeIdx::Some(idx + 1), - }; - Ok(output_info) } + fn mark_subframe_as_consumed_and_flushed(&mut self) { + assert!(self.remaining_frames > 0); + self.remaining_frames -= 1; + + self.subframe.consumed_and_flushed = true; + } + /// Advance over the rest of data for this (sub-)frame. /// Called after decoding the last row of a frame. fn finish_decoding(&mut self) -> Result<(), DecodingError> { - // Double-check that all rows of this frame have been decoded. + // Double-check that all rows of this frame have been decoded (i.e. that the potential + // `finish_decoding` call below won't be discarding any data). assert!(self.subframe.current_interlace_info.is_none()); // Discard the remaining data in the current sequence of `IDAT` or `fdAT` chunks. if !self.subframe.consumed_and_flushed { self.decoder.finish_decoding()?; - self.subframe.consumed_and_flushed = true; + self.mark_subframe_as_consumed_and_flushed(); } Ok(()) @@ -617,7 +595,7 @@ impl Reader { /// Read the rest of the image and chunks and finish up, including text chunks or others /// This will discard the rest of the image if the image is not read already with [`Reader::next_frame`], [`Reader::next_row`] or [`Reader::next_interlaced_row`] pub fn finish(&mut self) -> Result<(), DecodingError> { - self.next_frame = SubframeIdx::End; + self.remaining_frames = 0; self.data_stream.clear(); self.current_start = 0; self.prev_start = 0; @@ -729,7 +707,7 @@ impl Reader { match self.decoder.decode_next(&mut self.data_stream)? { Some(Decoded::ImageData) => (), - Some(Decoded::ImageDataFlushed) => self.subframe.consumed_and_flushed = true, + Some(Decoded::ImageDataFlushed) => self.mark_subframe_as_consumed_and_flushed(), _ => (), } } -- 2.47.0.rc0.187.ge670bccf7e-goog