Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't segment using clips with different positioning nodes #2859

Merged
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Don't segment using clips with different positioning nodes

When segmenting brushes, don't consider clips if they have a different
positioning node. The issue here is that since they have a different
positioning node, the segmentation results may change during scrolling.
This means that we would need to invalidate the cache result and
re-segment. We can do that in the future, but in the meantime this fixes
the rendering issues the bug causes.

Unfortunately, this bug is triggered by rendering, scrolling, and then
re-rendering, so it is tricky to make a test for it.

Original bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1446626
  • Loading branch information
mrobinson committed Jul 10, 2018
commit f8140f0e33523be0908dbf3b4317b89880fc61a4
@@ -257,19 +257,39 @@ impl ClipSource {
}
}
}

pub fn is_rect(&self) -> bool {
match *self {
ClipSource::Rectangle(..) => true,
_ => false,
}
}

pub fn is_image_or_line_decoration_clip(&self) -> bool {
match *self {
ClipSource::Image(..) | ClipSource::LineDecoration(..) => true,
_ => false,
}
}
}

#[derive(Debug)]
pub struct ClipSources {
pub clips: Vec<(ClipSource, GpuCacheHandle)>,
pub local_inner_rect: LayoutRect,
pub local_outer_rect: Option<LayoutRect>
pub local_outer_rect: Option<LayoutRect>,
pub only_rectangular_clips: bool,
pub has_image_or_line_decoration_clip: bool,
}

impl ClipSources {
pub fn new(clips: Vec<ClipSource>) -> Self {
let (local_inner_rect, local_outer_rect) = Self::calculate_inner_and_outer_rects(&clips);

let has_image_or_line_decoration_clip =
clips.iter().any(|clip| clip.is_image_or_line_decoration_clip());
let only_rectangular_clips =
!has_image_or_line_decoration_clip && clips.iter().all(|clip| clip.is_rect());
let clips = clips
.into_iter()
.map(|clip| (clip, GpuCacheHandle::new()))
@@ -279,6 +299,8 @@ impl ClipSources {
clips,
local_inner_rect,
local_outer_rect,
only_rectangular_clips,
has_image_or_line_decoration_clip,
}
}

@@ -2047,19 +2047,39 @@ impl PrimitiveStore {
}

let local_clips = frame_state.clip_store.get_opt(&clip_item.clip_sources).expect("bug");
rect_clips_only = rect_clips_only && local_clips.only_rectangular_clips;

// TODO(gw): We can easily extend the segment builder to support these clip sources in
// the future, but they are rarely used.
// We must do this check here in case we continue early below.
if local_clips.has_image_or_line_decoration_clip {
clip_mask_kind = BrushClipMaskKind::Global;
}

// If this clip item is positioned by another positioning node, its relative position
// could change during scrolling. This means that we would need to resegment. Instead
// of doing that, only segment with clips that have the same positioning node.
// TODO(mrobinson, #2858): It may make sense to include these nodes, resegmenting only
// when necessary while scrolling.
if clip_item.transform_index != prim_run_context.transform_index {
// We don't need to generate a global clip mask for rectangle clips because we are
// in the same coordinate system and rectangular clips are handled by the local
// clip chain rectangle.
if !local_clips.only_rectangular_clips {
clip_mask_kind = BrushClipMaskKind::Global;
}
continue;
}

for &(ref clip, _) in &local_clips.clips {
let (local_clip_rect, radius, mode) = match *clip {
ClipSource::RoundedRectangle(rect, radii, clip_mode) => {
rect_clips_only = false;

(rect, Some(radii), clip_mode)
}
ClipSource::Rectangle(rect, mode) => {
(rect, None, mode)
}
ClipSource::BoxShadow(ref info) => {
rect_clips_only = false;

// For inset box shadows, we can clip out any
// pixels that are inside the shadow region
// and are beyond the inner rect, as they can't
@@ -2084,16 +2104,7 @@ impl PrimitiveStore {

continue;
}
ClipSource::LineDecoration(..) |
ClipSource::Image(..) => {
rect_clips_only = false;

// TODO(gw): We can easily extend the segment builder
// to support these clip sources in the
// future, but they are rarely used.
clip_mask_kind = BrushClipMaskKind::Global;
continue;
}
ClipSource::LineDecoration(..) | ClipSource::Image(..) => continue,
};

// If the scroll node transforms are different between the clip
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.