From 4aac02123d988628507b7270e849aecc5ec1f3cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 17 Sep 2018 18:07:43 +0200 Subject: [PATCH] border: Don't always share instances across borders if the instances depend on the available border size. I've added a big comment to BorderRenderTaskInfo with my current understanding of the invariants this code is trying to preserve. Please sanity-check it, but I think this patch should both be sound and also prevent the scrolling-franzine issue (https://bugzilla.mozilla.org/show_bug.cgi?id=1489644). Fixes #3058 --- webrender/src/border.rs | 115 ++++++++++++++++-- webrender/src/prim_store.rs | 19 ++- .../border/border-dashed-dotted-caching.png | Bin 0 -> 6778 bytes .../border/border-dashed-dotted-caching.yaml | 76 ++++++++++++ wrench/reftests/border/reftest.list | 1 + 5 files changed, 197 insertions(+), 14 deletions(-) create mode 100644 wrench/reftests/border/border-dashed-dotted-caching.png create mode 100644 wrench/reftests/border/border-dashed-dotted-caching.yaml diff --git a/webrender/src/border.rs b/webrender/src/border.rs index 11360a6581..e52dd68879 100644 --- a/webrender/src/border.rs +++ b/webrender/src/border.rs @@ -125,7 +125,7 @@ impl BorderCacheKey { bottom: border.bottom.into(), widths: (*widths).into(), radius: border.radius.into(), - scale: Au::from_f32_px(0.0), + scale: Au(0), } } } @@ -528,10 +528,63 @@ pub struct BorderSegmentInfo { widths: DeviceSize, } +bitflags! { + /// Whether we depend on the available size for the border (effectively in + /// the local rect of the primitive), and in which direction. + /// + /// Note that this relies on the corners being only dependent on the border + /// widths and radius. + /// + /// This is not just a single boolean to allow instance caching for border + /// boxes where one of the directions differ but not the one on the affected + /// border is. + /// + /// This allows sharing instances for stuff like paragraphs of different + /// heights separated by horizontal borders. + pub struct AvailableSizeDependence : u8 { + /// There's a dependence on the vertical direction, that is, at least + /// one of the right or left edges is dashed or dotted. + const VERTICAL = 1 << 0; + /// Same but for the horizontal direction. + const HORIZONTAL = 1 << 1; + } +} + +/// This is the data that describes a single border with (up to) four sides and +/// four corners. +/// +/// This object gets created for each border primitive at least once. Note, +/// however, that the instances this produces via `build_instances()` can and +/// will be shared by multiple borders, as long as they share the same cache +/// key. +/// +/// Segments, however, also get build once per primitive. +/// +/// So the important invariant to preserve when going through this code is that +/// the result of `build_instances()` would remain invariant for a given cache +/// key. +/// +/// That means, then, that `border_segments` can't depend at all on something +/// that isn't on the key like the available_size, while the brush segments can +/// (and will, since we skip painting empty segments caused by things like edges +/// getting constrained by huge border-radius). +/// +/// Note that the cache key is not only `BorderCacheKey`, but also a +/// `size` from `RenderTaskCacheKey`, which will always be zero unless +/// `available_size_dependence` is non-empty, which is effectively just dashed +/// and dotted borders for now, since the spacing between the dash and dots +/// changes depending on that size. #[derive(Debug)] pub struct BorderRenderTaskInfo { pub border_segments: Vec, pub size: DeviceIntSize, + pub available_size_dependence: AvailableSizeDependence, +} + +#[derive(PartialEq, Eq)] +enum DependsOnAvailableSize { + No, + Yes, } /// Information needed to place and draw a border edge. @@ -543,6 +596,8 @@ struct EdgeInfo { local_size: f32, /// Size in device pixels needed in the render task. device_size: f32, + /// Whether this edge depends on the available size. + depends_on_available_size: bool, } impl EdgeInfo { @@ -550,11 +605,13 @@ impl EdgeInfo { local_offset: f32, local_size: f32, device_size: f32, - ) -> EdgeInfo { - EdgeInfo { + depends_on_avail_size: DependsOnAvailableSize, + ) -> Self { + Self { local_offset, local_size, device_size, + depends_on_available_size: depends_on_avail_size == DependsOnAvailableSize::Yes, } } } @@ -594,7 +651,7 @@ fn get_edge_info( ) -> EdgeInfo { // To avoid division by zero below. if side_width <= 0.0 { - return EdgeInfo::new(0.0, 0.0, 0.0); + return EdgeInfo::new(0.0, 0.0, 0.0, DependsOnAvailableSize::No); } match style { @@ -603,12 +660,12 @@ fn get_edge_info( let (half_dash, _num_half_dashes) = compute_half_dash(side_width, avail_size); let device_size = (2.0 * 2.0 * half_dash * scale).round(); - EdgeInfo::new(0., avail_size, device_size) + EdgeInfo::new(0., avail_size, device_size, DependsOnAvailableSize::Yes) } BorderStyle::Dotted => { let dot_and_space_size = 2.0 * side_width; if avail_size < dot_and_space_size * 0.75 { - return EdgeInfo::new(0.0, 0.0, 0.0); + return EdgeInfo::new(0.0, 0.0, 0.0, DependsOnAvailableSize::Yes); } let approx_dot_count = avail_size / dot_and_space_size; let dot_count = approx_dot_count.floor().max(1.0); @@ -616,10 +673,10 @@ fn get_edge_info( let extra_space = avail_size - used_size; let device_size = dot_and_space_size * scale; let offset = (extra_space * 0.5).round(); - EdgeInfo::new(offset, used_size, device_size) + EdgeInfo::new(offset, used_size, device_size, DependsOnAvailableSize::Yes) } _ => { - EdgeInfo::new(0.0, avail_size, 8.0) + EdgeInfo::new(0.0, avail_size, 8.0, DependsOnAvailableSize::No) } } } @@ -704,6 +761,7 @@ impl BorderRenderTaskInfo { rect.size.height - local_size_tr.height - local_size_br.height, scale.0, ); + let inner_height = left_edge_info.device_size.max(right_edge_info.device_size).ceil(); let size = DeviceSize::new( @@ -715,6 +773,19 @@ impl BorderRenderTaskInfo { return None; } + let mut size_dependence = AvailableSizeDependence::empty(); + if top_edge_info.depends_on_available_size || + bottom_edge_info.depends_on_available_size + { + size_dependence.insert(AvailableSizeDependence::HORIZONTAL); + } + + if left_edge_info.depends_on_available_size || + right_edge_info.depends_on_available_size + { + size_dependence.insert(AvailableSizeDependence::VERTICAL); + } + add_edge_segment( LayoutRect::from_floats( rect.origin.x, @@ -894,9 +965,33 @@ impl BorderRenderTaskInfo { Some(BorderRenderTaskInfo { border_segments, size: size.to_i32(), + available_size_dependence: size_dependence, }) } + /// Returns the cache key size for this task, based on our available size + /// dependence computed earlier. + #[inline] + pub fn cache_key_size( + &self, + local_size: &LayoutSize, + scale: LayoutToDeviceScale, + ) -> DeviceIntSize { + let mut size = DeviceIntSize::zero(); + if self.available_size_dependence.is_empty() { + return size; + } + + let device_size = (*local_size * scale).to_i32(); + if self.available_size_dependence.contains(AvailableSizeDependence::VERTICAL) { + size.height = device_size.height; + } + if self.available_size_dependence.contains(AvailableSizeDependence::HORIZONTAL) { + size.width = device_size.width; + } + size + } + pub fn build_instances(&self, border: &NormalBorder) -> Vec { let mut instances = Vec::new(); @@ -974,6 +1069,10 @@ fn add_brush_segment( edge_flags: EdgeAaSegmentMask, brush_segments: &mut Vec, ) { + if image_rect.size.width <= 0. || image_rect.size.width <= 0. { + return; + } + brush_segments.push( BrushSegment::new( image_rect, diff --git a/webrender/src/prim_store.rs b/webrender/src/prim_store.rs index 651b03d243..2b49b85bb3 100644 --- a/webrender/src/prim_store.rs +++ b/webrender/src/prim_store.rs @@ -522,14 +522,14 @@ pub struct BrushSegment { impl BrushSegment { pub fn new( - rect: LayoutRect, + local_rect: LayoutRect, may_need_clip_mask: bool, edge_flags: EdgeAaSegmentMask, extra_data: [f32; 4], brush_flags: BrushFlags, - ) -> BrushSegment { - BrushSegment { - local_rect: rect, + ) -> Self { + Self { + local_rect, clip_task_id: BrushSegmentTaskId::Opaque, may_need_clip_mask, edge_flags, @@ -2851,14 +2851,21 @@ impl Primitive { let max_scale = BorderRenderTaskInfo::get_max_scale(&border.radius, &widths); scale.0 = scale.0.min(max_scale.0); let scale_au = Au::from_f32_px(scale.0); + + // NOTE(emilio): This `needs_update` relies on the local rect for a + // given primitive being immutable. If that changes, this code + // should probably handle changes to it as well, retaining the old + // size in cache_key. let needs_update = scale_au != cache_key.scale; + let mut new_segments = Vec::new(); + let local_rect = &self.metadata.local_rect; if needs_update { cache_key.scale = scale_au; *task_info = BorderRenderTaskInfo::new( - &self.metadata.local_rect, + local_rect, border, widths, scale, @@ -2869,7 +2876,7 @@ impl Primitive { *handle = task_info.as_ref().map(|task_info| { frame_state.resource_cache.request_render_task( RenderTaskCacheKey { - size: DeviceIntSize::zero(), + size: task_info.cache_key_size(&local_rect.size, scale), kind: RenderTaskCacheKeyKind::Border(cache_key.clone()), }, frame_state.gpu_cache, diff --git a/wrench/reftests/border/border-dashed-dotted-caching.png b/wrench/reftests/border/border-dashed-dotted-caching.png new file mode 100644 index 0000000000000000000000000000000000000000..792b17b7641bda3134716c80f7713d475a9b0eea GIT binary patch literal 6778 zcmeHMdtB1z8vkio&CRsdYH5?KSy`D&^V0H?MyFB>FKw2nY%?t-#j;cs#a+C#PMMd1 zw`!|-In$tJiVhHGt0Xkb8zqW5LFY_KP!Unt`vWuEr(EqEKWFFi2cHkT_vd-O-|zFh zFQ;7g?b6cJ)r25OYtL?^8w9B%!DrEed7!1>@+Ex;(zV}%wBH|*-Pd~Gl~=7huYHKw zBQU90vUqg2=AO|I%}>?qUTcvTZZ)eoumRl^$UV>|X+c&;U}_Ev8l5VxZawrGwDP^U z1<(SNfjU&_o(RQ0Ua|^WY5jsGTx#8*!{AVI7UTGL)LsZG&+@|Na=)S`?FTc9tBU<2 zy=9}j!Q=y{XRpK4+sGy`&}jV?4QOQ3@tlkgJsgwV!2afsZ+ok~@cfLL2FsgdePNIk zs?ZfUnq8r!>F4OELD+K}Y6gE04<);E?%n5}grK&)6!ZGtJKSCCkx+%Fz|)LLAw+-c zFc&J^IZGWQofK$$gyteh=K|Q~i#%|u4kP5ht|`bZ5R=$^@tdQmd2|iQ@O@uCx@O*X zjEqzl!@&{bujyM>FaB?6HVxuj99~zd?aoQ-)n6G;K&O`oQBg};=Ep6FrH&o*m;u*j zLuYQ*>uX4VL*RJw z2+VG^pR0HtJmcJ;QI}pVS(=RGSMGNusGKi>jtTk-`j*yPMCT916YdZ=rft`nEHEN` ztd)4UG3<0emTg1i4qGrBoC}*pm0E9KftLr;Z(iSv2xJ;u1jDOog~c`8DE*aZ8PUb9 zp8zw8`R7nCYes!ZdO(tTDZgQrA41)e&AWHq*uVKQL;*ynbtTc&U(EKbf8Y+CA6^&R zi|5&dVDWmHB{!X;mQ)NaS=HVowrEhEealH-?=SyC*3Np+W@Pfq<*^w@Sr`>>K=!3@ zEby87)x~dwHg4vcR-0o)8DHm&r38fUp*d~p{a$TF26lPZcfJ(AbZj}_%-%_6Sl^!# zO!jK>onvJRCVX()i}N0rz&!jJN$ix{YS_A|tnl`{kV9ZX!Pzp>#}t6&xyC}WGu`P2 z@8buPwA!n{1dn$@jc<4X{J-XT$0Q?+e1a~xrmt6#3{QB!%QL( zVP<_P7lzkC{jCkA!t#nt0Vk%}(5BB7YS|W-m^foJa6ng6fsj@(B_&a-?vTf?J4FKI zmxgODS|sF@E4^~`*VCn>f2)0eiNLWD;TkVdfz62AnM*e}xkJOm8-7{Vt;XpzZOQP* zeA{Z??aXdw<56s~-Oj#?mBWYE^&V3r{lC|4XZ?jrpxF>J8=EXhH{4Xsy!Zi(LVzj_bSBASbuP(JEA^Jnt$f8FGD)PG&enSKkdr zbUH1`yf&)fVbdz&m(6x@eYGx4o$4Dm|F8E}&N3fFmvzR+v+6``M&oj_UDbr#BnQKC z8y#3p46(2AgksTACdk`jaW`rrzrDLwK|a6P9IFPEe_j(qw6eAlUN#$+ixtBw6o-%W zgVCZ-ig@oVG%=C=^7qzFIo;iake>%O=aj)SPS=D*SuvPkMhP#?2P66MR6;^p7X`k& zDEZoEItQwh>}Uk_K0izw`a7GU3FnU~-)#)YfWXB)zVs*ialVI8(qbIQV+S z=+P41jgCHJ#QE=Aa|cH=wf9+Df4e0-=oG^M->Eu;t!6UA6?<1XdW4km^!qn~_{&Y% z9z{!IhXzWb@Q1H#Rcs0%y5@KQ z>H|ptAq=(Z@zrE=tdk%*CH&xCAApr6zv)nUOEOTw6+O4mmc0HI>@XUc+qI(;f90vZw( zxr6d>N5zCt`%o z7_nv3o%wlDwIJ{=*k^p(JWxD#rqUwkA0svAFkzM?1tFm2Ftx%uoMdxI29$0?7K z+R^^vzp6BE;U>W-wVghrIV@ij%9WkasT%qn()S{(jGqSgNEiu(u5oJ?m(1S zCd9a~vB2Pd6};GbKX75fIz3;uen03aE#U?a4VIzr?HMu$+7_`p;P)%JBnr-?SiXz% zg25kas3hhzuh{K3#-&Yhcq4cy5S}*_r7}25PB%oVV0rJX-ra0W;)lS8I4bzuVWEqpFd(L+(w5sD_D`~ldOeZl4`ky1k|U$(Vw<$ zg^v$VOS-pXZLO^Rr%qqTNx|F)15O-&3&?IIvw6k!=sAYqE+yZKi59llBvgTm-HI)sKGVeoPW$Ff1fu+4pvFjRIP*$5j|@ccr;I2nL8 z8%Cx;>x{6mFfJ-bhplIbQ|=(mgyZxTF5~wti0?=$O%TWTo5QPbn4jI zvXN{|#Rrp^hIv~L*jdSxy=v}x#rzMPJ~6SkM=DC280R1kDaOflRE*L1NU~k&*DBpV z2XYvIcIP%=x9}QBV+04q7#s1{*qBU#s(328Yw=_lAJ;@_9z~>p>Dbm9eDGm6Wx5a9 zdeZX%w5F|p3~s?0`92WBjNqb~R9G1Z@vZ1QKrH9Hr?C;=V(aO03J!8kl>4nwY0I>5 z*UO&bd7U?)F+58cL%DF%lkw108-Ot1W;UWgjA)rY3gHoej56p`aCAzlfMqG zRNzcFsXw{km10Usro_-$qO8BP21mZQD~m%-6BB?w<+kMLSuQ;O@Q<0NfqTX3M?{n# Xu9dZ#j0J%Il%PF3_aP|`AAR;4JE^By literal 0 HcmV?d00001 diff --git a/wrench/reftests/border/border-dashed-dotted-caching.yaml b/wrench/reftests/border/border-dashed-dotted-caching.yaml new file mode 100644 index 0000000000..ca224852fd --- /dev/null +++ b/wrench/reftests/border/border-dashed-dotted-caching.yaml @@ -0,0 +1,76 @@ +--- +root: + items: + - type: stacking-context + bounds: [0, 0, 500, 500] + items: + + - type: border + bounds: [ 0, 10, 25, 30 ] + width: [ 10 ] + border-type: normal + style: [ dashed ] + color: [ blue ] + + - type: border + bounds: [ 0, 60, 200, 30 ] + width: [ 10 ] + border-type: normal + style: [ dashed ] + color: [ blue ] + + - type: border + bounds: [ 0, 110, 300, 30 ] + width: [ 10 ] + border-type: normal + style: [ dashed ] + color: [ blue ] + + - type: border + bounds: [ 0, 160, 400, 30 ] + width: [ 10 ] + border-type: normal + style: [ dashed ] + color: [ blue ] + + - type: border + bounds: [ 0, 210, 500, 30 ] + width: [ 10 ] + border-type: normal + style: [ dashed ] + color: [ blue ] + + - type: border + bounds: [ 0, 260, 25, 30 ] + width: [ 10 ] + border-type: normal + style: [ dotted ] + color: [ blue ] + + - type: border + bounds: [ 0, 310, 200, 30 ] + width: [ 10 ] + border-type: normal + style: [ dotted ] + color: [ blue ] + + - type: border + bounds: [ 0, 360, 300, 30 ] + width: [ 10 ] + border-type: normal + style: [ dotted ] + color: [ blue ] + + - type: border + bounds: [ 0, 410, 400, 30 ] + width: [ 10 ] + border-type: normal + style: [ dotted ] + color: [ blue ] + + - type: border + bounds: [ 0, 460, 500, 30 ] + width: [ 10 ] + border-type: normal + style: [ dotted ] + color: [ blue ] diff --git a/wrench/reftests/border/reftest.list b/wrench/reftests/border/reftest.list index 4e92705815..ce08a5dc30 100644 --- a/wrench/reftests/border/reftest.list +++ b/wrench/reftests/border/reftest.list @@ -23,3 +23,4 @@ platform(linux,mac) == dotted-corner-small-radius.yaml dotted-corner-small-radiu == zero-width.yaml blank.yaml platform(linux,mac) == small-dotted-border.yaml small-dotted-border.png == discontinued-dash.yaml discontinued-dash.png +== border-dashed-dotted-caching.yaml border-dashed-dotted-caching.png