From 1d845ee4f27b604d172f9d7b6fac6ce8e2495199 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Fri, 5 Dec 2014 03:55:39 -0800 Subject: [PATCH] gfx: Update Azure and Skia, and rewrite broken clipping logic. This exposed some problems in our clipping logic, which was never properly rewritten for the stacking context reform. The clipping code worked in terms of a stack of clips, but the new stacking context code has no concept of a stack of clip regions. Fixing that in turn exposed some flaky/incorrect tests: * `borders` had an incorrect reference image, as far as I can tell. * `negative_margins` had some stray pixels, fixed by changing the text. --- components/gfx/display_list/mod.rs | 94 ++++++++++++++++------------- components/gfx/paint_context.rs | 16 +++-- components/gfx/paint_task.rs | 6 +- components/servo/Cargo.lock | 6 +- tests/ref/borders.png | Bin 2090 -> 8233 bytes tests/ref/negative_margins_a.html | 5 +- tests/ref/negative_margins_b.html | 5 +- 7 files changed, 71 insertions(+), 61 deletions(-) diff --git a/components/gfx/display_list/mod.rs b/components/gfx/display_list/mod.rs index 1cc152333da..ffbeda602f4 100644 --- a/components/gfx/display_list/mod.rs +++ b/components/gfx/display_list/mod.rs @@ -176,8 +176,8 @@ impl StackingContext { pub fn optimize_and_draw_into_context(&self, paint_context: &mut PaintContext, tile_bounds: &Rect, - current_transform: &Matrix2D, - current_clip_stack: &mut Vec>) { + transform: &Matrix2D, + clip_rect: Option<&Rect>) { let temporary_draw_target = paint_context.get_or_create_temporary_draw_target(self.opacity); { @@ -186,7 +186,7 @@ impl StackingContext { font_ctx: &mut *paint_context.font_ctx, page_rect: paint_context.page_rect, screen_rect: paint_context.screen_rect, - ..*paint_context + transient_clip_rect: None, }; // Optimize the display list to throw out out-of-bounds display items and so forth. @@ -201,11 +201,17 @@ impl StackingContext { positioned_children.as_slice_mut() .sort_by(|this, other| this.z_index.cmp(&other.z_index)); + // Set up our clip rect and transform. + match clip_rect { + None => {} + Some(clip_rect) => paint_subcontext.draw_push_clip(clip_rect), + } + let old_transform = paint_subcontext.draw_target.get_transform(); + paint_subcontext.draw_target.set_transform(transform); + // Steps 1 and 2: Borders and background for the root. for display_item in display_list.background_and_borders.iter() { - display_item.draw_into_context(&mut paint_subcontext, - current_transform, - current_clip_stack) + display_item.draw_into_context(&mut paint_subcontext) } // Step 3: Positioned descendants with negative z-indices. @@ -215,41 +221,39 @@ impl StackingContext { } if positioned_kid.layer.is_none() { let new_transform = - current_transform.translate(positioned_kid.bounds.origin.x.to_nearest_px() - as AzFloat, - positioned_kid.bounds.origin.y.to_nearest_px() - as AzFloat); + transform.translate(positioned_kid.bounds + .origin + .x + .to_nearest_px() as AzFloat, + positioned_kid.bounds + .origin + .y + .to_nearest_px() as AzFloat); let new_tile_rect = self.compute_tile_rect_for_child_stacking_context(tile_bounds, &**positioned_kid); positioned_kid.optimize_and_draw_into_context(&mut paint_subcontext, &new_tile_rect, &new_transform, - current_clip_stack); + Some(&positioned_kid.clip_rect)) } } // Step 4: Block backgrounds and borders. for display_item in display_list.block_backgrounds_and_borders.iter() { - display_item.draw_into_context(&mut paint_subcontext, - current_transform, - current_clip_stack) + display_item.draw_into_context(&mut paint_subcontext) } // Step 5: Floats. for display_item in display_list.floats.iter() { - display_item.draw_into_context(&mut paint_subcontext, - current_transform, - current_clip_stack) + display_item.draw_into_context(&mut paint_subcontext) } // TODO(pcwalton): Step 6: Inlines that generate stacking contexts. // Step 7: Content. for display_item in display_list.content.iter() { - display_item.draw_into_context(&mut paint_subcontext, - current_transform, - current_clip_stack) + display_item.draw_into_context(&mut paint_subcontext) } // Steps 8 and 9: Positioned descendants with nonnegative z-indices. @@ -260,25 +264,38 @@ impl StackingContext { if positioned_kid.layer.is_none() { let new_transform = - current_transform.translate(positioned_kid.bounds.origin.x.to_nearest_px() - as AzFloat, - positioned_kid.bounds.origin.y.to_nearest_px() - as AzFloat); + transform.translate(positioned_kid.bounds + .origin + .x + .to_nearest_px() as AzFloat, + positioned_kid.bounds + .origin + .y + .to_nearest_px() as AzFloat); let new_tile_rect = self.compute_tile_rect_for_child_stacking_context(tile_bounds, &**positioned_kid); positioned_kid.optimize_and_draw_into_context(&mut paint_subcontext, &new_tile_rect, &new_transform, - current_clip_stack); + Some(&positioned_kid.clip_rect)) } } // TODO(pcwalton): Step 10: Outlines. + + // Undo our clipping and transform. + if paint_subcontext.transient_clip_rect.is_some() { + paint_subcontext.draw_pop_clip(); + paint_subcontext.transient_clip_rect = None + } + paint_subcontext.draw_target.set_transform(&old_transform); + if clip_rect.is_some() { + paint_subcontext.draw_pop_clip() + } } - paint_context.draw_temporary_draw_target_if_necessary(&temporary_draw_target, - self.opacity) + paint_context.draw_temporary_draw_target_if_necessary(&temporary_draw_target, self.opacity) } /// Translate the given tile rect into the coordinate system of a child stacking context. @@ -560,24 +577,17 @@ impl<'a> Iterator<&'a DisplayItem> for DisplayItemIterator<'a> { } impl DisplayItem { - /// Paints this display item into the given paint context. - fn draw_into_context(&self, - paint_context: &mut PaintContext, - current_transform: &Matrix2D, - current_clip_stack: &mut Vec>) { - // TODO(pcwalton): This will need some tweaking to deal with more complex clipping regions. - let clip_rect = &self.base().clip_rect; - if current_clip_stack.len() == 0 || current_clip_stack.last().unwrap() != clip_rect { - while current_clip_stack.len() != 0 { + /// Paints this display item into the given painting context. + fn draw_into_context(&self, paint_context: &mut PaintContext) { + let this_clip_rect = self.base().clip_rect; + if paint_context.transient_clip_rect != Some(this_clip_rect) { + if paint_context.transient_clip_rect.is_some() { paint_context.draw_pop_clip(); - drop(current_clip_stack.pop()); } - paint_context.draw_push_clip(clip_rect); - current_clip_stack.push(*clip_rect); + paint_context.draw_push_clip(&this_clip_rect); + paint_context.transient_clip_rect = Some(this_clip_rect) } - paint_context.draw_target.set_transform(current_transform); - match *self { SolidColorDisplayItemClass(ref solid_color) => { paint_context.draw_solid_color(&solid_color.base.bounds, solid_color.color) @@ -585,7 +595,7 @@ impl DisplayItem { TextDisplayItemClass(ref text) => { debug!("Drawing text at {}.", text.base.bounds); - paint_context.draw_text(&**text, current_transform); + paint_context.draw_text(&**text); } ImageDisplayItemClass(ref image_item) => { diff --git a/components/gfx/paint_context.rs b/components/gfx/paint_context.rs index 4a5e9767d08..499cdf1397e 100644 --- a/components/gfx/paint_context.rs +++ b/components/gfx/paint_context.rs @@ -40,6 +40,10 @@ pub struct PaintContext<'a> { pub page_rect: Rect, /// The rectangle that this context encompasses in screen coordinates (pixels). pub screen_rect: Rect, + /// The current transient clipping rect, if any. A "transient clipping rect" is the clipping + /// rect used by the last display item. We cache the last value so that we avoid pushing and + /// popping clip rects unnecessarily. + pub transient_clip_rect: Option>, } enum Direction { @@ -130,11 +134,11 @@ impl<'a> PaintContext<'a> { size, stride as i32, source_format); - let source_rect = Rect(Point2D(0u as AzFloat, 0u as AzFloat), + let source_rect = Rect(Point2D(0.0, 0.0), Size2D(image.width as AzFloat, image.height as AzFloat)); let dest_rect = bounds.to_azure_rect(); let draw_surface_options = DrawSurfaceOptions::new(Linear, true); - let draw_options = DrawOptions::new(1.0f64 as AzFloat, 0); + let draw_options = DrawOptions::new(1.0, 0); draw_target_ref.draw_surface(azure_surface, dest_rect, source_rect, @@ -628,9 +632,9 @@ impl<'a> PaintContext<'a> { self.draw_border_path(&original_bounds, direction, border, radius, scaled_color); } - pub fn draw_text(&mut self, - text: &TextDisplayItem, - current_transform: &Matrix2D) { + pub fn draw_text(&mut self, text: &TextDisplayItem) { + let current_transform = self.draw_target.get_transform(); + // Optimization: Don’t set a transform matrix for upright text, and pass a start point to // `draw_text_into_context`. // @@ -669,7 +673,7 @@ impl<'a> PaintContext<'a> { // Undo the transform, only when we did one. if text.orientation != Upright { - self.draw_target.set_transform(current_transform) + self.draw_target.set_transform(¤t_transform) } } diff --git a/components/gfx/paint_task.rs b/components/gfx/paint_task.rs index a15e82a031e..03fefad0867 100644 --- a/components/gfx/paint_task.rs +++ b/components/gfx/paint_task.rs @@ -509,6 +509,7 @@ impl WorkerThread { font_ctx: &mut self.font_context, page_rect: tile.page_rect, screen_rect: tile.screen_rect, + transient_clip_rect: None, }; // Apply the translation to paint the tile we want. @@ -518,18 +519,15 @@ impl WorkerThread { let matrix = matrix.translate(-tile_bounds.origin.x as AzFloat, -tile_bounds.origin.y as AzFloat); - paint_context.draw_target.set_transform(&matrix); - // Clear the buffer. paint_context.clear(); // Draw the display list. profile(time::PaintingPerTileCategory, None, self.time_profiler_sender.clone(), || { - let mut clip_stack = Vec::new(); stacking_context.optimize_and_draw_into_context(&mut paint_context, &tile.page_rect, &matrix, - &mut clip_stack); + None); paint_context.draw_target.flush(); }); } diff --git a/components/servo/Cargo.lock b/components/servo/Cargo.lock index 206a6cebb95..afaf29f0e2f 100644 --- a/components/servo/Cargo.lock +++ b/components/servo/Cargo.lock @@ -25,7 +25,7 @@ dependencies = [ [[package]] name = "azure" version = "0.1.0" -source = "git+https://github.com/servo/rust-azure#d323c3c7c248d3d5a2d46a6a5ee61c6e92aec0b0" +source = "git+https://github.com/servo/rust-azure#452939480f5ef7640714f16c6a9e5d02e0e2d147" dependencies = [ "core_foundation 0.1.0 (git+https://github.com/servo/rust-core-foundation)", "core_graphics 0.1.0 (git+https://github.com/servo/rust-core-graphics)", @@ -34,7 +34,7 @@ dependencies = [ "freetype 0.1.0 (git+https://github.com/servo/rust-freetype)", "geom 0.1.0 (git+https://github.com/servo/rust-geom)", "layers 0.1.0 (git+https://github.com/servo/rust-layers)", - "skia-sys 0.0.20130412 (git+https://github.com/servo/skia)", + "skia-sys 0.0.20130412 (git+https://github.com/servo/skia?ref=upstream-2014-06-16)", "xlib 0.1.0 (git+https://github.com/servo/rust-xlib)", ] @@ -600,7 +600,7 @@ source = "git+https://github.com/rust-lang/semver#7dca047a9cd40e929a4545b37a1917 [[package]] name = "skia-sys" version = "0.0.20130412" -source = "git+https://github.com/servo/skia#79aa9354837bc195b83fa041b9632ea628e6f7d0" +source = "git+https://github.com/servo/skia?ref=upstream-2014-06-16#c3dd8cacbddbfc20b0dae9b456ac1545b0402cff" dependencies = [ "expat-sys 2.1.0 (git+https://github.com/servo/libexpat)", "freetype-sys 2.4.11 (git+https://github.com/servo/libfreetype2)", diff --git a/tests/ref/borders.png b/tests/ref/borders.png index bd9644b9553925f56f14d1ccea6867cf68ec0638..1bbfc373b5e2874a8f27c42ae326916bf16b379b 100644 GIT binary patch literal 8233 zcmeAS@N?(olHy`uVBq!ia0y~yU~^z#VA;UI1{7J$*ft$Vu_bxCyDx` z7I;J!Gca%qgD@k*tT_@uLG}_)Usv{fJgnS$41#lX7#SGkw|TlahE&{obN6Cyl%s&d z#qAfvCSP&36?=Q*>yCMztc~qD>@}t5%;jeM-8(Dg>=_xLDL|mO-v+1|2m+KFz%+v; z69|>sNvhHDVZv8D=89jCk|-vNFVEUBV_u8XvEK9iO$LGopXz?HK$r}?Oa>5^Lkj~Bge9QJ09EM1aR91J z5R0;n_y13Ps?7*d5z`=!Pc4x~4nAc^{%w_)kB5fL1J6oEe1Y`)`@f%`SfL>@uZ83g z0qOtI1PyZkL)-}vW&;N!D476(f)fKMc>zJkk4OKRD}E+Hcntohuqb}?uAZq%w*Tkh zXSs5ps;_Un^E&SPuIqc>m9DS8i<|j<()TL7zQmFgh>W+5X=!=?k64+1ng8!Wb^W{k z+7(-Y4#VRXB*%W&(Qkl;tP)EC#M@W{Mu1dh*Up?dD{paa<-*F@#rrSJv#tL2=H}+~ z^EeoPPs}4ZUWp8`?6hak_}7$Xz4$!U^7ryNe=PC4 -You see here a scroll labeled JUYED AWK YACC. -
Here lies the body of Jonathan Blake.
-
Stepped on the gas instead of the brake.
+
X
+
X
diff --git a/tests/ref/negative_margins_b.html b/tests/ref/negative_margins_b.html index 092ad0316d3..017b93a5600 100644 --- a/tests/ref/negative_margins_b.html +++ b/tests/ref/negative_margins_b.html @@ -1,7 +1,6 @@ -You see here a scroll labeled JUYED AWK YACC. -
Here lies the body of Jonathan Blake. -
Stepped on the gas instead of the brake.
+
X +
X