From 5f0aee15c5928878c507398bf1bb0701ef8e6be2 Mon Sep 17 00:00:00 2001 From: Glenn Watson Date: Tue, 28 Nov 2017 15:30:27 +1000 Subject: [PATCH] Fix some redundant cases of clip mask generation. There are two fixes here: * Image pictures were generating clip masks, but they are never actually used. In the future, we *might* want to support this, but at the moment it just results in clip masks that are then never used. * If a primitive is in the root coordinate system, then we know that it is axis-aligned, and that there are no coordinate system changes in between this primitive and the root. In that case, we can infer that the local_clip_rect of the clip scroll node will correctly handle the screen space clip rect. In Gecko with the current WR code, I was seeing a lot of very large clip masks being generated (4x extra A8 render targets) on HN. The test case included here is a very reduced test case for these issues. The test case makes use of the new reftest annotations to assert that no alpha targets get allocated while rendering the yaml file. --- webrender/src/clip_scroll_tree.rs | 4 ++ webrender/src/prim_store.rs | 36 ++++++++++++------ wrench/reftests/performance/no-clip-mask.png | Bin 0 -> 11979 bytes wrench/reftests/performance/no-clip-mask.yaml | 23 +++++++++++ wrench/reftests/performance/reftest.list | 1 + wrench/reftests/reftest.list | 1 + 6 files changed, 54 insertions(+), 11 deletions(-) create mode 100644 wrench/reftests/performance/no-clip-mask.png create mode 100644 wrench/reftests/performance/no-clip-mask.yaml create mode 100644 wrench/reftests/performance/reftest.list diff --git a/webrender/src/clip_scroll_tree.rs b/webrender/src/clip_scroll_tree.rs index 23bd924c7d..e52c3013a4 100644 --- a/webrender/src/clip_scroll_tree.rs +++ b/webrender/src/clip_scroll_tree.rs @@ -26,6 +26,10 @@ pub type ScrollStates = FastHashMap; pub struct CoordinateSystemId(pub u32); impl CoordinateSystemId { + pub fn root() -> CoordinateSystemId { + CoordinateSystemId(0) + } + pub fn next(&self) -> CoordinateSystemId { let CoordinateSystemId(id) = *self; CoordinateSystemId(id + 1) diff --git a/webrender/src/prim_store.rs b/webrender/src/prim_store.rs index eae36a93c1..0be4a00f30 100644 --- a/webrender/src/prim_store.rs +++ b/webrender/src/prim_store.rs @@ -9,7 +9,7 @@ use api::{ClipMode, LayerSize, LayerVector2D, LayerToWorldTransform, LineOrienta use api::{ClipAndScrollInfo, EdgeAaSegmentMask, PremultipliedColorF, TileOffset}; use api::{ClipId, LayerTransform, PipelineId, YuvColorSpace, YuvFormat}; use border::BorderCornerInstance; -use clip_scroll_tree::ClipScrollTree; +use clip_scroll_tree::{CoordinateSystemId, ClipScrollTree}; use clip::{ClipSourcesHandle, ClipStore}; use frame_builder::PrimitiveContext; use glyph_rasterizer::{FontInstance, FontTransform}; @@ -1322,6 +1322,14 @@ impl PrimitiveStore { .collect(); if clips.is_empty() { + // If this item is in the root coordinate system, then + // we know that the local_clip_rect in the clip node + // will take care of applying this clip, so no need + // for a mask. + if prim_coordinate_system_id == CoordinateSystemId::root() { + return true; + } + // If we have filtered all clips and the screen rect isn't any smaller, we can just // skip masking entirely. if combined_outer_rect == prim_screen_rect { @@ -1375,7 +1383,7 @@ impl PrimitiveStore { // Reset the visibility of this primitive. // Do some basic checks first, that can early out // without even knowing the local rect. - let (cpu_prim_index, dependencies, cull_children) = { + let (cpu_prim_index, dependencies, cull_children, may_need_clip_mask) = { let metadata = &mut self.cpu_metadata[prim_index.0]; metadata.screen_rect = None; @@ -1385,7 +1393,7 @@ impl PrimitiveStore { return None; } - let (dependencies, cull_children) = match metadata.prim_kind { + let (dependencies, cull_children, may_need_clip_mask) = match metadata.prim_kind { PrimitiveKind::Picture => { let pic = &mut self.cpu_pictures[metadata.cpu_prim_index.0]; @@ -1393,18 +1401,24 @@ impl PrimitiveStore { return None; } - let rfid = match pic.kind { - PictureKind::Image { reference_frame_id, .. } => Some(reference_frame_id), - _ => None, + let (rfid, may_need_clip_mask) = match pic.kind { + PictureKind::Image { reference_frame_id, .. } => { + (Some(reference_frame_id), false) + } + _ => { + (None, true) + } }; - (Some((pic.pipeline_id, mem::replace(&mut pic.runs, Vec::new()), rfid)), pic.cull_children) + (Some((pic.pipeline_id, mem::replace(&mut pic.runs, Vec::new()), rfid)), + pic.cull_children, + may_need_clip_mask) } _ => { - (None, true) + (None, true, true) } }; - (metadata.cpu_prim_index, dependencies, cull_children) + (metadata.cpu_prim_index, dependencies, cull_children, may_need_clip_mask) }; // If we have dependencies, we need to prepare them first, in order @@ -1476,7 +1490,7 @@ impl PrimitiveStore { (local_rect, xf_rect.bounding_rect) }; - if !self.update_clip_task( + if perform_culling && may_need_clip_mask && !self.update_clip_task( prim_index, prim_context, &unclipped_device_rect, @@ -1486,7 +1500,7 @@ impl PrimitiveStore { render_tasks, clip_store, parent_tasks, - ) && perform_culling { + ) { return None; } diff --git a/wrench/reftests/performance/no-clip-mask.png b/wrench/reftests/performance/no-clip-mask.png new file mode 100644 index 0000000000000000000000000000000000000000..9d06d151445a969b893f7b3ce0df694b5665f273 GIT binary patch literal 11979 zcmbVSe{dC76}~So>ElNe5@OQ|F@)eyM`?}4!edB63TX?DjXx@FFcjYuhmJPNs|Z9$ zo27Kdbf!h?IHes@_`}XHBQZ$_r4d8BjEtid$h6KtIz(BmBRWYj6p;sHi07R9cK7bS zv-jP&e{DARp8M@N-}%nDci;10Zho+M#+(^Kh~hP?o4zVU!V;n|IqepB^3OjVUnRud zKUvds-`Co^$KH>X!M_S?umA9Y;?C9=uXGo{7`UAJ*sb!ebft*PyXQ)6c_Zc*){6cQ z&z<}BhwuEV@!gO1J~?r1*^ZOFKTQ|J*1dM7<;mfehqwOV;ukv$aeP;)6_fL73RCjN zniji5-c_#@h^nJhi0m!=rJFH+PvdjKg*M-X)-=tQ_C#4ht-OO_FT5?^#$ZOnbeyG5 zj#{IAt`N~L0!BRa{WUx+&a(9*3Dsc6hJ?C^4$GqHqBM_-vRH-}$~c%7s7xju&UE!NB|smL{c#97%&s|U)wBrXkdtN*dSdb6zbtgMi)>$fVntqq>{{Z^ZB$>+&ndJbsxm-G-ee~b(LW-B!lDhWCWfXML9z)HX;dQ)Zl~u+Apl`*{S1+lvvqk@kKp;Q3Y&30NW4A6FUoEVs5x z7F2T{CI^R5_E*4DR>V|?;LOz1#NnZ>f4=sSam>5xjbFdb zilsYoF+r4|yAWV}CMC(n)=ytXPE=W)mlN+Dl5uk$7M`xaLdy>DXIe?IMMRd?nV zzcX}|J2y+9WCE$j2)G`a4C(M~?%UuLh5SbZt8VGpMtUQYt(9P?(TxI318MdCyYOBw z4l=sVrvly&)I3gxtPk!pr}6OZ1PN)p<*}%60YXr`I$Vf5a6+i(hVV{YYF9!N@nr_3L$WTsh;C=S>860I*8}rlLla;ezBShiDX!SQXR4As zRzhYCx>)B~7^#Pzi}zrmaI3UZ^w_p@MPwLZUB8A9Trz4G)8T^BLqk8^Uk|>xzVNvx4>qO{++dXa^HG*@S3y*jDJ><25U2a%8r# z6faufwiBFu1OcOInaOno-Nc!L;B{9U0(lrAJuM0IZemzEP|eWCXW4IGi~lRw#uILG z-)D(8CiJZU#q&K5^fhQQQ%{+A1km8H&62{#HUANmU?clIB~zDRdk+_WuEX%+_FdW5 z8%S_V6+l;HCY*2ZUO7#bvzTkxSRsFDqpnZl91Qygy!`k^3XZ)W#&NER$_%vI9XIE2 z=KNBe0$1jfuAJu)pMA&#Wh2N%NqVFNMk?U_jX4_NHXg}JeWg^F+WnYh@RZIclE$af z&i4VkM>Yb?o|(D(I;*_gy;a{i+@F931$G?ccSrdmY{mKj-&e04DqgGJNZWDwnzP#e zCnq_wv4brGT&%i=G9n4t#6k~r@W9;cJf|xB_L`L@Z^Vg_Pze1q_xw$33AGXmJ(*C+ zhz`I}qQXnn$p9;NE8~36#3xzT;XVOxgZpwJ(!n!98brY#FsJY~N#Wkn{X?5J9eW72 z;dE&a5=LH$6eFDMYnIni^-BdYI7hHXjVK(%+g&D45D2q_&&sjzskny?7_3GW=KEaC)OvT@+^I6)z1DCyvzSsrEpyD|<49CR^v zwq!3bFCur0%pvT?2f7n@DsE)IE)GX*QQHkJARSc((L}Vt=KXXDy+LHw;eVw!+wsGK z;!VcuP@Mgv(SZeArQ?{9dWoa0FU=9VjduA#OkWuYWjk&`IQM|U=Nvx6(Eb@5Ojg_` zCVDzEbXp1X;A-xOyC0;Z$_I@jM{Yn_;DVV$kwx$ZitqDEamyT|mX$i&-@!*fyn+Q< zK8yS`(St5!OIx@bXqii#W6vt%6`iy(2*=V{i;s6(7yo2arSjR@j)}W0ac3pR^t^s0vaw# zt<-~Xu+*j%y~n6SfZ%J(2fYR1O;R6c0^NZ##3`i*F4BEFCpn>@ z;y_SjnH}Q`@lO-7S-(Z;&_jPiJ;=3yX4?M8*NgQv97b>R7*+V2$(tsUko;T(_CWpc zTS2FS%QbeTOP3Gd{Hyvo@ltUL7K00Gbe^+>Bc9UhWrje_r|wPb|OE~#`DFc%Nx1TteTP0r>Fwtv^3uX^8Ku=2(k;q! z#+_U0oR5<&CA=Z=f`3bmb1r+gI7OM^byoNsuB;oZdyP5p`=dO2mET#B<)A*;GwGp$ zR$BNKMInqT5Uilhj4SUKFj;7d@-3= zo;@AxIF8%$w^~Q%+Tbg^qEsyjzorW4K;#t|TQOD`Q46O$V*RGeSmcC`U#uDQRtsdr`S+5Z8HTJ2o` literal 0 HcmV?d00001 diff --git a/wrench/reftests/performance/no-clip-mask.yaml b/wrench/reftests/performance/no-clip-mask.yaml new file mode 100644 index 0000000000..ecc2f2c1bf --- /dev/null +++ b/wrench/reftests/performance/no-clip-mask.yaml @@ -0,0 +1,23 @@ +# In this case, there is no incompatible transform. +# Therefore, the clip condition should be handled +# by the vertex shader via the local_clip_rect, and +# there should be no clip mask generated. +--- +root: + items: + - + type: "stacking-context" + items: + - + bounds: [0, 111, 1887, 1365] + "clip-rect": [0, 111, 1887, 1365] + type: iframe + id: [1, 2] +pipelines: + - + id: [1, 2] + items: + - + bounds: [1875, -1, 12, 999] + "clip-rect": [1875, -1, 12, 999] + image: checkerboard(4, 8, 8) diff --git a/wrench/reftests/performance/reftest.list b/wrench/reftests/performance/reftest.list new file mode 100644 index 0000000000..50bbbb25dc --- /dev/null +++ b/wrench/reftests/performance/reftest.list @@ -0,0 +1 @@ +== color_targets(1) alpha_targets(0) no-clip-mask.yaml no-clip-mask.png diff --git a/wrench/reftests/reftest.list b/wrench/reftests/reftest.list index c573c70887..9db9842235 100644 --- a/wrench/reftests/reftest.list +++ b/wrench/reftests/reftest.list @@ -8,6 +8,7 @@ include filters/reftest.list include gradient/reftest.list include image/reftest.list include mask/reftest.list +include performance/reftest.list include scrolling/reftest.list include snap/reftest.list include split/reftest.list