Bug 1453364 - Update scrollbar transforms separately from OMTA transforms. r=nical
authorKartikaya Gupta <kgupta@mozilla.com>
Wed, 11 Apr 2018 15:28:00 -0400
changeset 413106 ab6d1fd27c3b78aa8e4ae2c635e93e0875b3b9a3
parent 413105 429c254bc8d6efade6fff1e823bf817c218c89f8
child 413107 385de3e4dca0432925d639d3eaa22d4b883c86f8
push id33833
push useraiakab@mozilla.com
push dateFri, 13 Apr 2018 09:41:15 +0000
treeherdermozilla-central@260e4c83c8a9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnical
bugs1453364
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1453364 - Update scrollbar transforms separately from OMTA transforms. r=nical Although they still happen in the same transaction, they are done in two separate frame messages. This results in better encapsulated code on the C++ side since we don't have to pass around an array of properties, and will simplify future changes to update these properties at render time rather than at GenerateFrame time. MozReview-Commit-ID: 9qUkHX7gmD1
gfx/layers/apz/public/APZSampler.h
gfx/layers/apz/src/APZCTreeManager.cpp
gfx/layers/apz/src/APZCTreeManager.h
gfx/layers/apz/src/APZSampler.cpp
gfx/layers/wr/WebRenderBridgeParent.cpp
gfx/layers/wr/WebRenderBridgeParent.h
gfx/webrender_bindings/WebRenderAPI.cpp
gfx/webrender_bindings/WebRenderAPI.h
gfx/webrender_bindings/src/bindings.rs
gfx/webrender_bindings/webrender_ffi_generated.h
--- a/gfx/layers/apz/public/APZSampler.h
+++ b/gfx/layers/apz/public/APZSampler.h
@@ -33,18 +33,17 @@ struct ScrollbarData;
  */
 class APZSampler {
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(APZSampler)
 
 public:
   explicit APZSampler(const RefPtr<APZCTreeManager>& aApz);
 
   bool PushStateToWR(wr::TransactionBuilder& aTxn,
-                     const TimeStamp& aSampleTime,
-                     nsTArray<wr::WrTransformProperty>& aTransformArray);
+                     const TimeStamp& aSampleTime);
 
   bool SampleAnimations(const LayerMetricsWrapper& aLayer,
                         const TimeStamp& aSampleTime);
 
   /**
    * Compute the updated shadow transform for a scroll thumb layer that
    * reflects async scrolling of the associated scroll frame.
    *
--- a/gfx/layers/apz/src/APZCTreeManager.cpp
+++ b/gfx/layers/apz/src/APZCTreeManager.cpp
@@ -527,18 +527,17 @@ APZCTreeManager::UpdateHitTestingTree(La
   AssertOnUpdaterThread();
 
   UpdateHitTestingTreeImpl(aRootLayerTreeId, aScrollWrapper, aIsFirstPaint,
                            aOriginatingLayersId, aPaintSequenceNumber);
 }
 
 bool
 APZCTreeManager::PushStateToWR(wr::TransactionBuilder& aTxn,
-                               const TimeStamp& aSampleTime,
-                               nsTArray<wr::WrTransformProperty>& aTransformArray)
+                               const TimeStamp& aSampleTime)
 {
   AssertOnSamplerThread();
 
   RecursiveMutexAutoLock lock(mTreeLock);
 
   // During the first pass through the tree, we build a cache of guid->HTTN so
   // that we can find the relevant APZC instances quickly in subsequent passes,
   // such as the one below to generate scrollbar transforms. Without this, perf
@@ -603,16 +602,17 @@ APZCTreeManager::PushStateToWR(wr::Trans
         activeAnimations |= apzc->AdvanceAnimations(aSampleTime);
       });
 
   // Now we iterate over the nodes again, and generate the transforms needed
   // for scrollbar thumbs. Although we *could* do this as part of the previous
   // iteration, it's cleaner and more efficient to do it as a separate pass
   // because now we have a populated httnMap which allows O(log n) lookup here,
   // resulting in O(n log n) runtime.
+  nsTArray<wr::WrTransformProperty> scrollbarTransforms;
   ForEachNode<ReverseIterator>(mRootNode.get(),
       [&](HitTestingTreeNode* aNode)
       {
         if (!aNode->IsScrollThumbNode()) {
           return;
         }
         ScrollableLayerGuid guid(aNode->GetLayersId(), 0, aNode->GetScrollTargetId());
         auto it = httnMap.find(guid);
@@ -632,20 +632,21 @@ APZCTreeManager::PushStateToWR(wr::Trans
                     aNode->GetTransform() * AsyncTransformMatrix(),
                     scrollTargetNode->GetTransform().ToUnknownMatrix(),
                     scrollTargetApzc,
                     aMetrics,
                     aNode->GetScrollbarData(),
                     scrollTargetNode->IsAncestorOf(aNode),
                     nullptr);
             });
-        aTransformArray.AppendElement(wr::ToWrTransformProperty(
+        scrollbarTransforms.AppendElement(wr::ToWrTransformProperty(
             aNode->GetScrollbarAnimationId(),
             transform));
       });
+  aTxn.AppendTransformProperties(scrollbarTransforms);
 
   return activeAnimations;
 }
 
 // Compute the clip region to be used for a layer with an APZC. This function
 // is only called for layers which actually have scrollable metrics and an APZC.
 template<class ScrollNode> static ParentLayerIntRegion
 ComputeClipRegion(const ScrollNode& aLayer)
--- a/gfx/layers/apz/src/APZCTreeManager.h
+++ b/gfx/layers/apz/src/APZCTreeManager.h
@@ -196,23 +196,22 @@ public:
 
   /**
    * Called when webrender is enabled, from the sampler thread. This function
    * walks through the tree of APZC instances and tells webrender about the
    * async scroll position. It also advances APZ animations to the specified
    * sample time. In effect it is the webrender equivalent of (part of) the
    * code in AsyncCompositionManager. If scrollbar transforms need updating
    * to reflect the async scroll position, the updated transforms are appended
-   * to the provided aTransformArray.
+   * to the provided transaction as well.
    * Returns true if any APZ animations are in progress and we need to keep
    * compositing.
    */
   bool PushStateToWR(wr::TransactionBuilder& aTxn,
-                     const TimeStamp& aSampleTime,
-                     nsTArray<wr::WrTransformProperty>& aTransformArray);
+                     const TimeStamp& aSampleTime);
 
   /**
    * Walk the tree of APZCs and flushes the repaint requests for all the APZCS
    * corresponding to the given layers id. Finally, sends a flush complete
    * notification to the GeckoContentController for the layers id.
    */
   void FlushApzRepaints(LayersId aLayersId);
 
--- a/gfx/layers/apz/src/APZSampler.cpp
+++ b/gfx/layers/apz/src/APZSampler.cpp
@@ -26,22 +26,21 @@ APZSampler::APZSampler(const RefPtr<APZC
 
 APZSampler::~APZSampler()
 {
   mApz->SetSampler(nullptr);
 }
 
 bool
 APZSampler::PushStateToWR(wr::TransactionBuilder& aTxn,
-                          const TimeStamp& aSampleTime,
-                          nsTArray<wr::WrTransformProperty>& aTransformArray)
+                          const TimeStamp& aSampleTime)
 {
   // This function will be removed eventually since we'll have WR pull
   // the transforms from APZ instead.
-  return mApz->PushStateToWR(aTxn, aSampleTime, aTransformArray);
+  return mApz->PushStateToWR(aTxn, aSampleTime);
 }
 
 bool
 APZSampler::SampleAnimations(const LayerMetricsWrapper& aLayer,
                              const TimeStamp& aSampleTime)
 {
   MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
   AssertOnSamplerThread();
--- a/gfx/layers/wr/WebRenderBridgeParent.cpp
+++ b/gfx/layers/wr/WebRenderBridgeParent.cpp
@@ -528,33 +528,32 @@ WebRenderBridgeParent::UpdateAPZScrollDa
   }
   LayersId rootLayersId = cbp->RootLayerTreeId();
   if (RefPtr<APZUpdater> apz = cbp->GetAPZUpdater()) {
     apz->UpdateScrollDataAndTreeState(rootLayersId, GetLayersId(), aEpoch, Move(aData));
   }
 }
 
 bool
-WebRenderBridgeParent::PushAPZStateToWR(wr::TransactionBuilder& aTxn,
-                                        nsTArray<wr::WrTransformProperty>& aTransformArray)
+WebRenderBridgeParent::PushAPZStateToWR(wr::TransactionBuilder& aTxn)
 {
   CompositorBridgeParent* cbp = GetRootCompositorBridgeParent();
   if (!cbp) {
     return false;
   }
   if (RefPtr<APZSampler> apz = cbp->GetAPZSampler()) {
     TimeStamp animationTime = cbp->GetTestingTimeStamp().valueOr(
         mCompositorScheduler->GetLastComposeTime());
     TimeDuration frameInterval = cbp->GetVsyncInterval();
     // As with the non-webrender codepath in AsyncCompositionManager, we want to
     // use the timestamp for the next vsync when advancing animations.
     if (frameInterval != TimeDuration::Forever()) {
       animationTime += frameInterval;
     }
-    return apz->PushStateToWR(aTxn, animationTime, aTransformArray);
+    return apz->PushStateToWR(aTxn, animationTime);
   }
   return false;
 }
 
 mozilla::ipc::IPCResult
 WebRenderBridgeParent::RecvSetDisplayList(const gfx::IntSize& aSize,
                                           InfallibleTArray<WebRenderParentCommand>&& aCommands,
                                           InfallibleTArray<OpDestroy>&& aToDestroy,
@@ -1229,41 +1228,43 @@ WebRenderBridgeParent::CompositeToTarget
   }
 
   if (!mAsyncImageManager->GetAndResetWillGenerateFrame() &&
       !mForceRendering) {
     // Could skip generating frame now.
     return;
   }
 
+  wr::TransactionBuilder txn;
+
   nsTArray<wr::WrOpacityProperty> opacityArray;
   nsTArray<wr::WrTransformProperty> transformArray;
 
   SampleAnimations(opacityArray, transformArray);
   if (!transformArray.IsEmpty() || !opacityArray.IsEmpty()) {
     ScheduleGenerateFrame();
   }
+  // We do this even if the arrays are empty, because it will clear out any
+  // previous properties stored on the WR side, which is desirable. Also, we
+  // must do this before the PushAPZStateToWR call which will append more
+  // properties, If we did this after that call, this would clobber those
+  // properties.
+  txn.UpdateDynamicProperties(opacityArray, transformArray);
 
-  wr::TransactionBuilder txn;
-
-  if (PushAPZStateToWR(txn, transformArray)) {
+  if (PushAPZStateToWR(txn)) {
     ScheduleGenerateFrame();
   }
 
   wr::RenderThread::Get()->IncPendingFrameCount(mApi->GetId());
 
 #if defined(ENABLE_FRAME_LATENCY_LOG)
   auto startTime = TimeStamp::Now();
   mApi->SetFrameStartTime(startTime);
 #endif
 
-  if (!transformArray.IsEmpty() || !opacityArray.IsEmpty()) {
-    txn.UpdateDynamicProperties(opacityArray, transformArray);
-  }
-
   txn.GenerateFrame();
 
   mApi->SendTransaction(txn);
 }
 
 void
 WebRenderBridgeParent::HoldPendingTransactionId(const wr::Epoch& aWrEpoch,
                                                 uint64_t aTransactionId,
--- a/gfx/layers/wr/WebRenderBridgeParent.h
+++ b/gfx/layers/wr/WebRenderBridgeParent.h
@@ -217,20 +217,19 @@ private:
   void AdvanceAnimations();
   void SampleAnimations(nsTArray<wr::WrOpacityProperty>& aOpacityArray,
                         nsTArray<wr::WrTransformProperty>& aTransformArray);
 
   CompositorBridgeParent* GetRootCompositorBridgeParent() const;
 
   // Have APZ push the async scroll state to WR. Returns true if an APZ
   // animation is in effect and we need to schedule another composition.
-  // If scrollbars need their transforms updated, the provided aTransformArray
-  // is populated with the property update details.
-  bool PushAPZStateToWR(wr::TransactionBuilder& aTxn,
-                        nsTArray<wr::WrTransformProperty>& aTransformArray);
+  // If scrollbars need their transforms updated, the transaction builder
+  // is populated with the property update details via AppendTransformProperties
+  bool PushAPZStateToWR(wr::TransactionBuilder& aTxn);
 
   wr::Epoch GetNextWrEpoch();
 
 private:
   struct PendingTransactionId {
     PendingTransactionId(const wr::Epoch& aEpoch, uint64_t aId, const TimeStamp& aTxnStartTime, const TimeStamp& aFwdTime)
       : mEpoch(aEpoch)
       , mId(aId)
--- a/gfx/webrender_bindings/WebRenderAPI.cpp
+++ b/gfx/webrender_bindings/WebRenderAPI.cpp
@@ -201,23 +201,31 @@ TransactionBuilder::GenerateFrame()
 {
   wr_transaction_generate_frame(mTxn);
 }
 
 void
 TransactionBuilder::UpdateDynamicProperties(const nsTArray<wr::WrOpacityProperty>& aOpacityArray,
                                      const nsTArray<wr::WrTransformProperty>& aTransformArray)
 {
-  wr_transaction_update_dynamic_properties(mTxn,
-                                           aOpacityArray.IsEmpty() ?
-                                             nullptr : aOpacityArray.Elements(),
-                                           aOpacityArray.Length(),
-                                           aTransformArray.IsEmpty() ?
-                                             nullptr : aTransformArray.Elements(),
-                                           aTransformArray.Length());
+  wr_transaction_update_dynamic_properties(
+      mTxn,
+      aOpacityArray.IsEmpty() ?  nullptr : aOpacityArray.Elements(),
+      aOpacityArray.Length(),
+      aTransformArray.IsEmpty() ?  nullptr : aTransformArray.Elements(),
+      aTransformArray.Length());
+}
+
+void
+TransactionBuilder::AppendTransformProperties(const nsTArray<wr::WrTransformProperty>& aTransformArray)
+{
+  wr_transaction_append_transform_properties(
+      mTxn,
+      aTransformArray.IsEmpty() ? nullptr : aTransformArray.Elements(),
+      aTransformArray.Length());
 }
 
 bool
 TransactionBuilder::IsEmpty() const
 {
   return wr_transaction_is_empty(mTxn);
 }
 
--- a/gfx/webrender_bindings/WebRenderAPI.h
+++ b/gfx/webrender_bindings/WebRenderAPI.h
@@ -72,16 +72,17 @@ public:
                       wr::Vec<uint8_t>& dl_data);
 
   void ClearDisplayList(Epoch aEpoch, wr::WrPipelineId aPipeline);
 
   void GenerateFrame();
 
   void UpdateDynamicProperties(const nsTArray<wr::WrOpacityProperty>& aOpacityArray,
                                const nsTArray<wr::WrTransformProperty>& aTransformArray);
+  void AppendTransformProperties(const nsTArray<wr::WrTransformProperty>& aTransformArray);
 
   void SetWindowParameters(const LayoutDeviceIntSize& aWindowSize,
                            const LayoutDeviceIntRect& aDocRect);
 
   void UpdateScrollPosition(const wr::WrPipelineId& aPipelineId,
                             const layers::FrameMetrics::ViewID& aScrollId,
                             const wr::LayoutPoint& aScrollPosition);
 
--- a/gfx/webrender_bindings/src/bindings.rs
+++ b/gfx/webrender_bindings/src/bindings.rs
@@ -1061,18 +1061,16 @@ pub extern "C" fn wr_transaction_generat
 #[no_mangle]
 pub extern "C" fn wr_transaction_update_dynamic_properties(
     txn: &mut Transaction,
     opacity_array: *const WrOpacityProperty,
     opacity_count: usize,
     transform_array: *const WrTransformProperty,
     transform_count: usize,
 ) {
-    debug_assert!(transform_count > 0 || opacity_count > 0);
-
     let mut properties = DynamicProperties {
         transforms: Vec::new(),
         floats: Vec::new(),
     };
 
     if transform_count > 0 {
         let transform_slice = make_slice(transform_array, transform_count);
 
@@ -1097,16 +1095,45 @@ pub extern "C" fn wr_transaction_update_
             properties.floats.push(prop);
         }
     }
 
     txn.update_dynamic_properties(properties);
 }
 
 #[no_mangle]
+pub extern "C" fn wr_transaction_append_transform_properties(
+    txn: &mut Transaction,
+    transform_array: *const WrTransformProperty,
+    transform_count: usize,
+) {
+    if transform_count == 0 {
+        return;
+    }
+
+    let mut properties = DynamicProperties {
+        transforms: Vec::new(),
+        floats: Vec::new(),
+    };
+
+    let transform_slice = make_slice(transform_array, transform_count);
+
+    for element in transform_slice.iter() {
+        let prop = PropertyValue {
+            key: PropertyBindingKey::new(element.id),
+            value: element.transform.into(),
+        };
+
+        properties.transforms.push(prop);
+    }
+
+    txn.append_dynamic_properties(properties);
+}
+
+#[no_mangle]
 pub extern "C" fn wr_transaction_scroll_layer(
     txn: &mut Transaction,
     pipeline_id: WrPipelineId,
     scroll_id: u64,
     new_scroll_origin: LayoutPoint
 ) {
     assert!(unsafe { is_in_compositor_thread() });
     let scroll_id = ExternalScrollId(scroll_id, pipeline_id);
--- a/gfx/webrender_bindings/webrender_ffi_generated.h
+++ b/gfx/webrender_bindings/webrender_ffi_generated.h
@@ -937,31 +937,31 @@ struct FontInstancePlatformOptions {
 
   bool operator==(const FontInstancePlatformOptions& aOther) const {
     return lcd_filter == aOther.lcd_filter &&
            hinting == aOther.hinting;
   }
 };
 #endif
 
+struct WrTransformProperty {
+  uint64_t id;
+  LayoutTransform transform;
+};
+
 struct WrOpacityProperty {
   uint64_t id;
   float opacity;
 
   bool operator==(const WrOpacityProperty& aOther) const {
     return id == aOther.id &&
            opacity == aOther.opacity;
   }
 };
 
-struct WrTransformProperty {
-  uint64_t id;
-  LayoutTransform transform;
-};
-
 extern "C" {
 
 /* DO NOT MODIFY THIS MANUALLY! This file was generated using cbindgen.
  * To generate this file:
  *   1. Get the latest cbindgen using `cargo install --force cbindgen`
  *      a. Alternatively, you can clone `https://github.com/eqrion/cbindgen` and use a tagged release
  *   2. Run `rustup run nightly cbindgen toolkit/library/rust/ --lockfile Cargo.lock --crate webrender_bindings -o gfx/webrender_bindings/webrender_ffi_generated.h`
  */
@@ -1593,16 +1593,22 @@ WR_INLINE
 void wr_thread_pool_delete(WrThreadPool *aThreadPool)
 WR_DESTRUCTOR_SAFE_FUNC;
 
 WR_INLINE
 WrThreadPool *wr_thread_pool_new()
 WR_FUNC;
 
 WR_INLINE
+void wr_transaction_append_transform_properties(Transaction *aTxn,
+                                                const WrTransformProperty *aTransformArray,
+                                                uintptr_t aTransformCount)
+WR_FUNC;
+
+WR_INLINE
 void wr_transaction_clear_display_list(Transaction *aTxn,
                                        WrEpoch aEpoch,
                                        WrPipelineId aPipelineId)
 WR_FUNC;
 
 WR_INLINE
 void wr_transaction_delete(Transaction *aTxn)
 WR_DESTRUCTOR_SAFE_FUNC;