Crate: switch to premultiplied alpha rendering pipeline#623
Merged
Crate: switch to premultiplied alpha rendering pipeline#623
Conversation
🦋 Changeset detectedLatest commit: a5e383d The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughImplemented a JS-native Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
kane50613
changed the title
Contributor
|
Docs preview is ready: https://refactor-tiny-skia.preview.takumi.kane.tw |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/content/docs/load-images.mdx (1)
13-25:⚠️ Potential issue | 🟠 MajorPass the parsed node into
extractResourceUrlshere.The helper usage everywhere else in this PR is
extractResourceUrls(node)afterfromJsx(). Keepingelementhere yields an empty URL list, and the explicitfetchedResourcesarray then disablesrender()'s fallback fetch path, so this example stops loading the external image.🛠️ Suggested doc fix
const element = <img src="https://example.com/image.png" />; // [!code ++:2] -const urls = await extractResourceUrls(element); +const { node } = await fromJsx(element); +const urls = extractResourceUrls(node); const fetchedResources = await fetchResources(urls);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/load-images.mdx` around lines 13 - 25, The example currently passes the JSX element directly into extractResourceUrls, which returns no URLs and causes fetchedResources to disable render()'s fallback; call fromJsx(element) to produce the parsed node (e.g., const node = fromJsx(element)) and pass that node into extractResourceUrls (extractResourceUrls(node)), then use the returned urls with fetchResources and pass fetchedResources into render so external images are properly discovered and fetched.
🧹 Nitpick comments (7)
takumi/tests/fixtures/style_overflow.rs (1)
103-105: Align the fixture name with the updated overflow modeLine 103 now tests
Overflow::Clipon one axis, but Line 105 still uses"style_overflow_hidden_visible_image". This makes the snapshot case name misleading. Consider renaming it (and its baseline asset) to something like"style_overflow_clip_visible_image"for clarity.Proposed tweak
- run_fixture_test(container, "style_overflow_hidden_visible_image"); + run_fixture_test(container, "style_overflow_clip_visible_image");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@takumi/tests/fixtures/style_overflow.rs` around lines 103 - 105, The test's snapshot name is inconsistent with the overflow mode: in the call that creates the fixture (create_overflow_fixture with SpacePair::from_pair(Overflow::Clip, Overflow::Visible)) change the run_fixture_test invocation's fixture name string from "style_overflow_hidden_visible_image" to "style_overflow_clip_visible_image" and update the corresponding baseline asset file name to match so the snapshot name reflects Overflow::Clip; verify any other references to the old name are updated as well.takumi/src/rendering/write.rs (1)
150-152: PNGqualityis now silently ignored inwrite_image.Line 151 hardwires encoder settings, so PNG callers passing
qualityno longer influence output. Please document this behavior explicitly (or reject PNG-specific quality input) to avoid surprising API changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@takumi/src/rendering/write.rs` around lines 150 - 152, The PNG encoder is being initialized in write_image with png::Encoder::new and configure_png_encoder(&mut encoder) which ignores the caller-provided quality parameter; update write_image to either honor quality for PNGs or explicitly reject/validate PNG-specific quality inputs: locate the write_image signature and its quality parameter, then (a) if supporting quality for PNG, map the quality value to appropriate encoder settings before/after configure_png_encoder, or (b) if not supporting it, return an error or early validation when format == PNG (or document the behavior in the public API docs and add a clear runtime check that rejects PNG quality), referencing the write_image function, png::Encoder::new, and configure_png_encoder to find where to apply the change.takumi/tests/fixtures/style_mix_blend_mode.rs (1)
11-38: Consider extracting a small helper for repeated solid-block nodes.The three block definitions are structurally identical except color. A helper would reduce duplication and make future fixture tuning easier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@takumi/tests/fixtures/style_mix_blend_mode.rs` around lines 11 - 38, The three repeated solid-block node constructions (using Node::container([...]).with_style(Style::default().with(StyleDeclaration::display(Display::Flex)).with(StyleDeclaration::width(Px(120.0))).with(StyleDeclaration::height(Px(200.0))).with(StyleDeclaration::background_color(ColorInput::Value(Color([...])))))) should be extracted into a small helper function (e.g., make_solid_block(color: Color) -> Node) that builds the Style and returns the container Node; replace each inline block with calls to that helper to reduce duplication and make future fixture tweaks easier while keeping the existing symbols (Node::container, Style::default, StyleDeclaration::background_color, Px) intact.takumi/src/rendering/canvas/buffer_pool.rs (1)
86-103: Consider handling the case where buffer length is 0 but capacity is non-zero.The release method skips buffers where
buffer.is_empty(), but a buffer could havelen() == 0with non-zero capacity (e.g., afterclear()). This means a cleared but not-dropped buffer won't be pooled.This is likely acceptable since
acquirecallsresizeanyway, but the condition could be simplified to just check capacity:pub(crate) fn release(&mut self, buffer: Vec<u8>) { - if buffer.is_empty() || buffer.capacity() == 0 { + if buffer.capacity() == 0 { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@takumi/src/rendering/canvas/buffer_pool.rs` around lines 86 - 103, The release method currently bails out if buffer.is_empty(), which prevents pooling Vecs with len==0 but non-zero capacity (e.g., after clear()); change the early-exit to only check capacity (keep the capacity==0 check) so buffers with capacity>0 are eligible for pooling, then proceed with computing the bucket via Self::bucket_index(cap), clamp against BUCKET_COUNT, update self.current_size and push into self.pools[index]; ensure this logic still respects self.max_size and uses the same symbols (release, bucket_index, BUCKET_COUNT, current_size, max_size, pools).takumi/src/layout/style/properties/radial_gradient.rs (1)
334-350: Consider using precomputedinv_radius_x/inv_radius_yfor consistency.The
sample_pixelmethod divides byself.radius_x.max(1e-6)whilebegin_rowuses the precomputedself.inv_radius_x. Using the precomputed values would be more consistent and marginally faster:- let dx = (x as f32 - self.cx) / self.radius_x.max(1e-6); - let dy = (y as f32 - self.cy) / self.radius_y.max(1e-6); + let dx = (x as f32 - self.cx) * self.inv_radius_x; + let dy = (y as f32 - self.cy) * self.inv_radius_y;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@takumi/src/layout/style/properties/radial_gradient.rs` around lines 334 - 350, Replace the ad-hoc divisions in sample_pixel by using the precomputed inverses: update sample_pixel (currently dividing by self.radius_x.max(1e-6) and self.radius_y.max(1e-6)) to use self.inv_radius_x and self.inv_radius_y (which are computed in begin_row) so the normalized dx/dy calculations match begin_row and avoid repeated max(1e-6) work; ensure inv_radius_x/inv_radius_y are used consistently and fallback behavior (non-zero small values) is preserved if those fields can be zero.takumi/src/resources/image.rs (1)
207-213: Consider more robustcurrentColordetection.Using
sanitized_svg.contains("currentColor")is a simple heuristic that works for most cases but may yield false positives ifcurrentColorappears in comments, CDATA sections, or string literals. This would cause unnecessary reparsing (not incorrect rendering).For now this is acceptable, but if SVG parsing performance becomes a concern, consider detecting
currentColorduring the actual parse by inspecting paint attributes on the usvg tree.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@takumi/src/resources/image.rs` around lines 207 - 213, The uses_current_color flag is set via sanitized_svg.contains("currentColor"), which can give false positives; instead, after parsing into the usvg tree (the variable tree used to build SvgSource), inspect the parsed tree's paint/style attributes to detect actual use of currentColor (e.g., traverse nodes and check fill/stroke/style paint values or CSS declarations that resolve to currentColor) and set SvgSource.uses_current_color based on that traversal; replace the sanitized_svg.contains check with this tree-based detection to avoid comment/CDATA/string false positives.takumi/src/rendering/stacking_context.rs (1)
50-67: General blend path demultiplies/remultiplies—verify performance impact.For non-
PlusDarkermodes, the code demultiplies both source and destination pixels, performs blending on straight-alpha values, then converts back to premultiplied:let s = src_pixel.demultiply(); let d = dst_pixel.demultiply(); // ... blend ... *dst_pixel = Color(out.0).into();This per-pixel demultiply/remultiply cycle may be a performance bottleneck for large offscreen compositing operations. Consider whether the blend operations can be adapted to work directly on premultiplied data for hot paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@takumi/src/rendering/stacking_context.rs` around lines 50 - 67, The per-pixel demultiply/remultiply in the loop (calls to src_pixel.demultiply(), dst_pixel.demultiply(), blend_pixel and then Color(out.0).into()) creates a hot-path allocation; change the blending path to operate on premultiplied values when the blend mode is not PlusDarker by either adding a premultiplied-blend variant (e.g., blend_pixel_premultiplied) or overloading blend_pixel to accept premultiplied inputs, then in the loop use src_pixel and dst_pixel directly (no demultiply) and write the blended premultiplied result back to *dst_pixel; keep the demultiply/remultiply only for modes that require straight alpha semantics (such as PlusDarker) and update any callers/tests accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@takumi-helpers/src/utils.ts`:
- Around line 5-18: The CSS URL collector currently adds any url(...) value
verbatim; change collectCssUrls (and reuse cssUrlPattern) to only add fetchable
remote URLs (e.g., absolute http/https, protocol-relative `//`, and allowed data
URLs) so it matches the same filter used for image `src` before calling
fetchResources; ignore bare/local paths like "background" or relative filenames.
Update collectCssUrls to validate each match[2] against that remote/data URL
predicate before urls.add(match[2]) and add a regression test that ensures a
style like backgroundImage: "url(background)" is not passed to fetchResources.
In `@takumi-js/src/render.ts`:
- Line 29: The global Map fetchedResourceCache defined outside render() keeps
ArrayBuffer values indefinitely; change this by removing the unbounded module
singleton and making caching opt-in or bounded: either (a) move cache creation
into render() or into an injected/cache-provider parameter so callers opt-in, or
(b) replace fetchedResourceCache with a bounded cache implementation (LRU or
fixed-size with eviction and optional TTL) that evicts old entries and
revalidates by URL before returning stale bytes; update all references to
fetchedResourceCache in render() and the related fetch logic (the Map at lines
noted and the code that reads/writes it) to use the new injected/bounded cache
interface.
In `@takumi/benches/fixtures.rs`:
- Around line 23-31: The benchmark currently computes an image in render_fixture
(via RenderOptions::builder() -> build() and render(options)) but drops it into
_image, allowing the optimizer to eliminate the work; modify render_fixture to
pass the render(...) result through black_box (i.e., call
black_box(render(options).unwrap())) so the returned image is used in a way the
optimizer cannot remove, keeping the expensive render call alive.
In `@takumi/src/rendering/components/blur.rs`:
- Around line 137-142: The current early return when data.len() != expected
silently ignores a size mismatch (computed as expected from width and height)
and can hide bugs; modify the code in blur.rs so that instead of returning
Ok(()), it returns a descriptive error (e.g., an Err with a message like "image
buffer size mismatch: expected {expected}, got {data.len()}") or at minimum
emits a warning log before returning; update the function's Result type if
needed to propagate the error and reference the existing variables expected,
data.len(), width and height so callers can diagnose the issue.
In `@takumi/src/rendering/components/border.rs`:
- Around line 250-255: The assert_eq! on clip_image dimensions can panic in
release; replace it with graceful handling in the border rendering path: check
(clip_image.width(), clip_image.height()) against (border_box.width as u32,
border_box.height as u32) and if they differ, either return early (propagate an
Err or None from the enclosing function/method) or perform a safe fallback such
as cropping/padding/resizing the clip_image before use; update the code around
clip_image and border_box (the conditional that currently contains assert_eq!)
to implement that non-panicking behavior and ensure callers receive a clear
error/result when sizes mismatch.
In `@takumi/src/rendering/stacking_context.rs`:
- Around line 70-88: blend_plus_darker_pixel_raw_4 is double-premultiplying
colors: after computing straight channel results (red/green/blue) it multiplies
them by result_alpha and calls fast_div_255, which re-applies premultiplication
to already-premultiplied input. Fix by writing the computed red/green/blue
directly into dst_px (dst_px[0] = red; dst_px[1] = green; dst_px[2] = blue;) and
leave dst_px[3] = result_alpha; remove the multiplication by result_alpha and
the fast_div_255 calls while keeping the existing saturating_sub logic and
early-return on src_alpha == 0.
In `@takumi/tests/fixtures/style_mix_blend_mode.rs`:
- Around line 11-57: The fixture currently uses fully opaque colors for all
layers (alpha = 255), so update at least one foreground and/or the background
Color inputs to be semi-transparent to exercise premultiplied-alpha math: edit
the Color([173, 107, 96, 255]), Color([102, 156, 116, 255]), Color([98, 122,
176, 255]) and/or the outer background Color([118, 128, 138, 255]) used in the
foreground variable and the final Node::container(...) so that at least one of
those alpha components is < 255 (e.g., 128) while keeping the RGB channels the
same; keep the same Style/Node structure and only change the alpha values in the
StyleDeclaration::background_color Color(...) literals.
---
Outside diff comments:
In `@docs/content/docs/load-images.mdx`:
- Around line 13-25: The example currently passes the JSX element directly into
extractResourceUrls, which returns no URLs and causes fetchedResources to
disable render()'s fallback; call fromJsx(element) to produce the parsed node
(e.g., const node = fromJsx(element)) and pass that node into
extractResourceUrls (extractResourceUrls(node)), then use the returned urls with
fetchResources and pass fetchedResources into render so external images are
properly discovered and fetched.
---
Nitpick comments:
In `@takumi/src/layout/style/properties/radial_gradient.rs`:
- Around line 334-350: Replace the ad-hoc divisions in sample_pixel by using the
precomputed inverses: update sample_pixel (currently dividing by
self.radius_x.max(1e-6) and self.radius_y.max(1e-6)) to use self.inv_radius_x
and self.inv_radius_y (which are computed in begin_row) so the normalized dx/dy
calculations match begin_row and avoid repeated max(1e-6) work; ensure
inv_radius_x/inv_radius_y are used consistently and fallback behavior (non-zero
small values) is preserved if those fields can be zero.
In `@takumi/src/rendering/canvas/buffer_pool.rs`:
- Around line 86-103: The release method currently bails out if
buffer.is_empty(), which prevents pooling Vecs with len==0 but non-zero capacity
(e.g., after clear()); change the early-exit to only check capacity (keep the
capacity==0 check) so buffers with capacity>0 are eligible for pooling, then
proceed with computing the bucket via Self::bucket_index(cap), clamp against
BUCKET_COUNT, update self.current_size and push into self.pools[index]; ensure
this logic still respects self.max_size and uses the same symbols (release,
bucket_index, BUCKET_COUNT, current_size, max_size, pools).
In `@takumi/src/rendering/stacking_context.rs`:
- Around line 50-67: The per-pixel demultiply/remultiply in the loop (calls to
src_pixel.demultiply(), dst_pixel.demultiply(), blend_pixel and then
Color(out.0).into()) creates a hot-path allocation; change the blending path to
operate on premultiplied values when the blend mode is not PlusDarker by either
adding a premultiplied-blend variant (e.g., blend_pixel_premultiplied) or
overloading blend_pixel to accept premultiplied inputs, then in the loop use
src_pixel and dst_pixel directly (no demultiply) and write the blended
premultiplied result back to *dst_pixel; keep the demultiply/remultiply only for
modes that require straight alpha semantics (such as PlusDarker) and update any
callers/tests accordingly.
In `@takumi/src/rendering/write.rs`:
- Around line 150-152: The PNG encoder is being initialized in write_image with
png::Encoder::new and configure_png_encoder(&mut encoder) which ignores the
caller-provided quality parameter; update write_image to either honor quality
for PNGs or explicitly reject/validate PNG-specific quality inputs: locate the
write_image signature and its quality parameter, then (a) if supporting quality
for PNG, map the quality value to appropriate encoder settings before/after
configure_png_encoder, or (b) if not supporting it, return an error or early
validation when format == PNG (or document the behavior in the public API docs
and add a clear runtime check that rejects PNG quality), referencing the
write_image function, png::Encoder::new, and configure_png_encoder to find where
to apply the change.
In `@takumi/src/resources/image.rs`:
- Around line 207-213: The uses_current_color flag is set via
sanitized_svg.contains("currentColor"), which can give false positives; instead,
after parsing into the usvg tree (the variable tree used to build SvgSource),
inspect the parsed tree's paint/style attributes to detect actual use of
currentColor (e.g., traverse nodes and check fill/stroke/style paint values or
CSS declarations that resolve to currentColor) and set
SvgSource.uses_current_color based on that traversal; replace the
sanitized_svg.contains check with this tree-based detection to avoid
comment/CDATA/string false positives.
In `@takumi/tests/fixtures/style_mix_blend_mode.rs`:
- Around line 11-38: The three repeated solid-block node constructions (using
Node::container([...]).with_style(Style::default().with(StyleDeclaration::display(Display::Flex)).with(StyleDeclaration::width(Px(120.0))).with(StyleDeclaration::height(Px(200.0))).with(StyleDeclaration::background_color(ColorInput::Value(Color([...]))))))
should be extracted into a small helper function (e.g., make_solid_block(color:
Color) -> Node) that builds the Style and returns the container Node; replace
each inline block with calls to that helper to reduce duplication and make
future fixture tweaks easier while keeping the existing symbols
(Node::container, Style::default, StyleDeclaration::background_color, Px)
intact.
In `@takumi/tests/fixtures/style_overflow.rs`:
- Around line 103-105: The test's snapshot name is inconsistent with the
overflow mode: in the call that creates the fixture (create_overflow_fixture
with SpacePair::from_pair(Overflow::Clip, Overflow::Visible)) change the
run_fixture_test invocation's fixture name string from
"style_overflow_hidden_visible_image" to "style_overflow_clip_visible_image" and
update the corresponding baseline asset file name to match so the snapshot name
reflects Overflow::Clip; verify any other references to the old name are updated
as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2bc4551e-7d34-4dda-bc4e-7de201ed5f62
⛔ Files ignored due to path filters (4)
takumi/tests/fixtures-generated/animation_bouncing_text.gifis excluded by!**/*.giftakumi/tests/fixtures-generated/animation_bouncing_text.pngis excluded by!**/*.pngtakumi/tests/fixtures-generated/animation_keyframe_interpolation.gifis excluded by!**/*.giftakumi/tests/fixtures-generated/animation_keyframe_interpolation.pngis excluded by!**/*.png
📒 Files selected for processing (139)
docs/app/playground/worker.tsdocs/content/docs/load-images.mdxdocs/content/docs/typography-and-fonts.mdxtakumi-helpers/src/utils.tstakumi-helpers/test/utils/extract-resource-urls.test.tstakumi-js/src/index.tstakumi-js/src/render.tstakumi-napi-core/src/export.tstakumi-napi-core/src/helper.rstakumi-napi-core/src/lib.rstakumi-napi-core/tests/render.test.tsxtakumi-wasm/package.jsontakumi-wasm/src/helper.rstakumi/Cargo.tomltakumi/benches/fixtures.rstakumi/src/layout/node/mod.rstakumi/src/layout/style/properties/color.rstakumi/src/layout/style/properties/conic_gradient.rstakumi/src/layout/style/properties/filter.rstakumi/src/layout/style/properties/gradient_utils.rstakumi/src/layout/style/properties/linear_gradient.rstakumi/src/layout/style/properties/mod.rstakumi/src/layout/style/properties/radial_gradient.rstakumi/src/rendering/background_drawing.rstakumi/src/rendering/blend.rstakumi/src/rendering/canvas.rstakumi/src/rendering/canvas/buffer_pool.rstakumi/src/rendering/canvas/mask.rstakumi/src/rendering/components/blur.rstakumi/src/rendering/components/border.rstakumi/src/rendering/components/shadow.rstakumi/src/rendering/debug_drawing.rstakumi/src/rendering/image_drawing.rstakumi/src/rendering/inline_drawing.rstakumi/src/rendering/path.rstakumi/src/rendering/stacking_context.rstakumi/src/rendering/text_drawing.rstakumi/src/rendering/write.rstakumi/src/resources/image.rstakumi/tests/fixtures-generated/animation_bouncing_text.webptakumi/tests/fixtures-generated/animation_keyframe_interpolation.webptakumi/tests/fixtures-generated/color_artifacts.webptakumi/tests/fixtures-generated/deep_nesting_stack_overflow.webptakumi/tests/fixtures-generated/inline_complex_nested_fixture.webptakumi/tests/fixtures-generated/inline_image.webptakumi/tests/fixtures-generated/inline_text_decorations.webptakumi/tests/fixtures-generated/inline_vertical_align_types.webptakumi/tests/fixtures-generated/style_backdrop_filter.webptakumi/tests/fixtures-generated/style_backdrop_filter_frosted_glass.webptakumi/tests/fixtures-generated/style_background_clip_border_area.webptakumi/tests/fixtures-generated/style_background_clip_text_gradient.webptakumi/tests/fixtures-generated/style_background_clip_text_multiline.webptakumi/tests/fixtures-generated/style_background_clip_text_radial.webptakumi/tests/fixtures-generated/style_background_image_builder_gradient_layers.webptakumi/tests/fixtures-generated/style_background_image_radial_mixed.webptakumi/tests/fixtures-generated/style_background_image_repeating_gradients.webptakumi/tests/fixtures-generated/style_background_position_percent_25_75.webptakumi/tests/fixtures-generated/style_background_repeat_space.webptakumi/tests/fixtures-generated/style_background_repeat_tile_from_top_left.webptakumi/tests/fixtures-generated/style_background_size_percent_20_20.webptakumi/tests/fixtures-generated/style_border_radius.webptakumi/tests/fixtures-generated/style_border_radius_circle.webptakumi/tests/fixtures-generated/style_border_radius_circle_avatar.webptakumi/tests/fixtures-generated/style_border_radius_per_corner.webptakumi/tests/fixtures-generated/style_border_width_on_image_node.webptakumi/tests/fixtures-generated/style_box_shadow.webptakumi/tests/fixtures-generated/style_box_shadow_inset.webptakumi/tests/fixtures-generated/style_filter.webptakumi/tests/fixtures-generated/style_filter_blur.webptakumi/tests/fixtures-generated/style_filter_combined.webptakumi/tests/fixtures-generated/style_filter_drop_shadow.webptakumi/tests/fixtures-generated/style_filter_sepia.webptakumi/tests/fixtures-generated/style_mask_image_diagonal_gradient.webptakumi/tests/fixtures-generated/style_mask_image_linear_gradient.webptakumi/tests/fixtures-generated/style_mask_image_multiple_gradients.webptakumi/tests/fixtures-generated/style_mask_image_on_image.webptakumi/tests/fixtures-generated/style_mask_image_radial_ellipse.webptakumi/tests/fixtures-generated/style_mask_image_radial_gradient.webptakumi/tests/fixtures-generated/style_mask_image_with_background.webptakumi/tests/fixtures-generated/style_mix_blend_mode.webptakumi/tests/fixtures-generated/style_mix_blend_mode_color.webptakumi/tests/fixtures-generated/style_mix_blend_mode_color_burn.webptakumi/tests/fixtures-generated/style_mix_blend_mode_color_dodge.webptakumi/tests/fixtures-generated/style_mix_blend_mode_darken.webptakumi/tests/fixtures-generated/style_mix_blend_mode_difference.webptakumi/tests/fixtures-generated/style_mix_blend_mode_exclusion.webptakumi/tests/fixtures-generated/style_mix_blend_mode_hard_light.webptakumi/tests/fixtures-generated/style_mix_blend_mode_hue.webptakumi/tests/fixtures-generated/style_mix_blend_mode_isolation.webptakumi/tests/fixtures-generated/style_mix_blend_mode_lighten.webptakumi/tests/fixtures-generated/style_mix_blend_mode_luminosity.webptakumi/tests/fixtures-generated/style_mix_blend_mode_multiply.webptakumi/tests/fixtures-generated/style_mix_blend_mode_normal.webptakumi/tests/fixtures-generated/style_mix_blend_mode_overlay.webptakumi/tests/fixtures-generated/style_mix_blend_mode_plus_darker.webptakumi/tests/fixtures-generated/style_mix_blend_mode_plus_lighter.webptakumi/tests/fixtures-generated/style_mix_blend_mode_saturation.webptakumi/tests/fixtures-generated/style_mix_blend_mode_screen.webptakumi/tests/fixtures-generated/style_mix_blend_mode_soft_light.webptakumi/tests/fixtures-generated/style_object_fit_contain.webptakumi/tests/fixtures-generated/style_object_fit_cover.webptakumi/tests/fixtures-generated/style_object_fit_fill.webptakumi/tests/fixtures-generated/style_object_fit_none.webptakumi/tests/fixtures-generated/style_object_fit_scale_down.webptakumi/tests/fixtures-generated/style_object_position_contain_bottom_right.webptakumi/tests/fixtures-generated/style_object_position_contain_center.webptakumi/tests/fixtures-generated/style_object_position_contain_top_left.webptakumi/tests/fixtures-generated/style_object_position_cover_center.webptakumi/tests/fixtures-generated/style_object_position_cover_top_left.webptakumi/tests/fixtures-generated/style_object_position_none_center.webptakumi/tests/fixtures-generated/style_object_position_none_top_left.webptakumi/tests/fixtures-generated/style_object_position_percentage_25_75.webptakumi/tests/fixtures-generated/style_opacity.webptakumi/tests/fixtures-generated/style_opacity_flex_text_node_vs_nested_container.webptakumi/tests/fixtures-generated/style_opacity_image_with_text.webptakumi/tests/fixtures-generated/style_overflow_clip_image.webptakumi/tests/fixtures-generated/style_overflow_hidden_image.webptakumi/tests/fixtures-generated/style_overflow_hidden_visible_image.webptakumi/tests/fixtures-generated/style_overflow_visible_image.webptakumi/tests/fixtures-generated/style_rotate.webptakumi/tests/fixtures-generated/style_rotate_image.webptakumi/tests/fixtures-generated/style_text_decoration.webptakumi/tests/fixtures-generated/style_text_decoration_thickness.webptakumi/tests/fixtures-generated/style_transform_origin_center.webptakumi/tests/fixtures-generated/style_transform_origin_top_left.webptakumi/tests/fixtures-generated/style_transform_translate_and_scale.webptakumi/tests/fixtures-generated/stylesheets.webptakumi/tests/fixtures-generated/stylesheets_background_multiple_gradients.webptakumi/tests/fixtures-generated/svg_attr_size_in_absolute_flex_container.webptakumi/tests/fixtures-generated/svg_current_color_fixture.webptakumi/tests/fixtures-generated/text_decoration_skip_ink_parapsychologists.webptakumi/tests/fixtures-generated/text_font_synthesis_weight_emoji.webptakumi/tests/fixtures-generated/text_mask_image_gradient_emoji.webptakumi/tests/fixtures-generated/text_shadow.webptakumi/tests/fixtures-generated/text_shadow_no_blur_radius.webptakumi/tests/fixtures-generated/text_stroke_background_clip.webptakumi/tests/fixtures-generated/text_super_bold_stroke_background_clip.webptakumi/tests/fixtures/style_mix_blend_mode.rstakumi/tests/fixtures/style_overflow.rs
💤 Files with no reviewable changes (5)
- takumi-napi-core/src/export.ts
- takumi-js/src/index.ts
- takumi-napi-core/src/lib.rs
- takumi-napi-core/src/helper.rs
- takumi-wasm/src/helper.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@takumi/src/rendering/stacking_context.rs`:
- Around line 604-610: The node-local mask (pushed via Canvas::begin_subcanvas)
is being popped after canvas.composite_subcanvas, which restores the parent mask
stack; move the conditional pop so that when has_constrain is true you call
canvas.pop_mask() before calling canvas.composite_subcanvas(isolated_canvas,
node.context.style.mix_blend_mode) so the mask for this node is removed from the
subcanvas stack (not the parent) — update the block around isolated_canvas,
has_constrain, and the calls to composite_subcanvas and pop_mask accordingly.
- Around line 782-874: The bounds-hint currently uses only the transformed
layout size in compute_isolation_bounds and thus crops effects that can paint
outside the border box; update can_use_bounds_hint to return false when the
style contains any properties that can paint beyond the layout rect (so the
full-viewport fallback is used). Concretely: in can_use_bounds_hint, add checks
on the style for box-shadow (e.g. style.box_shadow / equivalent),
outline/outline_style, stroke/border that can draw outside, text-shadow or any
non-empty painted-decorations, and any background images with spread; if any
such property is present, return false (disable the hint); keep the existing
checks (filter, backdrop_filter, clip_path, mask_image, children clip) and
reference can_use_bounds_hint and compute_isolation_bounds when making the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 119a1e60-8218-4f1f-b757-cfc843c70fd0
📒 Files selected for processing (31)
docs/content/docs/load-images.mdxtakumi-helpers/src/utils.tstakumi-helpers/test/utils/extract-resource-urls.test.tstakumi-js/src/render.tstakumi/benches/effects.rstakumi/benches/fixtures.rstakumi/benches/gradient.rstakumi/src/error.rstakumi/src/layout/style/properties/radial_gradient.rstakumi/src/rendering/canvas.rstakumi/src/rendering/canvas/buffer_pool.rstakumi/src/rendering/components/blur.rstakumi/src/rendering/render.rstakumi/src/rendering/stacking_context.rstakumi/tests/fixtures-generated/animation_bouncing_text_lossy.webptakumi/tests/fixtures-generated/inline_vertical_align_multiline.webptakumi/tests/fixtures-generated/style_backdrop_filter.webptakumi/tests/fixtures-generated/style_backdrop_filter_frosted_glass.webptakumi/tests/fixtures-generated/style_background_image_radial_mixed.webptakumi/tests/fixtures-generated/style_background_repeat_space.webptakumi/tests/fixtures-generated/style_background_repeat_tile_from_top_left.webptakumi/tests/fixtures-generated/style_background_size_percent_20_20.webptakumi/tests/fixtures-generated/style_mask_image_diagonal_gradient.webptakumi/tests/fixtures-generated/style_mask_image_multiple_gradients.webptakumi/tests/fixtures-generated/style_mask_image_radial_ellipse.webptakumi/tests/fixtures-generated/style_mask_image_radial_gradient.webptakumi/tests/fixtures-generated/style_mask_image_with_background.webptakumi/tests/fixtures-generated/style_overflow_clip_visible_image.webptakumi/tests/fixtures-generated/text_shadow.webptakumi/tests/fixtures-generated/text_shadow_no_blur_radius.webptakumi/tests/fixtures/style_overflow.rs
✅ Files skipped from review due to trivial changes (2)
- takumi-helpers/test/utils/extract-resource-urls.test.ts
- takumi/benches/fixtures.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/content/docs/load-images.mdx
- takumi-helpers/src/utils.ts
- takumi/tests/fixtures/style_overflow.rs
- takumi/src/rendering/canvas/buffer_pool.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@takumi/src/rendering/stacking_context.rs`:
- Around line 548-561: The emptiness check uses raw floor/ceil values before
clamping, so rectangles entirely off-screen (negative) can pass the check but
collapse to zero-sized SceneBounds after clamping; fix by computing clamped
coordinates first (clamp left/top to >=0 and right/bottom to >=0 or compute
clamped_left = left.max(0), etc.) and then perform the emptiness check against
the clamped values before returning Some(SceneBounds) in the function that
builds SceneBounds (the block using left, top, right, bottom and returning
Some(SceneBounds)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c7756c32-518e-4573-b7fd-692940bc65a4
📒 Files selected for processing (1)
takumi/src/rendering/stacking_context.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Documentation