Change FilterPoints() to not remove points later than metadata_

Currently, if no point in |points_| has the same timestamp as
|metadata_|, everything in the map is erased. However, this sometimes
results in some points that have timestamps later than the metadata's
being erased. Particularly, this can occur when the API is first used,
since the point used to make the metadata wasn't sent to viz by the
browser process, since it didn't know to send them yet. However, if it
then sends points before this metadata makes it to viz, those points
would be erased since none match metadata's timestamp. This could
cause future trails to not be drawn too.

This change fixes that by only removing points that have timestamps
earlier than metadata's timestamp.

Bug: 1132043
Change-Id: I1da7b4ca7de8e816cebb67dece7a2bb1743cddf7
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2429963
Commit-Queue: Mario Bianucci <mabian@microsoft.com>
Reviewed-by: weiliangc <weiliangc@chromium.org>
Reviewed-by: Daniel Libby <dlibby@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#818179}

components/viz/service/display/delegated_ink_point_renderer_base.cc

@@ -27,23 +27,11 @@ void DelegatedInkPointRendererBase::InitMessagePipeline(
   receiver_.Bind(std::move(receiver));
 }
 
-void DelegatedInkPointRendererBase::FilterPoints() {
+std::vector<DelegatedInkPoint> DelegatedInkPointRendererBase::FilterPoints() {
   if (points_.size() == 0)
-    return;
+    return {};
 
-  // |first_valid_point| is the first point in |points_| that will be drawn.
-  // It will match |metadata_|'s timestamp and point because the app rendered
-  // ink stroke and the delegated ink trail must overlap on the final point of
-  // the ink stroke in order to connect seamlessly.
-  auto first_valid_point = points_.find(metadata_->timestamp());
-
-  // It is possible that this results in |points_| being empty. This occurs when
-  // the points being forwarded from the browser process lose the race against
-  // the ink metadata arriving in Display, including the point that matches the
-  // metadata. There may still be old points in |points_| allowing execution to
-  // get here, but none of them match the metadata point, so they are all
-  // erased.
-  points_.erase(points_.begin(), first_valid_point);
+  DCHECK(metadata_);
 
   // TODO(1052145): Add additional filtering to prevent points in |points_| from
   // having a timestamp that is far ahead of |metadata_|'s timestamp. This could
@@ -53,8 +41,56 @@ void DelegatedInkPointRendererBase::FilterPoints() {
   // stored here, resulting in a long possibly incorrect trail if the max
   // number of points to store was reached.
 
-  TRACE_EVENT_INSTANT1("viz", "Filtered points for delegated ink trail",
-                       TRACE_EVENT_SCOPE_THREAD, "points", points_.size());
+  // First remove all points from |points_| with timestamps earlier than
+  // |metadata_|, as they have already been rendered by the app and are no
+  // longer useful for a trail.
+  // After that, there are three possible state of |points_|:
+  //   1. The earliest DelegatedInkPoint in |points_| matches |metadata_|'s
+  //      timestamp. All the points in |points_| can be used to draw a trail.
+  //   2. |points_| is empty. No DelegatedInkPoints arrived from the browser
+  //      process with a timestamp equal to or later than |metadata_|'s, so we
+  //      don't have any points to make a trail from.
+  //   3. There are DelegatedInkPoints in |points_|, but the earliest one is
+  //      later than |metadata_|. This can happen most often when the API is
+  //      first used, as the browser process did not know to send the point
+  //      to viz before it was used to make the metadata in the renderer. So
+  //      although it didn't send the DelegatedInkPoint matching |metadata_|, it
+  //      still may have sent future points before the metadata propagated all
+  //      the way here. In this case, we choose not to use the points in
+  //      |points_| to draw, as we have no way of confirming that there
+  //      shouldn't be any extra points between |metadata_| and the beginning
+  //      of |points_|. So instead, just leave everything after |metadata_| in
+  //      |points_| so that they may be used in future trails and don't draw
+  //      any trail for the current |metadata_|.
+  // So if |points_| contains a timestamp that matches |metadata_|'s timestamp,
+  // add it and every point after it to |points_to_draw| and return it for
+  // drawing. If it doesn't, just return an empty vector and leave any point
+  // with a timestamp later than |metadata_|'s in |points_|.
+  std::vector<DelegatedInkPoint> points_to_draw;
+
+  auto it = points_.begin();
+  while (points_.size() > 0 && it != points_.end()) {
+    if (it->first < metadata_->timestamp()) {
+      // Sanity check to confirm that we always find the points that are before
+      // |metadata_|'s timestamp at the beginning of |points_| since it should
+      // be sorted.
+      DCHECK(it == points_.begin());
+      it = points_.erase(it);
+    } else {
+      if (it->first == metadata_->timestamp() || points_to_draw.size() > 0) {
+        points_to_draw.emplace_back(it->second, it->first);
+        it++;
+      } else {
+        // If we find a point that is later than |metadata_|'s timestamp before
+        // finding one that matches |metadata_|'s timestamp, that means that
+        // it doesn't exist in |points_|, so return an empty vector as there are
+        // no valid points to draw.
+        break;
+      }
+    }
+  }
+
+  return points_to_draw;
 }
 
 void DelegatedInkPointRendererBase::StoreDelegatedInkPoint(

components/viz/service/display/delegated_ink_point_renderer_base.h

@@ -8,6 +8,7 @@
 #include <map>
 #include <memory>
 #include <utility>
+#include <vector>
 
 #include "components/viz/service/viz_service_export.h"
 #include "mojo/public/cpp/bindings/receiver.h"
@@ -54,15 +55,10 @@ class VIZ_SERVICE_EXPORT DelegatedInkPointRendererBase
   // ink trail. However, if a point has a timestamp that is earlier than the
   // timestamp on the metadata, then the point has already been drawn, and
   // therefore should be removed from |points_| before drawing.
-  void FilterPoints();
+  std::vector<DelegatedInkPoint> FilterPoints();
 
   std::unique_ptr<DelegatedInkMetadata> metadata_;
 
-  // The points that will be drawn as part of the ink stroke. After being
-  // filtered in FilterPoints(), but before drawing, the first point in this map
-  // will match |metadata_|'s point, or it will be empty.
-  std::map<base::TimeTicks, gfx::PointF> points_;
-
  private:
   friend class SkiaDelegatedInkRendererTest;
 
@@ -76,6 +72,10 @@ class VIZ_SERVICE_EXPORT DelegatedInkPointRendererBase
 
   virtual int GetPathPointCountForTest() const = 0;
 
+  // The points that arrived from the browser process and may be drawn as part
+  // of the ink trail.
+  std::map<base::TimeTicks, gfx::PointF> points_;
+
   mojo::Receiver<mojom::DelegatedInkPointRenderer> receiver_{this};
 };
 

components/viz/service/display/delegated_ink_point_renderer_skia.cc

@@ -73,29 +73,33 @@ void DelegatedInkPointRendererSkia::FinalizePathForDraw() {
 
   // First, filter the delegated ink points so that only ones that have a
   // timestamp that is equal to or later than the metadata still exist.
-  FilterPoints();
+  std::vector<DelegatedInkPoint> points_to_draw = FilterPoints();
+
+  TRACE_EVENT_INSTANT1("viz", "Filtered points for delegated ink trail",
+                       TRACE_EVENT_SCOPE_THREAD, "points",
+                       points_to_draw.size());
 
   // TODO(1052145): Predict points.
 
   base::TimeDelta improvement =
-      static_cast<int>(points_.size()) > 0
-          ? points_.rbegin()->first - metadata_->timestamp()
+      static_cast<int>(points_to_draw.size()) > 0
+          ? points_to_draw.back().timestamp() - metadata_->timestamp()
           : base::TimeDelta::FromMilliseconds(0);
   UMA_HISTOGRAM_TIMES(
       "Renderer.DelegatedInkTrail.LatencyImprovement.Skia.WithoutPrediction",
       improvement);
 
-  // If there is only one point total between |points_| and predicted points,
-  // then it will match the metadata point and therefore doesn't need to be
-  // drawn in this way, as it will be rendered normally.
-  if (points_.size() <= 1) {
+  // If there is only one point total after filtering and predicting, then it
+  // will match the metadata point and therefore doesn't need to be drawn in
+  // this way, as it will be rendered normally.
+  if (points_to_draw.size() <= 1) {
     SetDamageRect(gfx::RectF());
     return;
   }
 
   std::vector<SkPoint> sk_points;
-  for (auto it : points_)
-    sk_points.emplace_back(gfx::PointFToSkPoint(it.second));
+  for (auto ink_point : points_to_draw)
+    sk_points.emplace_back(gfx::PointFToSkPoint(ink_point.point()));
 
   path_.moveTo(sk_points[0]);
   switch (sk_points.size()) {
@@ -113,9 +117,7 @@ void DelegatedInkPointRendererSkia::FinalizePathForDraw() {
       // the second control point of the first curve, the end point of the first
       // curve/first control point of the second curve, and the second control
       // point of the second curve are colinear. Since this is unlikely to be
-      // the case, and in general it is unlikely to be common that more than 4
-      // points exist in |points_|, connecting all four points via lines should
-      // be acceptable.
+      // the case, connecting all four points via lines should be acceptable.
       for (uint64_t i = 1; i < sk_points.size(); ++i)
         path_.lineTo(sk_points[i]);
       break;

components/viz/service/display/display_unittest.cc

@@ -4548,6 +4548,11 @@ class SkiaDelegatedInkRendererTest : public DisplayTest {
       ink_renderer()->StoreDelegatedInkPoint(ink_point);
   }
 
+  void SendMetadata(DelegatedInkMetadata metadata) {
+    ink_renderer()->SetDelegatedInkMetadata(
+        std::make_unique<DelegatedInkMetadata>(metadata));
+  }
+
   DelegatedInkMetadata MakeAndSendMetadataFromStoredInkPoint(
       int index,
       float diameter,
@@ -4559,8 +4564,7 @@ class SkiaDelegatedInkRendererTest : public DisplayTest {
     DelegatedInkMetadata metadata(ink_points_[index].point(), diameter, color,
                                   ink_points_[index].timestamp(),
                                   presentation_area);
-    ink_renderer()->SetDelegatedInkMetadata(
-        std::make_unique<DelegatedInkMetadata>(metadata));
+    SendMetadata(metadata);
     return metadata;
   }
 
@@ -4663,8 +4667,8 @@ TEST_F(SkiaDelegatedInkRendererTest, SkiaDelegatedInkRendererFilteringPoints) {
   DrawDelegatedInkTrail();
   EXPECT_FALSE(GetMetadataFromRenderer());
 
-  // Finally, add more points than the maximum that will be stored to confirm
-  // only the max is stored and the correct ones are removed first.
+  // Add more points than the maximum that will be stored to confirm only the
+  // max is stored and the correct ones are removed first.
   const int kPointsBeyondMaxAllowed = 2;
   StoreAlreadyCreatedDelegatedInkPoints();
   while (ink_points_size() <
@@ -4680,6 +4684,14 @@ TEST_F(SkiaDelegatedInkRendererTest, SkiaDelegatedInkRendererFilteringPoints) {
             stored_points().begin()->second);
   EXPECT_EQ(ink_point(ink_points_size() - 1).point(),
             stored_points().rbegin()->second);
+
+  // Now send metadata with a timestamp before all of the points that are
+  // currently stored to confirm that no points are filtered out and the number
+  // stored remains the same while the histogram records 0 improvement.
+  const uint64_t kExpectedPoints = stored_points().size();
+  SendMetadata(metadata);
+  FinalizePathAndCheckHistograms(base::TimeDelta::FromMilliseconds(0));
+  EXPECT_EQ(kExpectedPoints, stored_points().size());
 }
 
 // Confirm that the delegated ink trail histograms record latency correctly.