Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 1 addition & 16 deletions apps/desktop-gpui/src/editor_sidebar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1895,22 +1895,7 @@ fn decode_scaled_rgba(path: &Path, max: u32) -> Option<image::RgbaImage> {
}

fn decode_jpeg_thumbnail(bytes: &[u8], max: u32) -> Option<(image::DynamicImage, u32, u32)> {
let mut decoder = jpeg_decoder::Decoder::new(bytes);
decoder.read_info().ok()?;
let info = decoder.info()?;
let requested = max.clamp(1, u16::MAX as u32) as u16;
let (width, height) = decoder.scale(requested, requested).ok()?;
let pixels = decoder.decode().ok()?;
let decoded = match info.pixel_format {
jpeg_decoder::PixelFormat::RGB24 => image::DynamicImage::ImageRgb8(
image::RgbImage::from_raw(width as u32, height as u32, pixels)?,
),
jpeg_decoder::PixelFormat::L8 => image::DynamicImage::ImageLuma8(
image::GrayImage::from_raw(width as u32, height as u32, pixels)?,
),
_ => return None,
};
Some((decoded, info.width as u32, info.height as u32))
crate::jpeg_thumbnail::decode_jpeg_thumbnail(bytes, |_, _| (max, max))
}

/// [`decode_scaled_rgba`] in gpui's BGRA order.
Expand Down
153 changes: 153 additions & 0 deletions apps/desktop-gpui/src/jpeg_thumbnail.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
use image::DynamicImage;

pub fn decode_jpeg_thumbnail(
bytes: &[u8],
request: impl FnOnce(u32, u32) -> (u32, u32),
) -> Option<(DynamicImage, u32, u32)> {
let mut decoder = jpeg_decoder::Decoder::new(bytes);
decoder.read_info().ok()?;
let info = decoder.info()?;
match info.pixel_format {
jpeg_decoder::PixelFormat::RGB24 | jpeg_decoder::PixelFormat::L8 => {}
_ => return None,
}
let (requested_width, requested_height) =
request(u32::from(info.width), u32::from(info.height));
let requested_width = requested_width.clamp(1, u32::from(u16::MAX)) as u16;
let requested_height = requested_height.clamp(1, u32::from(u16::MAX)) as u16;
let (width, height) = decoder.scale(requested_width, requested_height).ok()?;
let pixels = decoder.decode().ok()?;
let decoded = match info.pixel_format {
Comment on lines +19 to +20

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Unsupported JPEGs decode twice

The shared helper decodes pixels before rejecting unsupported formats. Library thumbnails now pay for that discarded decode and then decode again through the fallback. Check info.pixel_format before calling decoder.decode() so CMYK and 16-bit gray files go straight to the fallback.

Suggested change
let pixels = decoder.decode().ok()?;
let decoded = match info.pixel_format {
if !matches!(
info.pixel_format,
jpeg_decoder::PixelFormat::RGB24 | jpeg_decoder::PixelFormat::L8
) {
return None;
}
let pixels = decoder.decode().ok()?;
let decoded = match info.pixel_format {
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/jpeg_thumbnail.rs
Line: 17-18

Comment:
**Unsupported JPEGs decode twice**

The shared helper decodes pixels before rejecting unsupported formats. Library thumbnails now pay for that discarded decode and then decode again through the fallback. Check `info.pixel_format` before calling `decoder.decode()` so CMYK and 16-bit gray files go straight to the fallback.

```suggestion
    if !matches!(
        info.pixel_format,
        jpeg_decoder::PixelFormat::RGB24 | jpeg_decoder::PixelFormat::L8
    ) {
        return None;
    }
    let pixels = decoder.decode().ok()?;
    let decoded = match info.pixel_format {
```

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b519849. It calls read_info() first and returns None for anything other than RGB24 or L8 before scale or decode, so unsupported files only go through the fallback decode once.

jpeg_decoder::PixelFormat::RGB24 => DynamicImage::ImageRgb8(image::RgbImage::from_raw(
u32::from(width),
u32::from(height),
pixels,
)?),
jpeg_decoder::PixelFormat::L8 => DynamicImage::ImageLuma8(image::GrayImage::from_raw(
u32::from(width),
u32::from(height),
pixels,
)?),
_ => return None,
};
Some((decoded, u32::from(info.width), u32::from(info.height)))
}

pub fn cover_target(width: u32, height: u32, card_width: u32, card_height: u32) -> (f32, u32, u32) {
let width = width.max(1);
let height = height.max(1);
let scale = (card_width as f32 / width as f32)
.max(card_height as f32 / height as f32)
.min(1.0);
let target_width = ((width as f32 * scale).round() as u32).max(1);
let target_height = ((height as f32 * scale).round() as u32).max(1);
(scale, target_width, target_height)
}

#[cfg(test)]
mod tests {
use super::*;

const CARD_WIDTH: u32 = 392;
const CARD_HEIGHT: u32 = 224;

fn encode_rgb(width: u32, height: u32, pixel: [u8; 3]) -> Vec<u8> {
let image = image::RgbImage::from_pixel(width, height, image::Rgb(pixel));
let mut encoded = Vec::new();
image::codecs::jpeg::JpegEncoder::new_with_quality(&mut encoded, 75)
.encode_image(&image)
.unwrap();
encoded
}

#[test]
fn square_request_downsamples_before_the_full_frame() {
let encoded = encode_rgb(1024, 512, [40, 120, 200]);
let (decoded, width, height) = decode_jpeg_thumbnail(&encoded, |_, _| (128, 128)).unwrap();
assert_eq!((width, height), (1024, 512));
assert!(decoded.width() <= 256);
assert!(decoded.height() <= 128);
let pixel = decoded.to_rgb8().get_pixel(0, 0).0;
assert!(pixel[0].abs_diff(40) <= 3);
assert!(pixel[1].abs_diff(120) <= 3);
assert!(pixel[2].abs_diff(200) <= 3);
}

#[test]
fn library_cover_targets_match_the_card() {
assert_eq!(
cover_dims(800, 600),
(392, 294),
"800x600 covering the card"
);
assert_eq!(cover_dims(600, 600), (392, 392));
assert_eq!(cover_dims(1920, 1080), (398, 224));
assert_eq!(cover_dims(1512, 982), (392, 255));
}

#[test]
fn library_cover_idct_scales_1080p_to_a_quarter() {
let encoded = encode_rgb(1920, 1080, [40, 120, 200]);
let (decoded, width, height) = decode_jpeg_thumbnail(&encoded, |w, h| {
let (_, target_width, target_height) = cover_target(w, h, CARD_WIDTH, CARD_HEIGHT);
(target_width, target_height)
})
.unwrap();
assert_eq!((width, height), (1920, 1080));
assert_eq!((decoded.width(), decoded.height()), (480, 270));
}

#[test]
fn cmyk_frame_returns_none() {
let bytes = cmyk_jpeg();
let mut decoder = jpeg_decoder::Decoder::new(bytes.as_slice());
let pixels = decoder.decode().expect("cmyk fixture decodes");
assert_eq!(
decoder.info().map(|info| info.pixel_format),
Some(jpeg_decoder::PixelFormat::CMYK32)
);
assert_eq!(pixels.len(), 8 * 8 * 4);
assert!(decode_jpeg_thumbnail(&bytes, |w, h| (w, h)).is_none());
}

fn cmyk_jpeg() -> Vec<u8> {
fn marker(out: &mut Vec<u8>, tag: u8, payload: &[u8]) {
out.extend_from_slice(&[0xFF, tag]);
let len = u16::try_from(payload.len() + 2).unwrap();
out.extend_from_slice(&len.to_be_bytes());
out.extend_from_slice(payload);
}
fn huffman(class: u8) -> Vec<u8> {
let mut table = vec![class, 0x01];
table.extend(std::iter::repeat_n(0x00, 15));
table.push(0x00);
table
}
let mut quant = vec![0x00];
quant.extend(std::iter::repeat_n(0x01, 64));
let mut frame = vec![0x08, 0x00, 0x08, 0x00, 0x08, 0x04];
for id in 1..=4u8 {
frame.extend_from_slice(&[id, 0x11, 0x00]);
}
let mut scan = vec![0x04];
for id in 1..=4u8 {
scan.extend_from_slice(&[id, 0x00]);
}
scan.extend_from_slice(&[0x00, 0x3F, 0x00]);

let mut bytes = vec![0xFF, 0xD8];
marker(&mut bytes, 0xDB, &quant);
marker(&mut bytes, 0xC0, &frame);
marker(&mut bytes, 0xC4, &huffman(0x00));
marker(&mut bytes, 0xC4, &huffman(0x10));
marker(&mut bytes, 0xDA, &scan);
bytes.push(0x00);
bytes.extend_from_slice(&[0xFF, 0xD9]);
bytes
}

fn cover_dims(width: u32, height: u32) -> (u32, u32) {
let (_, target_width, target_height) = cover_target(width, height, CARD_WIDTH, CARD_HEIGHT);
(target_width, target_height)
}
}
53 changes: 39 additions & 14 deletions apps/desktop-gpui/src/library.rs
Original file line number Diff line number Diff line change
Expand Up @@ -808,6 +808,9 @@ pub fn recent_media() -> Vec<RecentItem> {
const THUMBNAIL_WIDTH: u32 = 392;
const THUMBNAIL_HEIGHT: u32 = 224;

/// IDCT-scaled JPEG thumbnails are not the old full-decode pixels.
const THUMBNAIL_CACHE_GENERATION: &str = "idct";

/// Cached thumbnails re-encode at 80: `display.jpg` is itself a quality-75
/// JPEG, and at a few hundred pixels wide the double re-encode stays clean
/// while the biggest cache file lands around 25KB.
Expand Down Expand Up @@ -959,17 +962,17 @@ impl CacheSlot {
return Some(Self {
dir: source.parent()?.to_path_buf(),
prefix: if source.file_name().is_some_and(|name| name == "preview.jpg") {
"preview-thumbnail".to_string()
format!("preview-thumbnail-{THUMBNAIL_CACHE_GENERATION}")
} else {
"thumbnail".to_string()
format!("thumbnail-{THUMBNAIL_CACHE_GENERATION}")
},
mtime,
});
}
Some(Self {
dir: thumbnail_cache_dir(),
prefix: format!(
"{:016x}-{THUMBNAIL_WIDTH}x{THUMBNAIL_HEIGHT}",
"{:016x}-{THUMBNAIL_WIDTH}x{THUMBNAIL_HEIGHT}-{THUMBNAIL_CACHE_GENERATION}",
fnv1a64(source.to_string_lossy().as_bytes())
),
mtime,
Expand Down Expand Up @@ -1094,14 +1097,29 @@ pub fn thumbnail_rgba(path: &Path) -> Option<image::RgbaImage> {
// Sniff rather than trust the extension: `list_screenshots` finds the
// preview by extension scan, and a bundle could hold a mislabelled file.
let format = image::guess_format(&bytes).ok()?;
let decoded = image::load_from_memory_with_format(&bytes, format).ok()?;

let (width, height) = (decoded.width().max(1), decoded.height().max(1));
let scale = (THUMBNAIL_WIDTH as f32 / width as f32)
.max(THUMBNAIL_HEIGHT as f32 / height as f32)
.min(1.0);
let target_width = ((width as f32 * scale).round() as u32).max(1);
let target_height = ((height as f32 * scale).round() as u32).max(1);
let (decoded, width, height) = if format == image::ImageFormat::Jpeg {
crate::jpeg_thumbnail::decode_jpeg_thumbnail(&bytes, |width, height| {
let (_, target_width, target_height) = crate::jpeg_thumbnail::cover_target(
width,
height,
THUMBNAIL_WIDTH,
THUMBNAIL_HEIGHT,
);
(target_width, target_height)
})
.or_else(|| {
let decoded = image::load_from_memory_with_format(&bytes, format).ok()?;
let dimensions = (decoded.width(), decoded.height());
Some((decoded, dimensions.0, dimensions.1))
})?
} else {
let decoded = image::load_from_memory_with_format(&bytes, format).ok()?;
let dimensions = (decoded.width(), decoded.height());
(decoded, dimensions.0, dimensions.1)
};
let (width, height) = (width.max(1), height.max(1));
let (scale, target_width, target_height) =
crate::jpeg_thumbnail::cover_target(width, height, THUMBNAIL_WIDTH, THUMBNAIL_HEIGHT);
// Box sampling for the big ratios (one pass over a multi-megapixel
// source, and at 8x down every output pixel averages a whole block);
// Triangle when the sizes are close, where box would alias and costs
Expand Down Expand Up @@ -2175,7 +2193,11 @@ mod tests {
let file = slot.file();
assert_eq!(file.parent(), Some(screenshots.as_path()));
let name = file.file_name().unwrap().to_str().unwrap();
assert!(name.starts_with("thumbnail-") && name.ends_with(".jpg"));
assert!(
name.starts_with(&format!("thumbnail-{THUMBNAIL_CACHE_GENERATION}-"))
&& name.ends_with(".jpg"),
"{name}"
);

let png = root.join("shot.png");
std::fs::write(&png, b"png bytes").unwrap();
Expand All @@ -2188,8 +2210,11 @@ mod tests {
assert!(
shared
.prefix
.contains(&format!("{THUMBNAIL_WIDTH}x{THUMBNAIL_HEIGHT}")),
"the target size is part of the key, so a size change re-decodes"
.contains(&format!("{THUMBNAIL_WIDTH}x{THUMBNAIL_HEIGHT}"))
&& shared
.prefix
.ends_with(&format!("-{THUMBNAIL_CACHE_GENERATION}")),
"the target size and idct generation are part of the key"
);

assert!(
Expand Down
1 change: 1 addition & 0 deletions apps/desktop-gpui/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ mod feeds;
mod hotkeys;
mod import;
mod installer;
mod jpeg_thumbnail;
mod library;
mod main_window;
mod media_access;
Expand Down