From f69883f8e3cc431cd1d2d94588498a1a184de48b Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Fri, 10 Apr 2020 03:06:03 +0000 Subject: [PATCH 1/2] Bug 1628508: Add 'IMPLICIT' comment for webrender_api::display_item::ScrollFrameDisplayItem. DONTBUILD r=gw Differential Revision: https://phabricator.services.mozilla.com/D70269 [ghsync] From https://hg.mozilla.org/mozilla-central/rev/2989b754632f15aa27065dccdb0eb2833dd86316 --- webrender_api/src/display_item.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webrender_api/src/display_item.rs b/webrender_api/src/display_item.rs index 39f2f9245e..6cde44af6c 100644 --- a/webrender_api/src/display_item.rs +++ b/webrender_api/src/display_item.rs @@ -289,7 +289,7 @@ pub struct ScrollFrameDisplayItem { /// should be added to those display item coordinates in order to get a /// normalized value that is consistent across display lists. pub external_scroll_offset: LayoutVector2D, -} +} // IMPLICIT: complex_clips: Vec /// A solid or an animating color to draw (may not actually be a rectangle due to complex clips) #[derive(Clone, Copy, Debug, Default, Deserialize, PartialEq, Serialize, PeekPoke)] From f074bd811b3f3b912f62240664bc7d0be99a1d1f Mon Sep 17 00:00:00 2001 From: Glenn Watson Date: Fri, 10 Apr 2020 03:06:12 +0000 Subject: [PATCH 2/2] Bug 1628564 - Remove the common primitive template data for picture primitives. r=nical Picture primitives are special, so it doesn't make sense to have the normal common primitive data for them. For example, the bounding rect of a picture is determined during frame building. Removing the common data for picture primitives simplifies the code and makes it impossible to accidentally access an invalid bounding rect for picture primitives. Differential Revision: https://phabricator.services.mozilla.com/D70295 [ghsync] From https://hg.mozilla.org/mozilla-central/rev/707947e62b49ce3f56c86731b803fe50cb18eacb --- webrender/src/batch.rs | 10 +++--- webrender/src/prim_store/mod.rs | 14 +++----- webrender/src/prim_store/picture.rs | 47 ++++++++++--------------- webrender/src/render_backend.rs | 54 ++++++++++++++++++++++++++--- webrender/src/scene_building.rs | 14 -------- 5 files changed, 78 insertions(+), 61 deletions(-) diff --git a/webrender/src/batch.rs b/webrender/src/batch.rs index 54635b1e45..d851393c5e 100644 --- a/webrender/src/batch.rs +++ b/webrender/src/batch.rs @@ -778,14 +778,13 @@ impl BatchBuilder { let z_id = z_generator.next(); - let prim_common_data = &ctx.data_stores.as_common_data(&prim_instance); - let prim_rect = LayoutRect::new( - prim_instance.prim_origin, - prim_common_data.prim_size + let prim_rect = ctx.data_stores.get_local_prim_rect( + prim_instance, + ctx.prim_store, ); let mut batch_features = BatchFeatures::empty(); - if prim_common_data.may_need_repetition { + if ctx.data_stores.prim_may_need_repetition(prim_instance) { batch_features |= BatchFeatures::REPETITION; } @@ -2037,6 +2036,7 @@ impl BatchBuilder { ); let specified_blend_mode = BlendMode::PremultipliedAlpha; + let prim_common_data = &ctx.data_stores.as_common_data(&prim_instance); let non_segmented_blend_mode = if !prim_common_data.opacity.is_opaque || prim_info.clip_task_index != ClipTaskIndex::INVALID || diff --git a/webrender/src/prim_store/mod.rs b/webrender/src/prim_store/mod.rs index 12ff39c6b9..9eac2a33bf 100644 --- a/webrender/src/prim_store/mod.rs +++ b/webrender/src/prim_store/mod.rs @@ -3597,12 +3597,10 @@ impl PrimitiveStore { // TODO(gw): Consider whether it's worth doing segment building // for gradient primitives. } - PrimitiveInstanceKind::Picture { pic_index, segment_instance_index, data_handle, .. } => { + PrimitiveInstanceKind::Picture { pic_index, segment_instance_index, .. } => { let pic = &mut self.pictures[pic_index.0]; let prim_info = &scratch.prim_info[prim_instance.visibility_info.0 as usize]; - data_stores.picture[*data_handle].common.may_need_repetition = false; - if pic.prepare_for_render( frame_context, frame_state, @@ -3886,9 +3884,9 @@ impl PrimitiveInstance { // Usually, the primitive rect can be found from information // in the instance and primitive template. - let mut prim_local_rect = LayoutRect::new( - self.prim_origin, - data_stores.as_common_data(self).prim_size, + let prim_local_rect = data_stores.get_local_prim_rect( + self, + prim_store, ); let segment_instance_index = match self.kind { @@ -3925,10 +3923,6 @@ impl PrimitiveInstance { pic.segments_are_valid = true; } - // Override the prim local rect with the dynamically calculated - // local rect for the picture. - prim_local_rect = pic.precise_local_rect; - segment_instance_index } else { return; diff --git a/webrender/src/prim_store/picture.rs b/webrender/src/prim_store/picture.rs index 3ddf9c56aa..46a52e0a80 100644 --- a/webrender/src/prim_store/picture.rs +++ b/webrender/src/prim_store/picture.rs @@ -4,9 +4,9 @@ use api::{ ColorU, MixBlendMode, FilterPrimitiveInput, FilterPrimitiveKind, ColorSpace, - PropertyBinding, PropertyBindingId, CompositeOperator, PrimitiveFlags, + PropertyBinding, PropertyBindingId, CompositeOperator, }; -use api::units::{Au, LayoutSize, LayoutVector2D}; +use api::units::{Au, LayoutVector2D}; use crate::scene_building::IsVisible; use crate::filterdata::SFilterData; use crate::intern::ItemUid; @@ -14,7 +14,6 @@ use crate::intern::{Internable, InternDebug, Handle as InternHandle}; use crate::internal_types::{LayoutPrimitiveInfo, Filter}; use crate::picture::PictureCompositeMode; use crate::prim_store::{ - PrimKey, PrimKeyCommonData, PrimTemplate, PrimTemplateCommonData, PrimitiveInstanceKind, PrimitiveStore, VectorKey, InternablePrimitive, }; @@ -235,21 +234,19 @@ pub struct Picture { pub composite_mode_key: PictureCompositeKey, } -pub type PictureKey = PrimKey; +#[cfg_attr(feature = "capture", derive(Serialize))] +#[cfg_attr(feature = "replay", derive(Deserialize))] +#[derive(Debug, Clone, Eq, MallocSizeOf, PartialEq, Hash)] +pub struct PictureKey { + pub composite_mode_key: PictureCompositeKey, +} impl PictureKey { pub fn new( - flags: PrimitiveFlags, - prim_size: LayoutSize, pic: Picture, ) -> Self { - PictureKey { - common: PrimKeyCommonData { - flags, - prim_size: prim_size.into(), - }, - kind: pic, + composite_mode_key: pic.composite_mode_key, } } } @@ -261,16 +258,14 @@ impl InternDebug for PictureKey {} #[derive(MallocSizeOf)] pub struct PictureData; -pub type PictureTemplate = PrimTemplate; +#[cfg_attr(feature = "capture", derive(Serialize))] +#[cfg_attr(feature = "replay", derive(Deserialize))] +#[derive(MallocSizeOf)] +pub struct PictureTemplate; impl From for PictureTemplate { - fn from(key: PictureKey) -> Self { - let common = PrimTemplateCommonData::with_key_common(key.common); - - PictureTemplate { - common, - kind: PictureData, - } + fn from(_: PictureKey) -> Self { + PictureTemplate } } @@ -285,13 +280,9 @@ impl Internable for Picture { impl InternablePrimitive for Picture { fn into_key( self, - info: &LayoutPrimitiveInfo, + _: &LayoutPrimitiveInfo, ) -> PictureKey { - PictureKey::new( - info.flags, - info.rect.size, - self, - ) + PictureKey::new(self) } fn make_instance_kind( @@ -323,6 +314,6 @@ fn test_struct_sizes() { // (b) You made a structure larger. This is not necessarily a problem, but should only // be done with care, and after checking if talos performance regresses badly. assert_eq!(mem::size_of::(), 88, "Picture size changed"); - assert_eq!(mem::size_of::(), 20, "PictureTemplate size changed"); - assert_eq!(mem::size_of::(), 104, "PictureKey size changed"); + assert_eq!(mem::size_of::(), 0, "PictureTemplate size changed"); + assert_eq!(mem::size_of::(), 88, "PictureKey size changed"); } diff --git a/webrender/src/render_backend.rs b/webrender/src/render_backend.rs index 3ac8c00e42..d393d96ea1 100644 --- a/webrender/src/render_backend.rs +++ b/webrender/src/render_backend.rs @@ -33,7 +33,7 @@ use crate::internal_types::{DebugOutput, FastHashMap, FastHashSet, RenderedDocum use malloc_size_of::{MallocSizeOf, MallocSizeOfOps}; use crate::picture::{RetainedTiles, TileCacheLogger}; use crate::prim_store::{PrimitiveScratchBuffer, PrimitiveInstance}; -use crate::prim_store::{PrimitiveInstanceKind, PrimTemplateCommonData}; +use crate::prim_store::{PrimitiveInstanceKind, PrimTemplateCommonData, PrimitiveStore}; use crate::prim_store::interned::*; use crate::profiler::{BackendProfileCounters, ResourceProfileCounters}; use crate::record::ApiRecordingReceiver; @@ -271,6 +271,53 @@ macro_rules! declare_data_stores { enumerate_interners!(declare_data_stores); impl DataStores { + /// Returns the local rect for a primitive. For most primitives, this is + /// stored in the template. For pictures, this is stored inside the picture + /// primitive instance itself, since this is determined during frame building. + pub fn get_local_prim_rect( + &self, + prim_instance: &PrimitiveInstance, + prim_store: &PrimitiveStore, + ) -> LayoutRect { + match prim_instance.kind { + PrimitiveInstanceKind::Picture { pic_index, .. } => { + let pic = &prim_store.pictures[pic_index.0]; + pic.precise_local_rect + } + PrimitiveInstanceKind::PushClipChain | + PrimitiveInstanceKind::PopClipChain => { + unreachable!(); + } + _ => { + LayoutRect::new( + prim_instance.prim_origin, + self.as_common_data(prim_instance).prim_size, + ) + } + } + } + + /// Returns true if this primitive might need repition. + // TODO(gw): This seems like the wrong place for this - maybe this flag should + // not be in the common prim template data? + pub fn prim_may_need_repetition( + &self, + prim_instance: &PrimitiveInstance, + ) -> bool { + match prim_instance.kind { + PrimitiveInstanceKind::Picture { .. } => { + false + } + PrimitiveInstanceKind::PushClipChain | + PrimitiveInstanceKind::PopClipChain => { + unreachable!(); + } + _ => { + self.as_common_data(prim_instance).may_need_repetition + } + } + } + pub fn as_common_data( &self, prim_inst: &PrimitiveInstance @@ -301,9 +348,8 @@ impl DataStores { let prim_data = &self.normal_border[data_handle]; &prim_data.common } - PrimitiveInstanceKind::Picture { data_handle, .. } => { - let prim_data = &self.picture[data_handle]; - &prim_data.common + PrimitiveInstanceKind::Picture { .. } => { + panic!("BUG: picture prims don't have common data!"); } PrimitiveInstanceKind::RadialGradient { data_handle, .. } => { let prim_data = &self.radial_grad[data_handle]; diff --git a/webrender/src/scene_building.rs b/webrender/src/scene_building.rs index a7c3faa41e..32120909cc 100644 --- a/webrender/src/scene_building.rs +++ b/webrender/src/scene_building.rs @@ -2053,7 +2053,6 @@ impl<'a> SceneBuilder<'a> { let mut cur_instance = create_prim_instance( leaf_pic_index, leaf_composite_mode.into(), - stacking_context.prim_flags, ClipChainId::NONE, &mut self.interners, ); @@ -2105,7 +2104,6 @@ impl<'a> SceneBuilder<'a> { cur_instance = create_prim_instance( current_pic_index, PictureCompositeKey::Identity, - stacking_context.prim_flags, ClipChainId::NONE, &mut self.interners, ); @@ -2169,7 +2167,6 @@ impl<'a> SceneBuilder<'a> { cur_instance = create_prim_instance( blend_pic_index, composite_mode.into(), - stacking_context.prim_flags, ClipChainId::NONE, &mut self.interners, ); @@ -2591,8 +2588,6 @@ impl<'a> SceneBuilder<'a> { ); let shadow_pic_key = PictureKey::new( - PrimitiveFlags::IS_BACKFACE_VISIBLE, - LayoutSize::zero(), Picture { composite_mode_key }, ); @@ -3339,7 +3334,6 @@ impl<'a> SceneBuilder<'a> { instance = create_prim_instance( backdrop_pic_index, composite_mode.into(), - prim_flags, clip_chain_id, &mut self.interners, ); @@ -3526,7 +3520,6 @@ impl<'a> SceneBuilder<'a> { cur_instance = create_prim_instance( current_pic_index, composite_mode.into(), - flags, ClipChainId::NONE, &mut self.interners, ); @@ -3597,7 +3590,6 @@ impl<'a> SceneBuilder<'a> { cur_instance = create_prim_instance( current_pic_index, Some(composite_mode).into(), - flags, ClipChainId::NONE, &mut self.interners, ); @@ -3913,7 +3905,6 @@ impl FlattenedStackingContext { let prim_instance = create_prim_instance( pic_index, composite_mode.into(), - self.prim_flags, self.clip_chain_id, interners, ); @@ -3981,13 +3972,10 @@ impl From> for ShadowItem { fn create_prim_instance( pic_index: PictureIndex, composite_mode_key: PictureCompositeKey, - flags: PrimitiveFlags, clip_chain_id: ClipChainId, interners: &mut Interners, ) -> PrimitiveInstance { let pic_key = PictureKey::new( - flags, - LayoutSize::zero(), Picture { composite_mode_key }, ); @@ -4112,8 +4100,6 @@ fn create_tile_cache( // Now, create a picture with tile caching enabled that will hold all // of the primitives selected as belonging to the main scroll root. let pic_key = PictureKey::new( - PrimitiveFlags::IS_BACKFACE_VISIBLE, - LayoutSize::zero(), Picture { composite_mode_key: PictureCompositeKey::Identity, },