Skip to content

Commit 560790e

Browse files
ladyhavocladyhavoc
committed
Bug 1918529 - fix some subpixel misalignment issues with gfx.webrender.svg-filter-effects r=gfx-reviewers,gw
Differential Revision: https://phabricator.services.mozilla.com/D223453
1 parent 03aed52 commit 560790e

File tree

3 files changed

+50
-42
lines changed

3 files changed

+50
-42
lines changed

‎webrender/src/picture.rs

Lines changed: 45 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -4183,6 +4183,8 @@ struct SurfaceAllocInfo {
41834183
// Only used for SVGFEGraph currently, this is the source pixels needed to
41844184
// render the pixels in clipped.
41854185
source: DeviceRect,
4186+
// Only used for SVGFEGraph, this is the same as clipped before rounding.
4187+
clipped_notsnapped: DeviceRect,
41864188
clipped_local: PictureRect,
41874189
uv_rect_kind: UvRectKind,
41884190
}
@@ -6378,6 +6380,14 @@ impl PicturePrimitive {
63786380
)
63796381
);
63806382

6383+
// Determine the local space to device pixel scaling in the most robust
6384+
// way, this accounts for local to device transform and
6385+
// device_pixel_scale (if the task is shrunk in get_surface_rects).
6386+
let subregion_to_device_scale_x = surface_rects.clipped_notsnapped.width() / surface_rects.clipped_local.width();
6387+
let subregion_to_device_scale_y = surface_rects.clipped_notsnapped.height() / surface_rects.clipped_local.height();
6388+
let subregion_to_device_offset_x = surface_rects.clipped_notsnapped.min.x - (surface_rects.clipped_local.min.x * subregion_to_device_scale_x).floor();
6389+
let subregion_to_device_offset_y = surface_rects.clipped_notsnapped.min.y - (surface_rects.clipped_local.min.y * subregion_to_device_scale_y).floor();
6390+
63816391
// Produce the target pixels, this is the result of the
63826392
// composite op
63836393
let filter_task_id = request_render_task(
@@ -6396,8 +6406,10 @@ impl PicturePrimitive {
63966406
source_subregion.cast_unit(),
63976407
target_subregion.cast_unit(),
63986408
prim_subregion.cast_unit(),
6399-
surface_rects.clipped.cast_unit(),
6400-
surface_rects.clipped_local.cast_unit(),
6409+
subregion_to_device_scale_x,
6410+
subregion_to_device_scale_y,
6411+
subregion_to_device_offset_x,
6412+
subregion_to_device_offset_y,
64016413
)
64026414
}
64036415
);
@@ -8050,7 +8062,11 @@ fn get_surface_rects(
80508062
}
80518063
};
80528064

8053-
let (mut clipped, mut unclipped, mut source) = if surface.raster_spatial_node_index != surface.surface_spatial_node_index {
8065+
// clipped_local is in LayoutPixel coordinates (technically if the parent
8066+
// surface has a parent surface this would be PicturePixel), it needs to be
8067+
// in WorldPixel before we can properly transform it into DevicePixel for
8068+
// rasterizing.
8069+
let (clipped, unclipped, source) = if surface.raster_spatial_node_index != surface.surface_spatial_node_index {
80548070
assert_eq!(surface.device_pixel_scale.0, 1.0);
80558071

80568072
let local_to_world = SpaceMapper::new_with_target(
@@ -8060,52 +8076,55 @@ fn get_surface_rects(
80608076
spatial_tree,
80618077
);
80628078

8063-
let clipped = (local_to_world.map(&clipped_local.cast_unit()).unwrap() * surface.device_pixel_scale).round_out();
8064-
let unclipped = local_to_world.map(&unclipped_local).unwrap() * surface.device_pixel_scale;
8065-
let source = (local_to_world.map(&source_local.cast_unit()).unwrap() * surface.device_pixel_scale).round_out();
8079+
let clipped = local_to_world.map(&clipped_local.cast_unit()).unwrap();
8080+
let unclipped = local_to_world.map(&unclipped_local).unwrap();
8081+
let source = local_to_world.map(&source_local.cast_unit()).unwrap();
80668082

80678083
(clipped, unclipped, source)
80688084
} else {
8069-
let clipped = (clipped_local.cast_unit() * surface.device_pixel_scale).round_out();
8070-
let unclipped = unclipped_local.cast_unit() * surface.device_pixel_scale;
8071-
let source = (source_local.cast_unit() * surface.device_pixel_scale).round_out();
8085+
let clipped = clipped_local.cast_unit();
8086+
let unclipped = unclipped_local.cast_unit();
8087+
let source = source_local.cast_unit();
80728088

80738089
(clipped, unclipped, source)
80748090
};
80758091

80768092
// Limit rendering extremely large pictures to something the hardware can
80778093
// handle, considering both clipped (target subregion) and source subregion.
80788094
//
8095+
// Do this before we apply rounding because the scaling should not vary with
8096+
// subpixel scrolling.
8097+
//
8098+
// Use slightly less than max_surface_size for subpixel rounding, and also
8099+
// leave room for SVGFEGraph to add a 1 pixel inflate.
8100+
//
80798101
// If you change this, test with:
80808102
// ./mach crashtest layout/svg/crashtests/387290-1.svg
80818103
let max_dimension =
80828104
clipped.width().max(
80838105
clipped.height().max(
80848106
source.width().max(
80858107
source.height()
8086-
))).ceil();
8087-
if max_dimension > max_surface_size {
8088-
let max_dimension =
8089-
clipped_local.width().max(
8090-
clipped_local.height().max(
8091-
source_local.width().max(
8092-
source_local.height()
8093-
))).ceil();
8108+
))) * surface.device_pixel_scale.0;
8109+
let max_allowed_dimension = max_surface_size - 4.0;
8110+
if max_allowed_dimension < max_dimension {
80948111
surface.raster_spatial_node_index = surface.surface_spatial_node_index;
8095-
surface.device_pixel_scale = Scale::new(max_surface_size / max_dimension);
8112+
surface.device_pixel_scale = Scale::new(surface.device_pixel_scale.0 * max_allowed_dimension / max_dimension);
80968113
surface.local_scale = (1.0, 1.0);
8097-
8098-
clipped = (clipped_local.cast_unit() * surface.device_pixel_scale).round();
8099-
unclipped = unclipped_local.cast_unit() * surface.device_pixel_scale;
8100-
source = (source_local.cast_unit() * surface.device_pixel_scale).round();
81018114
}
81028115

8103-
let task_size = clipped.size().to_i32();
8116+
// After deciding if downscaling is necessary, apply scaling and round.
8117+
let clipped = clipped * surface.device_pixel_scale;
8118+
let unclipped = unclipped * surface.device_pixel_scale;
8119+
let source = (source * surface.device_pixel_scale).round_out();
8120+
let clipped_snapped = clipped.round_out();
8121+
8122+
let task_size = clipped_snapped.size().to_i32();
81048123
debug_assert!(task_size.width <= max_surface_size as i32);
81058124
debug_assert!(task_size.height <= max_surface_size as i32);
81068125

81078126
let uv_rect_kind = calculate_uv_rect_kind(
8108-
clipped,
8127+
clipped_snapped,
81098128
unclipped,
81108129
);
81118130

@@ -8125,9 +8144,10 @@ fn get_surface_rects(
81258144
Some(SurfaceAllocInfo {
81268145
task_size,
81278146
needs_scissor_rect,
8128-
clipped,
8147+
clipped: clipped_snapped,
81298148
unclipped,
81308149
source,
8150+
clipped_notsnapped: clipped,
81318151
clipped_local,
81328152
uv_rect_kind,
81338153
})

‎webrender/src/render_task.rs

Lines changed: 4 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1649,8 +1649,10 @@ impl RenderTask {
16491649
source_subregion: LayoutRect,
16501650
target_subregion: LayoutRect,
16511651
prim_subregion: LayoutRect,
1652-
surface_rects_clipped: LayoutRect,
1653-
surface_rects_clipped_local: LayoutRect,
1652+
subregion_to_device_scale_x: f32,
1653+
subregion_to_device_scale_y: f32,
1654+
subregion_to_device_offset_x: f32,
1655+
subregion_to_device_offset_y: f32,
16541656
) -> RenderTaskId {
16551657
const BUFFER_LIMIT: usize = SVGFE_GRAPH_MAX;
16561658
let mut task_by_buffer_id: [RenderTaskId; BUFFER_LIMIT] = [RenderTaskId::INVALID; BUFFER_LIMIT];
@@ -1699,20 +1701,6 @@ impl RenderTask {
16991701
}
17001702
}
17011703

1702-
// Determine the local space to device pixel scaling in the most robust
1703-
// way, this accounts for local to device transform and
1704-
// device_pixel_scale (if the task is shrunk in get_surface_rects).
1705-
//
1706-
// This has some precision issues because surface_rects_clipped was
1707-
// rounded already, so it's not exactly the same transform that
1708-
// get_surface_rects performed, but it is very close, since it is not
1709-
// quite the same we have to round the offset a certain way to avoid
1710-
// introducing subpixel offsets caused by the slight deviation.
1711-
let subregion_to_device_scale_x = surface_rects_clipped.width() / surface_rects_clipped_local.width();
1712-
let subregion_to_device_scale_y = surface_rects_clipped.height() / surface_rects_clipped_local.height();
1713-
let subregion_to_device_offset_x = surface_rects_clipped.min.x - (surface_rects_clipped_local.min.x * subregion_to_device_scale_x).floor();
1714-
let subregion_to_device_offset_y = surface_rects_clipped.min.y - (surface_rects_clipped_local.min.y * subregion_to_device_scale_y).floor();
1715-
17161704
// Iterate the filter nodes and create tasks
17171705
let mut made_dependency_on_source = false;
17181706
for (filter_index, (filter_node, op)) in filter_nodes.iter().enumerate() {

‎wrench/reftests/transforms/reftest.list

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ fuzzy(1,10) == flatten-all-flat.yaml flatten-all-flat-ref.yaml
4444
== strange-w.yaml strange-w-ref.yaml
4545
== big-axis-aligned-scale.yaml big-axis-aligned-scale-ref.yaml
4646
# Compare ~8K raster root (>MAX_SURFACE_SIZE) with ~2K raster root. fuzzy due to lerping on edges.
47-
skip_on(android) fuzzy(56,804) == raster_root_A_8192.yaml raster_root_A_ref.yaml
47+
skip_on(android) fuzzy(59,804) == raster_root_A_8192.yaml raster_root_A_ref.yaml
4848
# Same as large-raster-root.yaml but resulting in a 10302×100 raster root (= >4096) vs 4000x100 in ref:
4949
skip_on(android) fuzzy(60,917) == raster_root_B_8192.yaml raster_root_B_ref.yaml
5050
# Make sure we don't panic

0 commit comments

Comments
 (0)