Coalesce animated image frame data into a single shared memory region (#37058)

This makes servo use less file descriptors for animated images and
avoids the crash described in
https://github.com/servo/servo/issues/36792.

Doing this also forces the end users to be more explicit about whether
they want to deal with all image frames or just the first one.
Previously, `Image::bytes` silently returned only the data for the first
frame. With this change there's now a `frames` method which returns an
iterator over all frames in the image.

Testing: No tests - this simply reduces the number of fds used. Servo
doesn't currently display animated gifs anyways.
Fixes: https://github.com/servo/servo/issues/36792

Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
This commit is contained in:
Simon Wülker 2025-05-20 22:40:46 +02:00 committed by GitHub
parent d5e02d27be
commit 384d8f1ff8
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 75 additions and 36 deletions

View file

@ -1456,10 +1456,11 @@ impl IOCompositor {
format: PixelFormat::RGBA8, format: PixelFormat::RGBA8,
frames: vec![ImageFrame { frames: vec![ImageFrame {
delay: None, delay: None,
bytes: ipc::IpcSharedMemory::from_bytes(&image), byte_range: 0..image.len(),
width: image.width(), width: image.width(),
height: image.height(), height: image.height(),
}], }],
bytes: ipc::IpcSharedMemory::from_bytes(&image),
id: None, id: None,
cors_status: CorsStatus::Safe, cors_status: CorsStatus::Safe,
})) }))

View file

@ -66,10 +66,10 @@ fn set_webrender_image_key(compositor_api: &CrossProcessCompositorApi, image: &m
return; return;
} }
let mut bytes = Vec::new(); let mut bytes = Vec::new();
let frame_bytes = image.bytes(); let frame_bytes = image.first_frame().bytes;
let is_opaque = match image.format { let is_opaque = match image.format {
PixelFormat::BGRA8 => { PixelFormat::BGRA8 => {
bytes.extend_from_slice(&frame_bytes); bytes.extend_from_slice(frame_bytes);
pixels::rgba8_premultiply_inplace(bytes.as_mut_slice()) pixels::rgba8_premultiply_inplace(bytes.as_mut_slice())
}, },
PixelFormat::RGB8 => { PixelFormat::RGB8 => {

View file

@ -4,6 +4,7 @@
use std::borrow::Cow; use std::borrow::Cow;
use std::io::Cursor; use std::io::Cursor;
use std::ops::Range;
use std::time::Duration; use std::time::Duration;
use std::{cmp, fmt, vec}; use std::{cmp, fmt, vec};
@ -126,13 +127,24 @@ pub struct Image {
pub format: PixelFormat, pub format: PixelFormat,
pub id: Option<ImageKey>, pub id: Option<ImageKey>,
pub cors_status: CorsStatus, pub cors_status: CorsStatus,
pub bytes: IpcSharedMemory,
pub frames: Vec<ImageFrame>, pub frames: Vec<ImageFrame>,
} }
#[derive(Clone, Deserialize, MallocSizeOf, Serialize)] #[derive(Clone, Deserialize, MallocSizeOf, Serialize)]
pub struct ImageFrame { pub struct ImageFrame {
pub delay: Option<Duration>, pub delay: Option<Duration>,
pub bytes: IpcSharedMemory, /// References a range of the `bytes` field from the image that this
/// frame belongs to.
pub byte_range: Range<usize>,
pub width: u32,
pub height: u32,
}
/// A non-owning reference to the data of an [ImageFrame]
pub struct ImageFrameView<'a> {
pub delay: Option<Duration>,
pub bytes: &'a [u8],
pub width: u32, pub width: u32,
pub height: u32, pub height: u32,
} }
@ -142,12 +154,19 @@ impl Image {
self.frames.len() > 1 self.frames.len() > 1
} }
pub fn bytes(&self) -> IpcSharedMemory { pub fn frames(&self) -> impl Iterator<Item = ImageFrameView> {
self.frames self.frames.iter().map(|frame| ImageFrameView {
.first() delay: frame.delay,
.expect("Should have at least one frame") bytes: self.bytes.get(frame.byte_range.clone()).unwrap(),
.bytes width: frame.width,
.clone() height: frame.height,
})
}
pub fn first_frame(&self) -> ImageFrameView {
self.frames()
.next()
.expect("All images should have at least one frame")
} }
} }
@ -189,7 +208,7 @@ pub fn load_from_memory(buffer: &[u8], cors_status: CorsStatus) -> Option<Image>
rgba8_byte_swap_colors_inplace(&mut rgba); rgba8_byte_swap_colors_inplace(&mut rgba);
let frame = ImageFrame { let frame = ImageFrame {
delay: None, delay: None,
bytes: IpcSharedMemory::from_bytes(&rgba), byte_range: 0..rgba.len(),
width: rgba.width(), width: rgba.width(),
height: rgba.height(), height: rgba.height(),
}; };
@ -198,6 +217,7 @@ pub fn load_from_memory(buffer: &[u8], cors_status: CorsStatus) -> Option<Image>
height: rgba.height(), height: rgba.height(),
format: PixelFormat::BGRA8, format: PixelFormat::BGRA8,
frames: vec![frame], frames: vec![frame],
bytes: IpcSharedMemory::from_bytes(&rgba),
id: None, id: None,
cors_status, cors_status,
}) })
@ -364,36 +384,52 @@ fn decode_gif(buffer: &[u8], cors_status: CorsStatus) -> Option<Image> {
// This uses `map_while`, because the first non-decodable frame seems to // This uses `map_while`, because the first non-decodable frame seems to
// send the frame iterator into an infinite loop. See // send the frame iterator into an infinite loop. See
// <https://github.com/image-rs/image/issues/2442>. // <https://github.com/image-rs/image/issues/2442>.
let mut frame_data = vec![];
let mut total_number_of_bytes = 0;
let frames: Vec<ImageFrame> = decoded_gif let frames: Vec<ImageFrame> = decoded_gif
.into_frames() .into_frames()
.map_while(|decoded_frame| { .map_while(|decoded_frame| {
let mut frame = match decoded_frame { let mut gif_frame = match decoded_frame {
Ok(decoded_frame) => decoded_frame, Ok(decoded_frame) => decoded_frame,
Err(error) => { Err(error) => {
debug!("decode GIF frame error: {error}"); debug!("decode GIF frame error: {error}");
return None; return None;
}, },
}; };
rgba8_byte_swap_colors_inplace(frame.buffer_mut()); rgba8_byte_swap_colors_inplace(gif_frame.buffer_mut());
let frame_start = total_number_of_bytes;
let frame = ImageFrame { total_number_of_bytes += gif_frame.buffer().len();
bytes: IpcSharedMemory::from_bytes(frame.buffer()),
delay: Some(Duration::from(frame.delay())),
width: frame.buffer().width(),
height: frame.buffer().height(),
};
// The image size should be at least as large as the largest frame. // The image size should be at least as large as the largest frame.
width = cmp::max(width, frame.width); let frame_width = gif_frame.buffer().width();
height = cmp::max(height, frame.height); let frame_height = gif_frame.buffer().height();
width = cmp::max(width, frame_width);
height = cmp::max(height, frame_height);
let frame = ImageFrame {
byte_range: frame_start..total_number_of_bytes,
delay: Some(Duration::from(gif_frame.delay())),
width: frame_width,
height: frame_height,
};
frame_data.push(gif_frame);
Some(frame) Some(frame)
}) })
.collect(); .collect();
if frames.is_empty() { if frames.is_empty() {
debug!("Animated Image decoding error"); debug!("Animated Image decoding error");
None return None;
} else { }
// Coalesce the frame data into one single shared memory region.
let mut bytes = Vec::with_capacity(total_number_of_bytes);
for frame in frame_data {
bytes.extend_from_slice(frame.buffer());
}
Some(Image { Some(Image {
width, width,
height, height,
@ -401,8 +437,8 @@ fn decode_gif(buffer: &[u8], cors_status: CorsStatus) -> Option<Image> {
frames, frames,
id: None, id: None,
format: PixelFormat::BGRA8, format: PixelFormat::BGRA8,
bytes: IpcSharedMemory::from_bytes(&bytes),
}) })
}
} }
#[cfg(test)] #[cfg(test)]

View file

@ -17,7 +17,7 @@ use cssparser::color::clamp_unit_f32;
use cssparser::{Parser, ParserInput}; use cssparser::{Parser, ParserInput};
use euclid::default::{Point2D, Rect, Size2D, Transform2D}; use euclid::default::{Point2D, Rect, Size2D, Transform2D};
use euclid::vec2; use euclid::vec2;
use ipc_channel::ipc::{self, IpcSender}; use ipc_channel::ipc::{self, IpcSender, IpcSharedMemory};
use net_traits::image_cache::{ImageCache, ImageResponse}; use net_traits::image_cache::{ImageCache, ImageResponse};
use net_traits::request::CorsSettings; use net_traits::request::CorsSettings;
use pixels::PixelFormat; use pixels::PixelFormat;
@ -350,7 +350,7 @@ impl CanvasState {
size.cast(), size.cast(),
format, format,
alpha_mode, alpha_mode,
img.bytes(), IpcSharedMemory::from_bytes(img.first_frame().bytes),
)) ))
} }

View file

@ -619,7 +619,8 @@ impl WebGLRenderingContext {
let size = Size2D::new(img.width, img.height); let size = Size2D::new(img.width, img.height);
TexPixels::new(img.bytes(), size, img.format, false) let data = IpcSharedMemory::from_bytes(img.first_frame().bytes);
TexPixels::new(data, size, img.format, false)
}, },
// TODO(emilio): Getting canvas data is implemented in CanvasRenderingContext2D, // TODO(emilio): Getting canvas data is implemented in CanvasRenderingContext2D,
// but we need to refactor it moving it to `HTMLCanvasElement` and support // but we need to refactor it moving it to `HTMLCanvasElement` and support

View file

@ -1709,7 +1709,8 @@ impl Handler {
"Unexpected screenshot pixel format" "Unexpected screenshot pixel format"
); );
let rgb = RgbaImage::from_raw(img.width, img.height, img.bytes().to_vec()).unwrap(); let rgb =
RgbaImage::from_raw(img.width, img.height, img.first_frame().bytes.to_vec()).unwrap();
let mut png_data = Cursor::new(Vec::new()); let mut png_data = Cursor::new(Vec::new());
DynamicImage::ImageRgba8(rgb) DynamicImage::ImageRgba8(rgb)
.write_to(&mut png_data, ImageFormat::Png) .write_to(&mut png_data, ImageFormat::Png)