1From 657f75b63a9c1d05a6d8dd2307c371ab304f968a Mon Sep 17 00:00:00 2001 2From: Lukasz Anforowicz <[email protected]> 3Date: Thu, 3 Oct 2024 19:52:22 +0000 4Subject: [PATCH 104/113] Simplify how `Reader` tracks how many frames remain 5 to be decoded. 6 7In an earlier commit 278b1d4fc1da9cebdeb511d8b106d239155c35cc we 8stopped using `Reader.next_frame` for deciding whether to call 9`read_until_image_data` from `next_frame`. This commit goes one 10step further and removes the `next_frame` field entirely. 11 12There are no known cases where the previous code would result 13in incorrect behavior, but this commit still seems desirable: 14 15* It simplifies the code 16* It ensures that a single "end-of-IDAT/fdAT-sequence" event kind 17 controls both 1) `consumed_and_flushed` state, and 2) counting 18 remaining frames. (Before this commit `next_frame` would also be 19 mutated after a separate "fcTL-encountered" event. And also after 20 "end-of-IDAT/fdAT-sequence" but only from `next_frame` and not from 21 `next_row`.) 22--- 23 src/decoder/mod.rs | 80 +++++++++++++++++----------------------------- 24 1 file changed, 29 insertions(+), 51 deletions(-) 25 26diff --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 27index 56570b3..d0a88e8 100644 28--- a/third_party/rust/chromium_crates_io/vendor/png-0.17.14/src/decoder/mod.rs 29+++ b/third_party/rust/chromium_crates_io/vendor/png-0.17.14/src/decoder/mod.rs 30@@ -205,8 +205,7 @@ impl<R: Read> Decoder<R> { 31 decoder: self.read_decoder, 32 bpp: BytesPerPixel::One, 33 subframe: SubframeInfo::not_yet_init(), 34- fctl_read: 0, 35- next_frame: SubframeIdx::Initial, 36+ remaining_frames: 0, // Temporary value - fixed below after reading `acTL` and `fcTL`. 37 data_stream: Vec::new(), 38 prev_start: 0, 39 current_start: 0, 40@@ -234,6 +233,19 @@ impl<R: Read> Decoder<R> { 41 } 42 43 reader.read_until_image_data()?; 44+ 45+ reader.remaining_frames = match reader.info().animation_control.as_ref() { 46+ None => 1, // No `acTL` => only expecting `IDAT` frame. 47+ Some(animation) => { 48+ let mut num_frames = animation.num_frames as usize; 49+ if reader.info().frame_control.is_none() { 50+ // No `fcTL` before `IDAT` => `IDAT` is not part of the animation, but 51+ // represents an *extra*, default frame for non-APNG-aware decoders. 52+ num_frames += 1; 53+ } 54+ num_frames 55+ } 56+ }; 57 Ok(reader) 58 } 59 60@@ -349,11 +361,8 @@ pub struct Reader<R: Read> { 61 decoder: ReadDecoder<R>, 62 bpp: BytesPerPixel, 63 subframe: SubframeInfo, 64- /// Number of frame control chunks read. 65- /// By the APNG specification the total number must equal the count specified in the animation 66- /// control chunk. The IDAT image _may_ have such a chunk applying to it. 67- fctl_read: u32, 68- next_frame: SubframeIdx, 69+ /// How many frames remain to be decoded. Decremented after each `IDAT` or `fdAT` sequence. 70+ remaining_frames: usize, 71 /// Vec containing the uncompressed image data currently being processed. 72 data_stream: Vec<u8>, 73 /// Index in `data_stream` where the previous row starts. 74@@ -386,19 +395,6 @@ struct SubframeInfo { 75 consumed_and_flushed: bool, 76 } 77 78-/// Denote a frame as given by sequence numbers. 79-#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] 80-enum SubframeIdx { 81- /// The initial frame in an IDAT chunk without fcTL chunk applying to it. 82- /// Note that this variant precedes `Some` as IDAT frames precede fdAT frames and all fdAT 83- /// frames must have a fcTL applying to it. 84- Initial, 85- /// An IDAT frame with fcTL or an fdAT frame. 86- Some(u32), 87- /// The past-the-end index. 88- End, 89-} 90- 91 impl<R: Read> Reader<R> { 92 /// Reads all meta data until the next frame data starts. 93 /// Requires IHDR before the IDAT and fcTL before fdAT. 94@@ -414,14 +410,6 @@ impl<R: Read> Reader<R> { 95 match state { 96 Some(Decoded::ChunkBegin(_, chunk::IDAT)) 97 | Some(Decoded::ChunkBegin(_, chunk::fdAT)) => break, 98- Some(Decoded::FrameControl(_)) => { 99- // The next frame is the one to which this chunk applies. 100- self.next_frame = SubframeIdx::Some(self.fctl_read); 101- // TODO: what about overflow here? That would imply there are more fctl chunks 102- // than can be specified in the animation control but also that we have read 103- // several gigabytes of data. 104- self.fctl_read += 1; 105- } 106 None => { 107 return Err(DecodingError::Format( 108 FormatErrorInner::MissingImageData.into(), 109@@ -473,7 +461,7 @@ impl<R: Read> Reader<R> { 110 /// Output lines will be written in row-major, packed matrix with width and height of the read 111 /// frame (or subframe), all samples are in big endian byte order where this matters. 112 pub fn next_frame(&mut self, buf: &mut [u8]) -> Result<OutputInfo, DecodingError> { 113- if self.next_frame == SubframeIdx::End { 114+ if self.remaining_frames == 0 { 115 return Err(DecodingError::Parameter( 116 ParameterErrorKind::PolledAfterEndOfImage.into(), 117 )); 118@@ -534,37 +522,27 @@ impl<R: Read> Reader<R> { 119 // Advance over the rest of data for this (sub-)frame. 120 self.finish_decoding()?; 121 122- // Advance our state to expect the next frame. 123- let past_end_subframe = self 124- .info() 125- .animation_control() 126- .map(|ac| ac.num_frames) 127- .unwrap_or(0); 128- self.next_frame = match self.next_frame { 129- SubframeIdx::End => unreachable!("Next frame called when already at image end"), 130- // Reached the end of non-animated image. 131- SubframeIdx::Initial if past_end_subframe == 0 => SubframeIdx::End, 132- // An animated image, expecting first subframe. 133- SubframeIdx::Initial => SubframeIdx::Some(0), 134- // This was the last subframe, slightly fuzzy condition in case of programmer error. 135- SubframeIdx::Some(idx) if past_end_subframe <= idx + 1 => SubframeIdx::End, 136- // Expecting next subframe. 137- SubframeIdx::Some(idx) => SubframeIdx::Some(idx + 1), 138- }; 139- 140 Ok(output_info) 141 } 142 143+ fn mark_subframe_as_consumed_and_flushed(&mut self) { 144+ assert!(self.remaining_frames > 0); 145+ self.remaining_frames -= 1; 146+ 147+ self.subframe.consumed_and_flushed = true; 148+ } 149+ 150 /// Advance over the rest of data for this (sub-)frame. 151 /// Called after decoding the last row of a frame. 152 fn finish_decoding(&mut self) -> Result<(), DecodingError> { 153- // Double-check that all rows of this frame have been decoded. 154+ // Double-check that all rows of this frame have been decoded (i.e. that the potential 155+ // `finish_decoding` call below won't be discarding any data). 156 assert!(self.subframe.current_interlace_info.is_none()); 157 158 // Discard the remaining data in the current sequence of `IDAT` or `fdAT` chunks. 159 if !self.subframe.consumed_and_flushed { 160 self.decoder.finish_decoding()?; 161- self.subframe.consumed_and_flushed = true; 162+ self.mark_subframe_as_consumed_and_flushed(); 163 } 164 165 Ok(()) 166@@ -617,7 +595,7 @@ impl<R: Read> Reader<R> { 167 /// Read the rest of the image and chunks and finish up, including text chunks or others 168 /// 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`] 169 pub fn finish(&mut self) -> Result<(), DecodingError> { 170- self.next_frame = SubframeIdx::End; 171+ self.remaining_frames = 0; 172 self.data_stream.clear(); 173 self.current_start = 0; 174 self.prev_start = 0; 175@@ -729,7 +707,7 @@ impl<R: Read> Reader<R> { 176 177 match self.decoder.decode_next(&mut self.data_stream)? { 178 Some(Decoded::ImageData) => (), 179- Some(Decoded::ImageDataFlushed) => self.subframe.consumed_and_flushed = true, 180+ Some(Decoded::ImageDataFlushed) => self.mark_subframe_as_consumed_and_flushed(), 181 _ => (), 182 } 183 } 184-- 1852.47.0.rc0.187.ge670bccf7e-goog 186 187