Merge pull request #2737 from telecos/refactor/bmp-rowread-byte-slice

Refactor BMP row decoding to work on byte slices

Author: 197g

src/codecs/bmp/decoder.rs

@@ -5,13 +5,10 @@ use std::iter::{repeat, Rev};
 use std::slice::ChunksExactMut;
 use std::{error, fmt};
 
-use byteorder_lite::{LittleEndian, ReadBytesExt};
-
 use crate::color::ColorType;
 use crate::error::{
     DecodingError, ImageError, ImageResult, UnsupportedError, UnsupportedErrorKind,
 };
-use crate::io::ReadExt;
 use crate::{ImageDecoder, ImageFormat};
 
 const BITMAPCOREHEADER_SIZE: u32 = 12;
@@ -428,6 +425,21 @@ fn calculate_row_padding(bytes_per_row: usize) -> usize {
     (4 - (bytes_per_row % 4)) % 4
 }
 
+/// Allocate a row buffer with OOM protection.
+fn allocate_row_buffer(size: usize) -> ImageResult<Vec<u8>> {
+    let mut buffer = vec_try_with_capacity(size).map_err(|_| {
+        ImageError::Unsupported(UnsupportedError::from_format_and_kind(
+            ImageFormat::Bmp.into(),
+            UnsupportedErrorKind::GenericFeature(format!(
+                "Row buffer allocation ({} bytes) too large",
+                size
+            )),
+        ))
+    })?;
+    buffer.resize(size, 0);
+    Ok(buffer)
+}
+
 /// Convenience function to check if the combination of width, length and number of
 /// channels would result in a buffer that would overflow.
 fn check_for_overflow(width: i32, length: i32, channels: usize) -> ImageResult<()> {
@@ -663,6 +675,54 @@ impl Bitfields {
     }
 }
 
+/// Helper to read RLE data using the already-buffered reader.
+/// Avoids double-buffering since BmpDecoder already requires BufRead.
+struct RleReader<'a, R> {
+    reader: &'a mut R,
+}
+
+impl<'a, R: BufRead> RleReader<'a, R> {
+    fn new(reader: &'a mut R) -> Self {
+        Self { reader }
+    }
+
+    fn read_byte(&mut self) -> io::Result<u8> {
+        let buf = self.reader.fill_buf()?;
+        if buf.is_empty() {
+            return Err(io::Error::new(
+                io::ErrorKind::UnexpectedEof,
+                "unexpected end of RLE data",
+            ));
+        }
+        let byte = buf[0];
+        self.reader.consume(1);
+        Ok(byte)
+    }
+
+    fn read_exact(&mut self, buf: &mut [u8]) -> io::Result<()> {
+        let mut remaining = buf.len();
+        let mut offset = 0;
+
+        while remaining > 0 {
+            let available = self.reader.fill_buf()?;
+            if available.is_empty() {
+                return Err(io::Error::new(
+                    io::ErrorKind::UnexpectedEof,
+                    "unexpected end of RLE data",
+                ));
+            }
+
+            let to_read = remaining.min(available.len());
+            buf[offset..offset + to_read].copy_from_slice(&available[..to_read]);
+            self.reader.consume(to_read);
+            offset += to_read;
+            remaining -= to_read;
+        }
+
+        Ok(())
+    }
+}
+
 /// A bmp decoder
 pub struct BmpDecoder<R> {
     reader: R,
@@ -686,11 +746,11 @@ pub struct BmpDecoder<R> {
     icc_profile: Option<Vec<u8>>,
 }
 
-enum RLEInsn {
+enum RLEInsn<'a> {
     EndOfFile,
     EndOfRow,
     Delta(u8, u8),
-    Absolute(u8, Vec<u8>),
+    Absolute(u8, &'a [u8]),
     PixelRun(u8, u8),
 }
 
@@ -1206,8 +1266,6 @@ impl<R: BufRead + Seek> BmpDecoder<R> {
         bitfields: Option<&Bitfields>,
     ) -> ImageResult<()> {
         let num_channels = self.num_channels();
-        let row_padding_len = self.width as usize % 2 * 2;
-        let row_padding = &mut [0; 2][..row_padding_len];
         let bitfields = match bitfields {
             Some(b) => b,
             None => self.bitfields.as_ref().unwrap(),
@@ -1216,15 +1274,27 @@ impl<R: BufRead + Seek> BmpDecoder<R> {
 
         reader.seek(SeekFrom::Start(self.data_offset))?;
 
+        // Calculate row size in bytes (2 bytes per pixel + padding)
+        let row_data_len = self.width as usize * 2;
+        let row_padding_len = calculate_row_padding(row_data_len);
+        let total_row_len = row_data_len + row_padding_len;
+
+        let mut row_buffer = allocate_row_buffer(total_row_len)?;
+
         with_rows(
             buf,
             self.width,
             self.height,
             num_channels,
             self.top_down,
             |row| {
-                for pixel in row.chunks_mut(num_channels) {
-                    let data = u32::from(reader.read_u16::<LittleEndian>()?);
+                reader.read_exact(&mut row_buffer)?;
+
+                for (row_data, pixel) in row_buffer
+                    .chunks_exact(2)
+                    .zip(row.chunks_exact_mut(num_channels))
+                {
+                    let data = u32::from(u16::from_le_bytes(row_data.try_into().unwrap()));
 
                     pixel[0] = bitfields.r.read(data);
                     pixel[1] = bitfields.g.read(data);
@@ -1237,7 +1307,7 @@ impl<R: BufRead + Seek> BmpDecoder<R> {
                         }
                     }
                 }
-                reader.read_exact(row_padding)
+                Ok(())
             },
         )?;
 
@@ -1253,15 +1323,25 @@ impl<R: BufRead + Seek> BmpDecoder<R> {
         let reader = &mut self.reader;
         reader.seek(SeekFrom::Start(self.data_offset))?;
 
+        // Calculate row size in bytes (4 bytes per pixel, no padding for 32-bit)
+        let row_data_len = self.width as usize * 4;
+
+        let mut row_buffer = allocate_row_buffer(row_data_len)?;
+
         with_rows(
             buf,
             self.width,
             self.height,
             num_channels,
             self.top_down,
             |row| {
-                for pixel in row.chunks_mut(num_channels) {
-                    let data = reader.read_u32::<LittleEndian>()?;
+                reader.read_exact(&mut row_buffer)?;
+
+                for (row_data, pixel) in row_buffer
+                    .chunks_exact(4)
+                    .zip(row.chunks_exact_mut(num_channels))
+                {
+                    let data = u32::from_le_bytes(row_data.try_into().unwrap());
 
                     pixel[0] = bitfields.r.read(data);
                     pixel[1] = bitfields.g.read(data);
@@ -1288,46 +1368,55 @@ impl<R: BufRead + Seek> BmpDecoder<R> {
         format: &FormatFullBytes,
     ) -> ImageResult<()> {
         let num_channels = self.num_channels();
+        let row_data_len = match *format {
+            FormatFullBytes::RGB24 => self.width as usize * 3,
+            FormatFullBytes::Format888 => self.width as usize * 4,
+            FormatFullBytes::RGB32 | FormatFullBytes::RGBA32 => self.width as usize * 4,
+        };
         let row_padding_len = match *format {
-            FormatFullBytes::RGB24 => calculate_row_padding(self.width as usize * 3),
+            FormatFullBytes::RGB24 => calculate_row_padding(row_data_len),
             _ => 0,
         };
-        let row_padding = &mut [0; 4][..row_padding_len];
+        let total_row_len = row_data_len + row_padding_len;
 
         self.reader.seek(SeekFrom::Start(self.data_offset))?;
 
         let reader = &mut self.reader;
 
+        let mut row_buffer = allocate_row_buffer(total_row_len)?;
+
         with_rows(
             buf,
             self.width,
             self.height,
             num_channels,
             self.top_down,
             |row| {
-                for pixel in row.chunks_mut(num_channels) {
-                    if *format == FormatFullBytes::Format888 {
-                        reader.read_u8()?;
-                    }
+                reader.read_exact(&mut row_buffer)?;
 
-                    // Read the colour values (b, g, r).
-                    // Reading 3 bytes and reversing them is significantly faster than reading one
-                    // at a time.
-                    reader.read_exact(&mut pixel[0..3])?;
-                    pixel[0..3].reverse();
+                for (i, pixel) in row.chunks_mut(num_channels).enumerate() {
+                    let offset = match *format {
+                        FormatFullBytes::Format888 => i * 4 + 1, // Skip first byte
+                        _ => {
+                            i * match *format {
+                                FormatFullBytes::RGB24 => 3,
+                                _ => 4,
+                            }
+                        }
+                    };
 
-                    if *format == FormatFullBytes::RGB32 {
-                        reader.read_u8()?;
-                    }
+                    // Read the colour values (b, g, r) and reverse to (r, g, b)
+                    pixel[0..3].copy_from_slice(&row_buffer[offset..offset + 3]);
+                    pixel[0..3].reverse();
 
                     // Read the alpha channel if present
                     if *format == FormatFullBytes::RGBA32 {
-                        reader.read_exact(&mut pixel[3..4])?;
+                        pixel[3] = row_buffer[offset + 3];
                     } else if num_channels == 4 {
                         pixel[3] = ALPHA_OPAQUE;
                     }
                 }
-                reader.read_exact(row_padding)
+                Ok(())
             },
         )?;
 
@@ -1347,23 +1436,30 @@ impl<R: BufRead + Seek> BmpDecoder<R> {
         // two rows.
         let mut row_iter = self.rows(buf);
 
+        // Wrap reader in buffered RLE reader for efficient byte-by-byte access
+        let mut rle_reader = RleReader::new(&mut self.reader);
+
+        // Pre-allocate buffer for RLE absolute mode (max 256 bytes)
+        let mut rle_indices_buffer = [0u8; 256];
+
         while let Some(row) = row_iter.next() {
             let mut pixel_iter = row.chunks_exact_mut(num_channels);
 
             let mut x = 0;
             loop {
                 let instruction = {
-                    let control_byte = self.reader.read_u8()?;
+                    let control_byte = rle_reader.read_byte()?;
+
                     match control_byte {
                         RLE_ESCAPE => {
-                            let op = self.reader.read_u8()?;
+                            let op = rle_reader.read_byte()?;
 
                             match op {
                                 RLE_ESCAPE_EOL => RLEInsn::EndOfRow,
                                 RLE_ESCAPE_EOF => RLEInsn::EndOfFile,
                                 RLE_ESCAPE_DELTA => {
-                                    let xdelta = self.reader.read_u8()?;
-                                    let ydelta = self.reader.read_u8()?;
+                                    let xdelta = rle_reader.read_byte()?;
+                                    let ydelta = rle_reader.read_byte()?;
                                     RLEInsn::Delta(xdelta, ydelta)
                                 }
                                 _ => {
@@ -1372,14 +1468,13 @@ impl<R: BufRead + Seek> BmpDecoder<R> {
                                         length = length.div_ceil(2);
                                     }
                                     length += length & 1;
-                                    let mut buffer = Vec::new();
-                                    self.reader.read_exact_vec(&mut buffer, length)?;
-                                    RLEInsn::Absolute(op, buffer)
+                                    rle_reader.read_exact(&mut rle_indices_buffer[..length])?;
+                                    RLEInsn::Absolute(op, &rle_indices_buffer[..length])
                                 }
                             }
                         }
                         _ => {
-                            let palette_index = self.reader.read_u8()?;
+                            let palette_index = rle_reader.read_byte()?;
                             RLEInsn::PixelRun(control_byte, palette_index)
                         }
                     }