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

--HG--
extra : moz-landing-system : lando

gfx/wr/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 ||

gfx/wr/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;

gfx/wr/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<Picture>;
+#[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<PictureData>;
+#[cfg_attr(feature = "capture", derive(Serialize))]
+#[cfg_attr(feature = "replay", derive(Deserialize))]
+#[derive(MallocSizeOf)]
+pub struct PictureTemplate;
 
 impl From<PictureKey> 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::<Picture>(), 88, "Picture size changed");
-    assert_eq!(mem::size_of::<PictureTemplate>(), 20, "PictureTemplate size changed");
-    assert_eq!(mem::size_of::<PictureKey>(), 104, "PictureKey size changed");
+    assert_eq!(mem::size_of::<PictureTemplate>(), 0, "PictureTemplate size changed");
+    assert_eq!(mem::size_of::<PictureKey>(), 88, "PictureKey size changed");
 }

gfx/wr/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];

gfx/wr/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<PendingPrimitive<TextRun>> 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,
         },