Repository navigation
Conversation
Co-authored-by: Kyzer <kyzer1023@users.noreply.github.com>
|
@kyzer1023 is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let bytes = [ | ||
| 0xFF, 0xD8, 0xFF, 0xC0, 0x00, 0x14, 0x08, 0x00, 0x08, 0x00, 0x08, 0x04, 0x01, 0x11, | ||
| 0x00, 0x02, 0x11, 0x00, 0x03, 0x11, 0x00, 0x04, 0x11, 0x00, | ||
| ]; | ||
| assert!(decode_jpeg_thumbnail(&bytes, |w, h| (w, h)).is_none()); |
There was a problem hiding this comment.
cmyk_frame_returns_none uses a truncated JPEG header with no scan data. The test passes when decoding fails, even if CMYK rejection is removed. Use a complete CMYK JPEG so the test guards the unsupported-format branch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/jpeg_thumbnail.rs
Line: 101-105
Comment:
**CMYK test misses its branch**
`cmyk_frame_returns_none` uses a truncated JPEG header with no scan data. The test passes when decoding fails, even if CMYK rejection is removed. Use a complete CMYK JPEG so the test guards the unsupported-format branch.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in b519849. The test now builds a complete 8x8 CMYK JPEG, asserts the fixture itself decodes as CMYK32, then asserts the thumbnail path returns None.
| let pixels = decoder.decode().ok()?; | ||
| let decoded = match info.pixel_format { |
There was a problem hiding this comment.
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.
| 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.There was a problem hiding this comment.
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.
| Some((decoded, u32::from(info.width), u32::from(info.height))) | ||
| } | ||
|
|
||
| /// Cover size for a card. Never upscales. |
There was a problem hiding this comment.
The new cover_target comment only describes what the function does. The repository guide bans comments that narrate code and requires non-obvious context instead. Remove this comment or express the guarantee through a test. This repository requirement must be satisfied before merging.
| /// Cover size for a card. Never upscales. |
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/jpeg_thumbnail.rs
Line: 34
Comment:
**Comment breaks the guide**
The new `cover_target` comment only describes what the function does. The repository guide bans comments that narrate code and requires non-obvious context instead. Remove this comment or express the guarantee through a test. This repository requirement must be satisfied before merging.
```suggestion
```
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Removed in b519849, along with the other restating comment on decode_jpeg_thumbnail.
Co-authored-by: Kyzer <kyzer1023@users.noreply.github.com>
|
@greptileai review |
|
Thanks @kyzer1023, the IDCT path looks right. Two small things: 1. Bumping the cache prefix ( |
Co-authored-by: Kyzer <kyzer1023@users.noreply.github.com>
|
Thanks @richiemcilroy, both addressed in d16afcb.
The |
|
@greptileai review |
Summary
Library card thumbnails (
thumbnail_rgbainapps/desktop-gpui/src/library.rs) decoded JPEGs at full resolution and then resampled to the 392×224 cover size. JPEGs now usejpeg_decoder::Decoder::scaleto IDCT-scale toward the cover size first, then resize to the exact size. A 1920×1080 or 3840×2160 frame decodes at 480×270.The sidebar's existing IDCT decode moved into a shared
jpeg_thumbnail.rs, sodecode_jpeg_thumbnailis the only IDCT decoder. The sidebar still asks for a square max; library cards pass the cover size. PNG is unchanged. Unsupported formats (CMYK, 16-bit gray) are rejected afterread_info()and before any decode, so they go straight to the existing full-decode fallback.The resize filter is picked from the decoded buffer's size against the cover target (
thumbnail_resize), so the near 1:1 step after IDCT (for example 480×270 to 398×224) uses Triangle, while a full-size buffer still uses box sampling. Nothing is upscaled.Cache keys are unchanged (
thumbnail-<mtime>.jpg,preview-thumbnail-<mtime>.jpg, and the shared{hash}-{w}x{h}-<mtime>.jpg), so existing thumbnails stay valid and no orphaned files are left behind.3 commits, 4 files, +246/-32. Based on
mainat85219015.Correctness
jpeg_thumbnailtests pass (5): square downsample, historical cover sizes, 1080p to 480×270, a complete 8×8 CMYK JPEG returnsNone, and the resize choice follows the decoded buffer rather than the original scale. Run through a standalone crate that path-includes the module; clippy and rustfmt are clean on the touched files. The full desktop-gpui app and the cache-slot tests inlibrary.rswere not built here because gpui is patched to a localzed-capcheckout.Benchmarks
Release, Linux VM, median of 7 samples after one warmup. Before is the old
imagefull decode plus cover resample.sequoia-dark.jpg4000×2500tahoe-dark.jpg3840×2160A second run put synthetic 1080p at 1.67× and 4K at 1.87×. These numbers predate the switch to Triangle for the post-IDCT resize.
Overlap
Does not touch clip thumbnails (#2707) or recording recovery in
library.rs(#2630).
The PR appears safe to merge; no actionable issues remain.Summary
The PR shares JPEG thumbnail decoding between the sidebar and library. Library cards decode fewer pixels before resizing.
This review checked the code; it did not run the app or tests.
Reviews (3) · Last reviewed commit: "address review feedback" · Reviewed by Greptile