Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds first-class WebP support: new Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant UI as UI Renderer
participant Action as Action System
participant Jobs as Job System
participant Loader as WebP Loader
participant Cache as WebpTexCache
Client->>UI: request render (WebP URL)
UI->>Action: FetchImage(WebP)
Action->>Jobs: enqueue WebP load (url)
Jobs->>Loader: load from disk or network
Loader->>Loader: detect_webp_type(bytes)
alt Animated
Loader->>Loader: generate_webp_anim_pkg()
Loader->>Cache: insert Animation + frames
else Static
Loader->>Cache: insert TextureHandle
end
Cache-->>UI: texture + WebpState
UI->>UI: schedule repaints / render frames
UI-->>Client: display updated frames
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/notedeck/src/jobs/media.rs (1)
26-64:⚠️ Potential issue | 🟠 MajorFix WebP cache routing inconsistency in pre-action phase.
WebP files are marked Pending in
animated.cache(media.rs:81-82) but results are delivered towebp.cache(media.rs:49-50). Since requests query the type-specific cache (webp_cacheforMediaCacheType::Webp), the Pending state is never found, causing duplicate requests for the same WebP file. Align pre-action to mark Pending inwebp.cachefor WebP files, or detect the file extension during job kind assignment to avoid extension-based routing altogether.
🤖 Fix all issues with AI agents
In `@Cargo.toml`:
- Line 59: Update the webp crate version in Cargo.toml from the open-ended "0.3"
to the fixed release "0.3.1" to address RUSTSEC-2024-0443; locate the dependency
entry for the webp crate (the line containing webp = "0.3") and change it to
webp = "0.3.1" so Cargo will pull the patched 0.3.1 release (this affects the
standalone webp crate APIs like AnimDecoder and Encoder).
In `@crates/notedeck/src/imgcache.rs`:
- Around line 232-258: The current animated-WebP branch encodes frames into
encoded_frames but then writes only the first frame to disk
(encoded_frames.first()) causing animated images to be cached as static; fix by
caching the original network bytes instead of re-encoding: in the else branch
where you iterate over data/frame_data and build encoded_frames, replace the
re-encode-and-write logic with writing the original downloaded byte buffer (the
raw bytes that produced data) to file_path so from_disk_job_run and WebpType
detection see the original animated WebP; reference the variables
encoded_frames, frame_data, file_path, data, and the
from_disk_job_run/WebpType::Static behavior when making the change and add a
TODO comment to implement proper animation encoding later.
In `@crates/notedeck/src/jobs/media.rs`:
- Around line 47-53: The routing currently inspects
id.to_lowercase().ends_with(".webp") in the delivery path and always inserts
TextureState::Pending into tex_cache.animated.cache in run_media_job_pre_action,
causing mismatches for WebP results; update the job metadata instead: add a
MediaJobKind::WebpImg (or include a MediaCacheType field on the job) and change
run_media_job_pre_action to insert Pending into the appropriate cache based on
that kind/field, then change the delivery code (the branch that now checks
id.ends_with(".webp")) to consult job.kind/MediaCacheType and insert the result
into the matching tex_cache.webp.cache or tex_cache.animated.cache so pre-action
and delivery use the same canonical cache routing.
- Line 32: The local binding job_kind (completed.job_id.job_kind.clone()) is
unused and causes a compiler warning; either remove that line or use job_kind to
drive the routing logic instead of relying on file extension string matching.
Locate the code that currently inspects file extensions for dispatch
(referencing completed and completed.job_id) and replace the extension checks
with a match/if on the job_kind value (or, if you prefer removal, delete the let
job_kind = ... line and keep the existing extension logic). Ensure you update
any pattern matching or routing functions so they consume job_kind consistently
and eliminate the unused-variable warning.
In `@crates/notedeck/src/media/webp.rs`:
- Around line 345-384: get_or_request currently dispatches a job (via jobs.send
and JobPackage in get_or_request) but never records the URL as pending in
self.cache, which lets duplicate jobs be sent; after successfully sending the
job (both the sync from_disk branch and the async from_net branch) insert a
Pending marker into self.cache keyed by MediaCache::key(url) (i.e.
self.cache.insert(key.clone(), TextureState::Pending) or equivalent) so
subsequent calls return the cached Pending state instead of re-dispatching; also
verify run_media_job_pre_action updates the same cache (webp cache) used here
rather than animated.cache so the post-job state transitions operate on the same
cache.
- Around line 81-101: The fallback detector try_detect_with_image_crate
currently decodes and collects all frames via decoder.into_frames().collect()
just to check frames.len(), causing heavy allocation/decoding for animated WebP;
instead implement a lightweight check that avoids full frame decoding (e.g.,
iterate frames streamingly without collecting or better: parse the RIFF
container for ANMF chunks) and return WebpType::Animated on the first ANMF/frame
header found, WebpType::Static if only a single frame/header is present, and an
error if none; update try_detect_with_image_crate to use either a streaming
iterator over decoder.into_frames() (counting up to 2 frames) or a RIFF/ANMF
parser rather than building a VecDeque<Frame>, keeping function signature and
error messages intact.
- Around line 52-78: detect_webp_type currently calls
webp::AnimDecoder::new(...).decode() which fully decodes frames (via
anim_decoder.decode()) and is expensive; instead either parse the WebP RIFF
headers to look for the ANIM chunk (i.e., inspect the header bytes for the
"RIFF" / "WEBP" / "ANIM" chunk markers) to determine animation without decoding,
or change detect_webp_type to use a lightweight header-only check before falling
back to try_detect_with_image_crate; update the function (detect_webp_type) to
implement the header-chunk inspection path (or adjust the docstring to state
that full decode occurs) and remove the full decode call (anim_decoder.decode())
from the hot path so large animated WebPs are not fully decoded unnecessarily.
- Around line 117-141: The decode_static_webp function currently assumes
decode() returns RGBA and passes image.as_ref() into
ColorImage::from_rgba_unmultiplied, which will panic for RGB data; update
decode_static_webp to mirror decode_static_webp_processed: check
image.color_mode() or image.as_ref().len() to detect 3 vs 4 bytes per pixel,
convert RGB->RGBA by inserting an opaque alpha byte for each pixel when
necessary, then construct the ColorImage via ColorImage::from_rgba_unmultiplied
and call load_texture_checked as before (referencing decode_static_webp,
decode_static_webp_processed, ColorImage::from_rgba_unmultiplied, and
load_texture_checked to locate the code).
🧹 Nitpick comments (7)
crates/notedeck/src/media/webp.rs (3)
221-298:process_webp_frameduplicates GIF frame processing logic.
WebpStateis structurally identical toGifState, andprocess_webp_framemirrors the GIF equivalent. This violates DRY — consider a genericprocess_animation_framefunction parameterized over the state type, or unifyGifState/WebpStateinto a singleAnimationFrameState.
456-465: Unnecessaryimg.clone()for static WebP from network.Line 465:
img.clone()copies the entireColorImagepixel buffer just to pass it into the write-to-disk closure. Since theColorImageis consumed byload_texture_checkedonly for its data (which is uploaded to GPU), consider restructuring to avoid the clone — e.g., write to disk first, then create the texture, or useArc<ColorImage>.
558-564: Each animation frame'sColorImageis cloned.Line 563:
image: img.clone()copies the full pixel buffer for every frame to buildImageFramefor disk caching, after the same data is also used to create a GPU texture. For a 100-frame animation at 512×512, this is ~100 MB of extra allocations. Consider taking ownership ofimgfor theImageFrameand creating the texture from borrowed data, or usingArc.crates/notedeck/src/imgcache.rs (1)
513-520:WebpStateis structurally identical toGifState— consider unifying.Both structs have the same four fields with the same types. A single
AnimationFrameStatetype alias or struct could serve both, reducing maintenance burden and enabling shared frame-processing logic.crates/notedeck_ui/src/note/media.rs (1)
134-140: Nit: combine identical match arms.
GifandWebpboth returnSome(24.0). You can merge them into a single arm for conciseness.♻️ Suggested diff
- let fps = match media_type { - MediaCacheType::Gif => Some(24.0), - MediaCacheType::Webp => Some(24.0), - MediaCacheType::Image => None, - }; + let fps = match media_type { + MediaCacheType::Gif | MediaCacheType::Webp => Some(24.0), + MediaCacheType::Image => None, + };crates/notedeck/src/media/latest.rs (2)
116-136: LGTM — but consider extracting shared animation logic.The WebP branch is correct and follows the established GIF pattern. However, the GIF (lines 93–115) and WebP (lines 116–136) branches are nearly identical, and the underlying
process_gif_frame/process_webp_framefunctions (ingif.rsandwebp.rs) are also structurally identical. This duplication will compound as more animated formats are added.Consider introducing a common
AnimatedCachetrait (or a generic helper) that both GIF and WebP caches implement, solatest_statecan handle any animated format through a single code path. This isn't blocking, but worth tracking for a follow-up.
33-55:NoLoadingLatestTexgrows linearly with each new animated format.Each new animated format (GIF, WebP, and any future ones) adds two fields (cache + state map) to this struct and its constructor. A map-based or trait-object approach would keep this struct stable as formats are added. Same recommendation as above — not blocking, but good to keep in mind.
|
@sergey3bv thanks for the PR. Can you please address coderabbitai review feedback, and run your code against AGENTS.md found in #1263 |
Yes, I am working on it |
sergey3bv
force-pushed
the
feat/webp-support
branch
3 times, most recently
from
e02fbb8 to
e5f035d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/notedeck/src/media/webp.rs (2)
405-424: Minor: preferis_empty()overlen() == 0.Line 413 uses
f.len() == 0. Clippy'slen_zerolint typically suggestsf.is_empty()if available on the type. Verify the type returned byAnimDecoder::decode()supportsis_empty(); if so, switch to it. As per coding guidelines: "Runcargo fmt, cargo clippy, cargo testbefore committing."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/media/webp.rs` around lines 405 - 424, In generate_webp_anim_pkg, replace the length check f.len() == 0 with f.is_empty() (if the collection returned by AnimDecoder::decode() supports is_empty()) to satisfy Clippy's len_zero lint; update the match branch that handles the decoded frames from AnimDecoder::decode() to use f.is_empty() for the empty-case check and then proceed to return f as before, and run cargo fmt, cargo clippy, cargo test before committing.
296-330:from_disk_job_rungenerates and discardsimg_framesfor animated WebP loaded from cache.
generate_webp_anim_pkgproduces bothanim(GPU textures) andimg_frames(CPUColorImagecopies). The disk path only uses.map(|f| f.anim), meaning every frame'sColorImageis cloned (line 452) and immediately dropped. For large animations this is a non-trivial waste of memory and CPU.Consider a flag or a separate decode path that skips
img_framesgeneration when loading from disk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/media/webp.rs` around lines 296 - 330, from_disk_job_run is calling generate_webp_anim_pkg and then immediately discarding img_frames by mapping to f.anim, causing unnecessary cloning of ColorImage frames; change generate_webp_anim_pkg to accept a flag (e.g., produce_cpu_frames: bool) or add a new function (e.g., generate_webp_anim_pkg_no_cpu_frames) that decodes only GPU textures (anim) without creating img_frames, then call that new/flagged path from the Animated branch in from_disk_job_run so it returns only anim and avoids generating or cloning ColorImage frames (refer to generate_webp_anim_pkg, img_frames, anim, and buffer_to_color_image to locate the code to change).crates/notedeck/src/imgcache.rs (1)
489-505:WebpStateis structurally identical toGifState— consider a shared animation state type.Both
GifStateandWebpStatehave the exact same fields (last_frame_rendered,last_frame_duration,next_frame_time,last_frame_index). Similarly,process_webp_frameinwebp.rsis a near-verbatim copy ofprocess_gif_frameingif.rs. Extracting a genericAnimFrameState(and a genericprocess_anim_frame) would eliminate this duplication and simplify adding future animated formats.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/imgcache.rs` around lines 489 - 505, GifState and WebpState are identical duplicates (and GifStateMap/WebpStateMap mirror each other) and process_gif_frame/process_webp_frame are near-verbatim; replace them with a single generic AnimFrameState struct (or AnimState) containing last_frame_rendered, last_frame_duration, next_frame_time, last_frame_index and use a type alias (e.g., AnimStateMap) for HashMap<String, AnimFrameState>, then refactor process_gif_frame and process_webp_frame into a shared process_anim_frame function that accepts format-specific behavior via parameters or a trait so both gif.rs and webp.rs call the same generic logic with only format-specific pieces provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/notedeck/src/imgcache.rs`:
- Around line 224-241: The timestamp calculation uses index *
current_frame_delay which breaks for variable delays; change it to compute a
cumulative presentation timestamp by summing each frame's delay (with the same
as_millis() -> clamp-to-300ms fallback logic) for all frames up to the current
one and pass that running total to AnimFrame::from_rgba; implement a mutable
accumulator (e.g., let mut cumulative_ts = 0i32) outside/above the for (index,
frame) in data.iter().enumerate() loop and update it per-frame before calling
encoder.add_frame so encoder.add_frame(..., cumulative_ts) reflects the correct
ms-from-start presentation time.
---
Duplicate comments:
In `@crates/notedeck/src/media/webp.rs`:
- Around line 254-293: get_or_request currently takes &self and dispatches jobs
without marking the URL Pending in the webp cache, causing duplicate jobs;
either update the job pre-action in jobs/media.rs (run_media_job_pre_action) so
that when handling MediaJobKind::WebpImg it inserts
TextureState::<WebpCacheEntry>::Pending into the webp cache key for the URL, or
change get_or_request to take &mut self and immediately insert Pending into
self.cache (using MediaCache::key(url) and TextureState::Pending) right after
dispatching the JobPackage; reference get_or_request,
TextureState::<WebpCacheEntry>::Pending, MediaJobKind::WebpImg, and
run_media_job_pre_action to locate the places to update.
---
Nitpick comments:
In `@crates/notedeck/src/imgcache.rs`:
- Around line 489-505: GifState and WebpState are identical duplicates (and
GifStateMap/WebpStateMap mirror each other) and
process_gif_frame/process_webp_frame are near-verbatim; replace them with a
single generic AnimFrameState struct (or AnimState) containing
last_frame_rendered, last_frame_duration, next_frame_time, last_frame_index and
use a type alias (e.g., AnimStateMap) for HashMap<String, AnimFrameState>, then
refactor process_gif_frame and process_webp_frame into a shared
process_anim_frame function that accepts format-specific behavior via parameters
or a trait so both gif.rs and webp.rs call the same generic logic with only
format-specific pieces provided.
In `@crates/notedeck/src/media/webp.rs`:
- Around line 405-424: In generate_webp_anim_pkg, replace the length check
f.len() == 0 with f.is_empty() (if the collection returned by
AnimDecoder::decode() supports is_empty()) to satisfy Clippy's len_zero lint;
update the match branch that handles the decoded frames from
AnimDecoder::decode() to use f.is_empty() for the empty-case check and then
proceed to return f as before, and run cargo fmt, cargo clippy, cargo test
before committing.
- Around line 296-330: from_disk_job_run is calling generate_webp_anim_pkg and
then immediately discarding img_frames by mapping to f.anim, causing unnecessary
cloning of ColorImage frames; change generate_webp_anim_pkg to accept a flag
(e.g., produce_cpu_frames: bool) or add a new function (e.g.,
generate_webp_anim_pkg_no_cpu_frames) that decodes only GPU textures (anim)
without creating img_frames, then call that new/flagged path from the Animated
branch in from_disk_job_run so it returns only anim and avoids generating or
cloning ColorImage frames (refer to generate_webp_anim_pkg, img_frames, anim,
and buffer_to_color_image to locate the code to change).
e5f035d to
935f5ee
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
crates/notedeck/src/media/webp.rs (2)
411-414: Preferis_empty()overlen() == 0.Clippy's
clippy::len_zerolint flags this pattern. As per coding guidelines,cargo clippymust pass before committing.♻️ Proposed fix
- if image.len() == 0 { + if image.is_empty() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/media/webp.rs` around lines 411 - 414, Replace the manual length check used when decoding frames with the idiomatic is_empty() call to satisfy clippy's len_zero lint: in the AnimDecoder::new(webp_bytes).decode() match arm where you currently test `if image.len() == 0` (the block that returns Err(Generic("No frames found in webp"...))), change it to `if image.is_empty()` so the decoded-empty check uses is_empty() instead of len() == 0.
405-410: Missing#[profiling::function]ongenerate_webp_anim_pkgandCopybound needs verification.Two concerns:
generate_webp_anim_pkgfully decodes all frames and uploads GPU textures for each — this is the single most expensive call in the WebP loading path. Per coding guidelines, performance-critical functions should be annotated with#[profiling::function].The
process_to_egui: impl Fn(DynamicImage) -> ColorImage + Send + Copy + 'staticbound requires the closure to beCopy. Infrom_net_run(Line 380) this ismove |img| process_image(imgtype, img), which isCopyonly ifImageType: Copy. Verify thatImageTypederivesCopy; if not, this is a compile error.♻️ Proposed annotation
+#[profiling::function] fn generate_webp_anim_pkg(As per coding guidelines: "Mark performance-critical functions with
#[profiling::function]for visibility in the puffin profiler."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/media/webp.rs` around lines 405 - 410, The function generate_webp_anim_pkg is performance-critical and should be annotated with #[profiling::function]; add that attribute above its signature. Also remove or relax the unnecessary Copy requirement on the process_to_egui parameter (currently impl Fn(DynamicImage) -> ColorImage + Send + Copy + 'static) — either drop + Copy or change the bound to a Clone/Arc-wrapped closure as appropriate; if you keep Copy then ensure the captured ImageType used in from_net_run is actually derived Copy (verify ImageType derives Copy) and if not, make ImageType Copy or change the closure bound instead so from_net_run's move |img| process_image(...) compiles. Ensure only one of these fixes is applied to match how the closure and ImageType are used.crates/notedeck/src/imgcache.rs (2)
196-248: Previous encoding bugs addressed;foldfor side effects is a minor code smell.Both previously flagged issues — dropping all frames except the first, and the incorrect non-cumulative timestamp — are now correctly handled: all frames are written via
AnimEncoder, and thefoldaccumulates a running cumulative timestamp. Good.The
foldis used solely for its side effect of mutatingencoder, which is unconventional. Aforloop with an explicitaccvariable is more readable and avoidslet _ =suppression.♻️ Proposed refactor
- let _ = data.iter().fold(0i32, |acc_timestamp, frame| { - let [width, height] = frame.image.size; - let delay = frame.delay.as_millis(); - let frame_delay = if delay < i32::MAX as u128 { - delay as i32 - } else { - 300i32 - }; - - let timestamp = acc_timestamp; - - encoder.add_frame(AnimFrame::from_rgba( - frame.image.as_raw(), - width as u32, - height as u32, - timestamp, - )); - - acc_timestamp.saturating_add(frame_delay) - }); + let mut cumulative_ts = 0i32; + for frame in &data { + let [width, height] = frame.image.size; + let delay_ms = { + let d = frame.delay.as_millis(); + if d < i32::MAX as u128 { d as i32 } else { 300i32 } + }; + encoder.add_frame(AnimFrame::from_rgba( + frame.image.as_raw(), + width as u32, + height as u32, + cumulative_ts, + )); + cumulative_ts = cumulative_ts.saturating_add(delay_ms); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/imgcache.rs` around lines 196 - 248, The write_webp function currently uses data.iter().fold(...) solely for side effects on encoder which is a code smell; replace the fold with an explicit for loop that declares let mut acc_timestamp = 0i32 and then iterates over data (or data.iter()) computing frame_delay and timestamp the same way and calling encoder.add_frame(AnimFrame::from_rgba(...)) inside the loop, updating acc_timestamp with acc_timestamp = acc_timestamp.saturating_add(frame_delay); keep the same uses of encoder, frame_delay, timestamp, and AnimFrame::from_rgba to preserve behavior.
490-506:WebpStateandGifStateare identical structs.Both contain exactly the same four fields (
last_frame_rendered,last_frame_duration,next_frame_time,last_frame_index). Consider unifying them into a single genericAnimFrameStatestruct to avoid duplication and keep future maintenance in sync.♻️ Proposed refactor
-pub type GifStateMap = HashMap<String, GifState>; - -pub struct GifState { - pub last_frame_rendered: Instant, - pub last_frame_duration: Duration, - pub next_frame_time: Option<SystemTime>, - pub last_frame_index: usize, -} - -pub type WebpStateMap = HashMap<String, WebpState>; - -pub struct WebpState { - pub last_frame_rendered: Instant, - pub last_frame_duration: Duration, - pub next_frame_time: Option<SystemTime>, - pub last_frame_index: usize, -} +/// Shared playback state for frame-based animations (GIF, WebP, etc.) +pub struct AnimFrameState { + pub last_frame_rendered: Instant, + pub last_frame_duration: Duration, + pub next_frame_time: Option<SystemTime>, + pub last_frame_index: usize, +} + +pub type GifStateMap = HashMap<String, AnimFrameState>; +pub type GifState = AnimFrameState; // back-compat alias +pub type WebpStateMap = HashMap<String, AnimFrameState>; +pub type WebpState = AnimFrameState; // back-compat alias🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/imgcache.rs` around lines 490 - 506, The two identical structs GifState and WebpState should be merged into a single AnimFrameState struct to remove duplication: replace both GifState and WebpState definitions with one pub struct AnimFrameState containing the existing fields (last_frame_rendered, last_frame_duration, next_frame_time, last_frame_index) and update GifStateMap and WebpStateMap to be type aliases using AnimFrameState (e.g., pub type GifStateMap = HashMap<String, AnimFrameState>; pub type WebpStateMap = HashMap<String, AnimFrameState>), then update any references to GifState or WebpState to use AnimFrameState (or keep short type aliases if you want to retain names).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/notedeck/src/media/webp.rs`:
- Around line 254-293: get_or_request currently uses &self so it never records a
Pending marker in self.cache, allowing duplicate job dispatches; fix by either
changing get_or_request (and request) to take &mut self and immediately insert
TextureState::Pending into self.cache (or replace the cache entry) right after
enqueueing the JobPackage (use the same key produced by MediaCache::key(url)),
or keep &self and add an interior-mutable in-flight set to WebpTexCache (e.g.,
HashSet<String> behind Mutex/RwLock/RefCell) and check/insert the url into that
set before sending the job via jobs.send to prevent re-dispatch. Ensure you
reference WebpTexCache, get_or_request, self.cache, TextureState::Pending, and
the JobPackage enqueue points when making the change.
---
Nitpick comments:
In `@crates/notedeck/src/imgcache.rs`:
- Around line 196-248: The write_webp function currently uses
data.iter().fold(...) solely for side effects on encoder which is a code smell;
replace the fold with an explicit for loop that declares let mut acc_timestamp =
0i32 and then iterates over data (or data.iter()) computing frame_delay and
timestamp the same way and calling encoder.add_frame(AnimFrame::from_rgba(...))
inside the loop, updating acc_timestamp with acc_timestamp =
acc_timestamp.saturating_add(frame_delay); keep the same uses of encoder,
frame_delay, timestamp, and AnimFrame::from_rgba to preserve behavior.
- Around line 490-506: The two identical structs GifState and WebpState should
be merged into a single AnimFrameState struct to remove duplication: replace
both GifState and WebpState definitions with one pub struct AnimFrameState
containing the existing fields (last_frame_rendered, last_frame_duration,
next_frame_time, last_frame_index) and update GifStateMap and WebpStateMap to be
type aliases using AnimFrameState (e.g., pub type GifStateMap = HashMap<String,
AnimFrameState>; pub type WebpStateMap = HashMap<String, AnimFrameState>), then
update any references to GifState or WebpState to use AnimFrameState (or keep
short type aliases if you want to retain names).
In `@crates/notedeck/src/media/webp.rs`:
- Around line 411-414: Replace the manual length check used when decoding frames
with the idiomatic is_empty() call to satisfy clippy's len_zero lint: in the
AnimDecoder::new(webp_bytes).decode() match arm where you currently test `if
image.len() == 0` (the block that returns Err(Generic("No frames found in
webp"...))), change it to `if image.is_empty()` so the decoded-empty check uses
is_empty() instead of len() == 0.
- Around line 405-410: The function generate_webp_anim_pkg is
performance-critical and should be annotated with #[profiling::function]; add
that attribute above its signature. Also remove or relax the unnecessary Copy
requirement on the process_to_egui parameter (currently impl Fn(DynamicImage) ->
ColorImage + Send + Copy + 'static) — either drop + Copy or change the bound to
a Clone/Arc-wrapped closure as appropriate; if you keep Copy then ensure the
captured ImageType used in from_net_run is actually derived Copy (verify
ImageType derives Copy) and if not, make ImageType Copy or change the closure
bound instead so from_net_run's move |img| process_image(...) compiles. Ensure
only one of these fixes is applied to match how the closure and ImageType are
used.
|
@alltheseas, I think the PR is ready for a review. |
run_media_job_pre_action inserts Pending into tex_cache.webp.cache (jobs/media.rs:77): MediaJobKind::WebpImg => { Completed jobs return MediaJobResult::StaticImg / Animation, which deliver_completed_media_job routes to the wrong caches (jobs/media.rs:39,46): MediaJobResult::StaticImg(job_complete) => { The UI reads only webp_cache (latest.rs:116): MediaCacheType::Webp => match self.webp_cache.get_or_request(jobs, ctx, url, imgtype) { Entries stay Pending indefinitely. Feature is non-functional.
Adds webp = "0.3.1" (Cargo.toml:59) pulling libwebp-sys, when image already has WebP enabled (Cargo.toml:58): image = { version = "0.25", features = ["jpeg", "png", "webp"] } The image crate's WebPDecoder implements AnimationDecoder::into_frames() — the same trait used by GifDecoder (gif.rs:22,255): // Existing GIF pattern: // Could reuse for WebP:
Near-copy pairs. Example — WebpState vs GifState (imgcache.rs:501 vs imgcache.rs:492): // GifState // WebpState — identical Same applies to process_webp_frame/process_gif_frame (webp.rs:133 vs gif.rs:33), WebpTexCache/AnimatedImgTexCache (webp.rs:218 vs gif.rs:112), from_disk_job_run
decode_static_webp / decode_static_webp_processed (webp.rs:69-123) duplicate what two existing paths already do via image::load_from_memory(). Network path } else if content_type.starts_with("image/") { And the disk static loader (static_imgs.rs:114). Both already handle all image/* types including WebP.
write_webp (imgcache.rs:196-248) re-encodes every frame through AnimEncoder: let mut config = webp::WebPConfig::new().or(/* ... */)?; Lossless, so no quality loss — but the original network bytes could be written directly to disk, avoiding unnecessary CPU work.
generate_webp_anim_pkg produces both anim and img_frames. The disk path discards img_frames immediately (webp.rs:313): generate_webp_anim_pkg(ctx.clone(), url.to_owned(), webp_bytes.as_slice(), |img| {
Not human-reviewable per project guidelines. Includes unrelated whitespace changes (Cargo.toml:20,52): -chrono = "0.4.40" Should be 3-4 logically distinct standalone commits.
generate_webp_anim_pkg (webp.rs:405) does full frame decode + texture upload with no profiling annotation: fn generate_webp_anim_pkg( // no #[profiling::function] Comparable existing functions (images.rs:52,129,176) all use #[profiling::function].
(imgcache.rs:224): let _ = data.iter().fold(0i32, |acc_timestamp, frame| { A for loop with an explicit accumulator is more readable.
(notedeck_ui/src/note/media.rs:134-135): let fps = match media_type { |
|
@alltheseas, got it, will fix it next week |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/notedeck/src/imgcache.rs (1)
196-247: Add profiler annotation on the new WebP encode hot path.
write_webpis CPU-heavy (frame loop + encoding). It should be annotated for puffin visibility.Proposed fix
+ #[profiling::function] pub fn write_webp(cache_dir: &path::Path, url: &str, data: Vec<ImageFrame>) -> Result<()> {As per coding guidelines "Mark performance-critical functions with
#[profiling::function]for visibility in the puffin profiler".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/imgcache.rs` around lines 196 - 247, Add a puffin profiler annotation to the CPU-heavy WebP encode path by putting the attribute #[profiling::function] directly above the pub fn write_webp(...) declaration in imgcache.rs; this marks the write_webp function for profiling (ensure the profiling crate is in scope/compiled in this crate as required).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/notedeck/src/imgcache.rs`:
- Around line 61-65: Add missing rustdoc comments for the newly added public
API: document TextureState::is_loaded (what it checks/returns), the write_webp
function (purpose, parameters, return/behavior), the WebpStateMap type (what it
maps/usage), and the WebpState enum (variants meaning and lifecycle). Place
concise /// comments above the definitions of TextureState::is_loaded,
write_webp, WebpStateMap, and WebpState describing their intent, inputs/outputs,
and any important invariants or thread-safety/async notes so they meet the
crate's docstring coverage guidelines.
- Around line 218-241: The animated WebP encoder is built from
reference_frame.image.size but subsequent frames are added without checking
dimensions, which can break AnimEncoder; before calling AnimFrame::from_rgba and
encoder.add_frame in the loop, validate that frame.image.size equals
reference_frame.image.size (compare width and height) and handle mismatches
(either resize/pad the frame to the canvas size or return an error/skip the
frame) so that all frames match the canvas; update the loop around
encoder.add_frame / timestamp to perform this size check using the
frame.image.size, and if you choose to error, return a descriptive error; if you
choose to resize, use the existing image utils/helpers so frames passed to
AnimFrame::from_rgba always match the encoder canvas.
---
Nitpick comments:
In `@crates/notedeck/src/imgcache.rs`:
- Around line 196-247: Add a puffin profiler annotation to the CPU-heavy WebP
encode path by putting the attribute #[profiling::function] directly above the
pub fn write_webp(...) declaration in imgcache.rs; this marks the write_webp
function for profiling (ensure the profiling crate is in scope/compiled in this
crate as required).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/notedeck/src/imgcache.rscrates/notedeck_ui/src/note/media.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/notedeck_ui/src/note/media.rs
sergey3bv
force-pushed
the
feat/webp-support
branch
3 times, most recently
from
d634888 to
e3124f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/notedeck/src/media/webp.rs (1)
54-56:⚠️ Potential issue | 🟠 Major
detect_webp_typestill does a full decode on fallback.This path decodes frames just to classify static vs animated, then decoding happens again in the actual load path. That is expensive for large files and was already flagged earlier.
For Rust crate `webp` version `0.3.1`, does `AnimDecoder::decode()` eagerly decode all frame pixel data into memory? Please cite docs.rs source lines.crates/notedeck/src/imgcache.rs (1)
60-64:⚠️ Potential issue | 🟠 MajorAdd rustdoc for newly added/modified public API items.
TextureState::is_loaded,GifState,GifStateMap,WebpState, andWebpStateMapstill need doc comments to satisfy repo standards.Suggested patch
impl<T> TextureState<T> { + /// Returns `true` when the texture state is fully loaded. pub fn is_loaded(&self) -> bool { matches!(self, TextureState::Loaded(_)) } } @@ +/// Per-URL playback state map for GIF animations. pub type GifStateMap = HashMap<String, GifState>; +/// Playback timing/index state for a single GIF animation. pub type GifState = crate::media::animated::AnimatedFrameState; +/// Per-URL playback state map for animated WebP media. pub type WebpStateMap = HashMap<String, WebpState>; +/// Playback timing/index state for a single animated WebP media item. pub type WebpState = crate::media::animated::AnimatedFrameState;As per coding guidelines "Ensure docstring coverage for any code added or modified".
Also applies to: 447-451
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/imgcache.rs` around lines 60 - 64, Add missing rustdoc comments for the newly added public API: document TextureState::is_loaded, the GifState type, GifStateMap, WebpState, and WebpStateMap; locate the impl block containing pub fn is_loaded and the type definitions/aliases for GifState/GifStateMap and WebpState/WebpStateMap and add brief /// comments describing their purpose, behavior, and any important invariants or usage notes so they meet the repo's docstring coverage standards.
🧹 Nitpick comments (3)
crates/notedeck/src/media/webp.rs (1)
281-297: Static WebP network path re-encodes before disk cache write.At Line 293,
MediaCache::write(&path, &url, image)writes re-encoded content, while the original bytes are already available from the response. If byte-preserving cache is desired (as in animated path), write source bytes after successful decode/validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/media/webp.rs` around lines 281 - 297, The static WebP branch currently calls MediaCache::write(&path, &url, image) which writes the re-encoded image bytes instead of preserving the original response bytes; after successful decode/validation in decode_static_webp_processed (and after creating the texture via load_texture_checked and constructing WebpCacheEntry::Static) change the cache write to use the original response bytes (res.bytes) rather than the decoded/re-encoded image, and keep the write guarded so it only runs on the Ok(image) path (i.e., after decode_static_webp_processed returns Ok) to ensure byte-preserving disk caching consistent with the animated path.crates/notedeck/src/media/latest.rs (1)
93-139: Consider extracting shared animated-render logic for GIF/WebP branches.Both branches duplicate the same frame-state update + repaint scheduling flow. A small helper would reduce divergence risk and simplify future animation fixes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/media/latest.rs` around lines 93 - 139, Both GIF and WebP branches duplicate the same "process frame -> update stored per-url state -> schedule repaint -> return texture" logic; extract that into a small helper to avoid duplication. Create a helper (e.g., handle_animated_next_state) that accepts the result from process_gif_frame/process_webp_frame (the next_state), a mutable reference to the per-url state map (gif_state or webp_state), the url, and the ctx; have it insert maybe_new_state into the provided state map, schedule repaint via ctx.request_repaint_after when next_state.repaint_at exists, and return LatestImageTex::Loaded(next_state.texture). Replace the duplicated blocks in the MediaCacheType::Gif branch (after process_gif_frame) and the WebpCacheEntry::Animated branch (after process_webp_frame) with calls to this helper, keeping error/pending handling unchanged for TextureState and WebpCacheEntry::Static.crates/notedeck/src/media/gif.rs (1)
119-123: Disk GIF load still buildsimg_framesthat are immediately discarded.This path returns only
anim(.map(|f| f.anim)) butgenerate_anim_pkgstill clones/collects every frame intoimg_frames. Consider a disk-specific builder path that skipsimg_framesentirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/notedeck/src/media/gif.rs` around lines 119 - 123, The disk-load path is wasting work by building img_frames in generate_anim_pkg even though the caller only needs f.anim (see the map(|f| f.anim) usage); modify or add a disk-specific builder variant (e.g., a new function or a flag on generate_anim_pkg) that skips cloning/collecting per-frame data (img_frames) and instead constructs the AnimPackage/anim directly from the encoded gif_bytes using buffer_to_color_image only as needed for metadata or first-frame operations; update the call site currently invoking generate_anim_pkg(..., |img| buffer_to_color_image(...)) to call the new disk-only builder so no img_frames are allocated when only anim is returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 64-65: The image crate's webp feature was removed but code uses
image::codecs::webp::WebPEncoder and generic image::load_from_memory paths that
require that feature; re-enable the webp feature on the image dependency in
Cargo.toml (restore features = ["jpeg","png","webp"] or add "webp") so the
WebPEncoder and WebP decoding paths compile, ensuring the image dependency entry
(image = { version = "0.25", features = [...] }) includes "webp".
In `@crates/notedeck/src/media/gif.rs`:
- Line 32: The GIF wrapper call to process_animation_frame(animation,
frame_state, animation_mode, false) disables scheduling of the initial repaint;
change the call so the initial render passes true (or otherwise sets repaint_at)
so the function can set repaint_at for GIFs — e.g., detect first-frame/initial
state via frame_state or animation_mode and call process_animation_frame(...,
true) to enable initial repaint scheduling instead of passing false.
---
Duplicate comments:
In `@crates/notedeck/src/imgcache.rs`:
- Around line 60-64: Add missing rustdoc comments for the newly added public
API: document TextureState::is_loaded, the GifState type, GifStateMap,
WebpState, and WebpStateMap; locate the impl block containing pub fn is_loaded
and the type definitions/aliases for GifState/GifStateMap and
WebpState/WebpStateMap and add brief /// comments describing their purpose,
behavior, and any important invariants or usage notes so they meet the repo's
docstring coverage standards.
---
Nitpick comments:
In `@crates/notedeck/src/media/gif.rs`:
- Around line 119-123: The disk-load path is wasting work by building img_frames
in generate_anim_pkg even though the caller only needs f.anim (see the map(|f|
f.anim) usage); modify or add a disk-specific builder variant (e.g., a new
function or a flag on generate_anim_pkg) that skips cloning/collecting per-frame
data (img_frames) and instead constructs the AnimPackage/anim directly from the
encoded gif_bytes using buffer_to_color_image only as needed for metadata or
first-frame operations; update the call site currently invoking
generate_anim_pkg(..., |img| buffer_to_color_image(...)) to call the new
disk-only builder so no img_frames are allocated when only anim is returned.
In `@crates/notedeck/src/media/latest.rs`:
- Around line 93-139: Both GIF and WebP branches duplicate the same "process
frame -> update stored per-url state -> schedule repaint -> return texture"
logic; extract that into a small helper to avoid duplication. Create a helper
(e.g., handle_animated_next_state) that accepts the result from
process_gif_frame/process_webp_frame (the next_state), a mutable reference to
the per-url state map (gif_state or webp_state), the url, and the ctx; have it
insert maybe_new_state into the provided state map, schedule repaint via
ctx.request_repaint_after when next_state.repaint_at exists, and return
LatestImageTex::Loaded(next_state.texture). Replace the duplicated blocks in the
MediaCacheType::Gif branch (after process_gif_frame) and the
WebpCacheEntry::Animated branch (after process_webp_frame) with calls to this
helper, keeping error/pending handling unchanged for TextureState and
WebpCacheEntry::Static.
In `@crates/notedeck/src/media/webp.rs`:
- Around line 281-297: The static WebP branch currently calls
MediaCache::write(&path, &url, image) which writes the re-encoded image bytes
instead of preserving the original response bytes; after successful
decode/validation in decode_static_webp_processed (and after creating the
texture via load_texture_checked and constructing WebpCacheEntry::Static) change
the cache write to use the original response bytes (res.bytes) rather than the
decoded/re-encoded image, and keep the write guarded so it only runs on the
Ok(image) path (i.e., after decode_static_webp_processed returns Ok) to ensure
byte-preserving disk caching consistent with the animated path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fec05267-b26e-44ab-979b-eea4df3f91b4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlcrates/notedeck/Cargo.tomlcrates/notedeck/src/imgcache.rscrates/notedeck/src/jobs/media.rscrates/notedeck/src/lib.rscrates/notedeck/src/media/action.rscrates/notedeck/src/media/animated.rscrates/notedeck/src/media/gif.rscrates/notedeck/src/media/latest.rscrates/notedeck/src/media/mod.rscrates/notedeck/src/media/webp.rscrates/notedeck/src/urls.rscrates/notedeck_ui/src/note/media.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/notedeck/src/media/action.rs
- crates/notedeck/src/media/mod.rs
- crates/notedeck_ui/src/note/media.rs
- crates/notedeck/Cargo.toml
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-Authored-By: Codex <codex@openai.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-Authored-By: Codex <codex@openai.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-Authored-By: Codex <codex@openai.com>
sergey3bv
force-pushed
the
feat/webp-support
branch
from
e3124f0 to
291e4e3
Compare
|
@alltheseas @kernelkind @jb55, I think now the PR is ready. |
2 participants
Addressing #753 to support both static and animated WEBPs.
Summary by CodeRabbit
New Features
Chores