ambient: Refractoring photo refresh logic

Previously we allow to prefetch any preset number of images, but this is
not required and increases the complexity of code.
This patch simplifies the photo refresh logic. We only prefetch one more
image than displayed, which is used for photo transition animation.

Bug: b/156271483
Test: new unittests.
Change-Id: I0222c8f4db80d98dbd8f65453b4f8fe07bcb59e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2209538Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Commit-Queue: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#770850}

ash/ambient/ambient_constants.h

@@ -14,11 +14,6 @@ namespace ash {
 constexpr base::TimeDelta kAnimationDuration =
     base::TimeDelta::FromMilliseconds(250);
 
-// PhotoModel has a local in memory cache of downloaded photos. This is the
-// desired max number of photos stored in cache. If this is an even number,
-// the max number could be one larger.
-constexpr int kImageBufferLength = 5;
-
 // The default interval to refresh photos.
 // TODO(b/139953713): Change to a correct time interval.
 constexpr base::TimeDelta kPhotoRefreshInterval =

ash/ambient/ambient_photo_controller.cc

@@ -55,18 +55,7 @@ void AmbientPhotoController::StopScreenUpdate() {
 }
 
 void AmbientPhotoController::OnTopicsChanged() {
-  RefreshImage();
-}
-
-void AmbientPhotoController::RefreshImage() {
-  if (ambient_backend_model_.ShouldFetchImmediately()) {
-    base::SequencedTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(&AmbientPhotoController::GetNextImage,
-                                  weak_factory_.GetWeakPtr()));
-  } else {
-    ambient_backend_model_.ShowNextImage();
-    ScheduleRefreshImage();
-  }
+  ScheduleRefreshImage();
 }
 
 void AmbientPhotoController::ScheduleRefreshImage() {
@@ -76,11 +65,9 @@ void AmbientPhotoController::ScheduleRefreshImage() {
 
   // |photo_refresh_timer_| will start immediately if ShouldFetchImmediately()
   // is true.
-  // TODO(b/156271483): Consolidate RefreshImage() and ScheduleRefreshImage() to
-  // only check ShouldFetchImmediately() once.
   photo_refresh_timer_.Start(
       FROM_HERE, refresh_interval,
-      base::BindOnce(&AmbientPhotoController::RefreshImage,
+      base::BindOnce(&AmbientPhotoController::GetNextImage,
                      weak_factory_.GetWeakPtr()));
 }
 

ash/ambient/ambient_photo_controller.h

@@ -64,8 +64,8 @@ class ASH_EXPORT AmbientPhotoController : public AmbientBackendModelObserver {
  private:
   friend class AmbientAshTestBase;
 
-  void RefreshImage();
   void ScheduleRefreshImage();
+
   void GetScreenUpdateInfo();
 
   void GetNextImage();

ash/ambient/ambient_photo_controller_unittest.cc

@@ -27,18 +27,18 @@ using AmbientPhotoControllerTest = AmbientAshTestBase;
 
 // Test that image is downloaded when starting screen update.
 TEST_F(AmbientPhotoControllerTest, ShouldStartScreenUpdateSuccessfully) {
-  auto image = photo_controller()->ambient_backend_model()->GetCurrImage();
+  auto image = photo_controller()->ambient_backend_model()->GetCurrentImage();
   EXPECT_TRUE(image.isNull());
 
   // Start to refresh images.
   photo_controller()->StartScreenUpdate();
   base::RunLoop().RunUntilIdle();
-  image = photo_controller()->ambient_backend_model()->GetCurrImage();
+  image = photo_controller()->ambient_backend_model()->GetCurrentImage();
   EXPECT_FALSE(image.isNull());
 
   // Stop to refresh images.
   photo_controller()->StopScreenUpdate();
-  image = photo_controller()->ambient_backend_model()->GetCurrImage();
+  image = photo_controller()->ambient_backend_model()->GetCurrentImage();
   EXPECT_TRUE(image.isNull());
 }
 
@@ -51,20 +51,20 @@ TEST_F(AmbientPhotoControllerTest, ShouldUpdatePhotoPeriodically) {
   // Start to refresh images.
   photo_controller()->StartScreenUpdate();
   base::RunLoop().RunUntilIdle();
-  image1 = photo_controller()->ambient_backend_model()->GetCurrImage();
+  image1 = photo_controller()->ambient_backend_model()->GetCurrentImage();
   EXPECT_FALSE(image1.isNull());
   EXPECT_TRUE(image2.isNull());
 
   // Fastforward enough time to update the photo.
   task_environment()->FastForwardBy(1.2 * kPhotoRefreshInterval);
-  image2 = photo_controller()->ambient_backend_model()->GetCurrImage();
+  image2 = photo_controller()->ambient_backend_model()->GetCurrentImage();
   EXPECT_FALSE(image2.isNull());
   EXPECT_FALSE(image1.BackedBySameObjectAs(image2));
   EXPECT_TRUE(image3.isNull());
 
   // Fastforward enough time to update another photo.
   task_environment()->FastForwardBy(1.2 * kPhotoRefreshInterval);
-  image3 = photo_controller()->ambient_backend_model()->GetCurrImage();
+  image3 = photo_controller()->ambient_backend_model()->GetCurrentImage();
   EXPECT_FALSE(image3.isNull());
   EXPECT_FALSE(image1.BackedBySameObjectAs(image3));
   EXPECT_FALSE(image2.BackedBySameObjectAs(image3));

ash/ambient/model/ambient_backend_model.cc

@@ -44,37 +44,21 @@ const AmbientModeTopic& AmbientBackendModel::GetNextTopic() {
 }
 
 bool AmbientBackendModel::ShouldFetchImmediately() const {
-  // If currently shown image is close to the end of images cache, we prefetch
-  // more image.
-  const int next_load_image_index = GetImageBufferLength() / 2;
-  return images_.empty() ||
-         current_image_index_ >
-             static_cast<int>(images_.size() - 1 - next_load_image_index);
+  // Prefetch one image |next_image_| for photo transition animation.
+  return current_image_.isNull() || next_image_.isNull();
 }
 
-void AmbientBackendModel::ShowNextImage() {
-  // Do not show next if have not downloaded enough images.
-  if (ShouldFetchImmediately())
-    return;
-
-  const int max_current_image_index = GetImageBufferLength() / 2;
-  if (current_image_index_ >= max_current_image_index) {
-    // Pop the first image and keep |current_image_index_| unchanged, will be
-    // equivalent to show next image.
-    images_.pop_front();
+void AmbientBackendModel::AddNextImage(const gfx::ImageSkia& image) {
+  if (current_image_.isNull()) {
+    current_image_ = image;
+  } else if (next_image_.isNull()) {
+    next_image_ = image;
   } else {
-    ++current_image_index_;
+    current_image_ = next_image_;
+    next_image_ = image;
   }
-  NotifyImagesChanged();
-}
 
-void AmbientBackendModel::AddNextImage(const gfx::ImageSkia& image) {
-  images_.emplace_back(image);
-
-  // Update the first two images. The second image is used for photo transition
-  // animation.
-  if (images_.size() <= 2)
-    NotifyImagesChanged();
+  NotifyImagesChanged();
 }
 
 base::TimeDelta AmbientBackendModel::GetPhotoRefreshInterval() {
@@ -90,31 +74,17 @@ void AmbientBackendModel::SetPhotoRefreshInterval(base::TimeDelta interval) {
 
 void AmbientBackendModel::Clear() {
   topics_.clear();
-  images_.clear();
-  current_image_index_ = 0;
-}
-
-gfx::ImageSkia AmbientBackendModel::GetPrevImage() const {
-  if (current_image_index_ == 0)
-    return gfx::ImageSkia();
-
-  return images_[current_image_index_ - 1];
+  topic_index_ = 0;
+  current_image_ = gfx::ImageSkia();
+  next_image_ = gfx::ImageSkia();
 }
 
-gfx::ImageSkia AmbientBackendModel::GetCurrImage() const {
-  if (images_.empty())
-    return gfx::ImageSkia();
-
-  return images_[current_image_index_];
+gfx::ImageSkia AmbientBackendModel::GetCurrentImage() const {
+  return current_image_;
 }
 
 gfx::ImageSkia AmbientBackendModel::GetNextImage() const {
-  if (images_.empty() ||
-      static_cast<int>(images_.size() - current_image_index_) == 1) {
-    return gfx::ImageSkia();
-  }
-
-  return images_[current_image_index_ + 1];
+  return next_image_;
 }
 
 void AmbientBackendModel::UpdateWeatherInfo(
@@ -137,11 +107,6 @@ void AmbientBackendModel::NotifyImagesChanged() {
     observer.OnImagesChanged();
 }
 
-int AmbientBackendModel::GetImageBufferLength() const {
-  return buffer_length_for_testing_ == -1 ? kImageBufferLength
-                                          : buffer_length_for_testing_;
-}
-
 void AmbientBackendModel::NotifyWeatherInfoUpdated() {
   for (auto& observer : observers_)
     observer.OnWeatherInfoUpdated();

ash/ambient/model/ambient_backend_model.h

@@ -39,9 +39,6 @@ class ASH_EXPORT AmbientBackendModel {
   // Prefetch one more image for ShowNextImage animations.
   bool ShouldFetchImmediately() const;
 
-  // Show the next downloaded image.
-  void ShowNextImage();
-
   // Add image to local storage.
   void AddNextImage(const gfx::ImageSkia& image);
 
@@ -53,8 +50,7 @@ class ASH_EXPORT AmbientBackendModel {
   void Clear();
 
   // Get images from local storage. Could be null image.
-  gfx::ImageSkia GetPrevImage() const;
-  gfx::ImageSkia GetCurrImage() const;
+  gfx::ImageSkia GetCurrentImage() const;
   gfx::ImageSkia GetNextImage() const;
 
   // Updates the weather information and notifies observers if the icon image is
@@ -71,22 +67,18 @@ class ASH_EXPORT AmbientBackendModel {
   // Returns the cached temperature value in Fahrenheit.
   float temperature() const { return temperature_; }
 
-  void set_buffer_length_for_testing(int length) {
-    buffer_length_for_testing_ = length;
-  }
-
  private:
+  friend class AmbientBackendModelTest;
+
   void NotifyTopicsChanged();
   void NotifyImagesChanged();
   void NotifyWeatherInfoUpdated();
 
-  int GetImageBufferLength() const;
-
   std::vector<AmbientModeTopic> topics_;
 
-  // A local cache for downloaded images. This buffer is split into two equal
-  // length of kImageBufferLength / 2 for previous seen and next unseen images.
-  base::circular_deque<gfx::ImageSkia> images_;
+  // Local cache of downloaded images for photo transition animation.
+  gfx::ImageSkia current_image_;
+  gfx::ImageSkia next_image_;
 
   // The index of a topic to download.
   size_t topic_index_ = 0;

ash/ambient/model/ambient_backend_model_unittest.cc

@@ -15,14 +15,6 @@
 
 namespace ash {
 
-namespace {
-
-// This class has a local in memory cache of downloaded photos. This is the max
-// number of photos before and after currently shown image.
-constexpr int kTestImageBufferLength = 3;
-
-}  // namespace
-
 class AmbientBackendModelTest : public AshTestBase {
  public:
   AmbientBackendModelTest() = default;
@@ -32,10 +24,7 @@ class AmbientBackendModelTest : public AshTestBase {
 
   void SetUp() override {
     AshTestBase::SetUp();
-
     ambient_backend_model_ = std::make_unique<AmbientBackendModel>();
-    ambient_backend_model_->set_buffer_length_for_testing(
-        kTestImageBufferLength);
   }
 
   void TearDown() override {
@@ -43,8 +32,6 @@ class AmbientBackendModelTest : public AshTestBase {
     AshTestBase::TearDown();
   }
 
-  void ShowNextImage() { ambient_backend_model_->ShowNextImage(); }
-
   // Adds n test images to the model.
   void AddNTestImages(int n) {
     while (n > 0) {
@@ -78,9 +65,9 @@ class AmbientBackendModelTest : public AshTestBase {
     return ambient_backend_model_.get();
   }
 
-  gfx::ImageSkia prev_image() { return ambient_backend_model_->GetPrevImage(); }
-
-  gfx::ImageSkia curr_image() { return ambient_backend_model_->GetCurrImage(); }
+  gfx::ImageSkia current_image() {
+    return ambient_backend_model_->GetCurrentImage();
+  }
 
   gfx::ImageSkia next_image() { return ambient_backend_model_->GetNextImage(); }
 
@@ -92,8 +79,7 @@ class AmbientBackendModelTest : public AshTestBase {
 TEST_F(AmbientBackendModelTest, AddFirstImage) {
   AddNTestImages(1);
 
-  EXPECT_TRUE(IsNullImage(prev_image()));
-  EXPECT_TRUE(EqualsToTestImage(curr_image()));
+  EXPECT_TRUE(EqualsToTestImage(current_image()));
   EXPECT_TRUE(IsNullImage(next_image()));
 }
 
@@ -101,44 +87,22 @@ TEST_F(AmbientBackendModelTest, AddFirstImage) {
 TEST_F(AmbientBackendModelTest, AddSecondImage) {
   AddNTestImages(2);
 
-  // The default |current_image_index_| is 0.
-  EXPECT_TRUE(IsNullImage(prev_image()));
-  EXPECT_TRUE(EqualsToTestImage(curr_image()));
+  EXPECT_TRUE(EqualsToTestImage(current_image()));
   EXPECT_TRUE(EqualsToTestImage(next_image()));
-
-  // Increment the |current_image_index_| to 1.
-  ShowNextImage();
-  EXPECT_TRUE(EqualsToTestImage(prev_image()));
-  EXPECT_TRUE(EqualsToTestImage(curr_image()));
-  EXPECT_TRUE(IsNullImage(next_image()));
 }
 
 // Test adding the third image.
 TEST_F(AmbientBackendModelTest, AddThirdImage) {
-  AddNTestImages(3);
+  AddNTestImages(2);
 
-  // The default |current_image_index_| is 0.
-  EXPECT_TRUE(IsNullImage(prev_image()));
-  EXPECT_TRUE(EqualsToTestImage(curr_image()));
+  EXPECT_TRUE(EqualsToTestImage(current_image()));
   EXPECT_TRUE(EqualsToTestImage(next_image()));
 
-  // Increment the |current_image_index_| to 1.
-  ShowNextImage();
-  EXPECT_TRUE(EqualsToTestImage(prev_image()));
-  EXPECT_TRUE(EqualsToTestImage(curr_image()));
+  auto previous_next_image = next_image();
+  AddNTestImages(1);
+  EXPECT_TRUE(EqualsToTestImage(current_image()));
   EXPECT_TRUE(EqualsToTestImage(next_image()));
-
-  // Pop the |images_| front and keep the |current_image_index_| to 1.
-  ShowNextImage();
-  EXPECT_TRUE(EqualsToTestImage(prev_image()));
-  EXPECT_TRUE(EqualsToTestImage(curr_image()));
-  EXPECT_TRUE(IsNullImage(next_image()));
-
-  // ShowNextImage() will early return.
-  ShowNextImage();
-  EXPECT_TRUE(EqualsToTestImage(prev_image()));
-  EXPECT_TRUE(EqualsToTestImage(curr_image()));
-  EXPECT_TRUE(IsNullImage(next_image()));
+  EXPECT_TRUE(previous_next_image.BackedBySameObjectAs(current_image()));
 }
 
 // Test the photo refresh interval is expected.

ash/ambient/ui/photo_view.cc

@@ -95,17 +95,13 @@ const char* PhotoView::GetClassName() const {
 }
 
 void PhotoView::AddedToWidget() {
-  // Set the bounds to show |image_view_curr_| for the first time.
+  // Set the bounds to show |image_view_current_| for the first time.
   // TODO(b/140066694): Handle display configuration changes, e.g. resolution,
   // rotation, etc.
   const gfx::Size widget_size = GetWidget()->GetRootView()->size();
-  image_view_prev_->SetImageSize(widget_size);
-  image_view_curr_->SetImageSize(widget_size);
+  image_view_current_->SetImageSize(widget_size);
   image_view_next_->SetImageSize(widget_size);
-  gfx::Rect view_bounds = gfx::Rect(GetPreferredSize());
-  const int width = widget_size.width();
-  view_bounds.set_x(-width);
-  SetBoundsRect(view_bounds);
+  SetBoundsRect(gfx::Rect(GetPreferredSize()));
 }
 
 void PhotoView::OnImagesChanged() {
@@ -129,9 +125,7 @@ void PhotoView::Init() {
   layout->set_cross_axis_alignment(
       views::BoxLayout::CrossAxisAlignment::kStart);
 
-  image_view_prev_ =
-      AddChildView(std::make_unique<AmbientBackgroundImageView>(delegate_));
-  image_view_curr_ =
+  image_view_current_ =
       AddChildView(std::make_unique<AmbientBackgroundImageView>(delegate_));
   image_view_next_ =
       AddChildView(std::make_unique<AmbientBackgroundImageView>(delegate_));
@@ -143,8 +137,7 @@ void PhotoView::UpdateImages() {
   // TODO(b/140193766): Investigate a more efficient way to update images and do
   // layer animation.
   auto* model = delegate_->GetAmbientBackendModel();
-  image_view_prev_->SetImage(model->GetPrevImage());
-  image_view_curr_->SetImage(model->GetCurrImage());
+  image_view_current_->SetImage(model->GetCurrentImage());
   image_view_next_->SetImage(model->GetNextImage());
 }
 
@@ -158,7 +151,7 @@ void PhotoView::StartTransitionAnimation() {
   animation.SetAnimationMetricsReporter(metrics_reporter_.get());
   animation.AddObserver(this);
 
-  const int x_offset = image_view_curr_->GetPreferredSize().width();
+  const int x_offset = image_view_current_->GetPreferredSize().width();
   gfx::Transform transform;
   transform.Translate(-x_offset, 0);
   layer->SetTransform(transform);

ash/ambient/ui/photo_view.h

@@ -58,8 +58,7 @@ class ASH_EXPORT PhotoView : public views::View,
   std::unique_ptr<ui::AnimationMetricsReporter> metrics_reporter_;
 
   // Image containers used for animation. Owned by view hierarchy.
-  AmbientBackgroundImageView* image_view_prev_ = nullptr;
-  AmbientBackgroundImageView* image_view_curr_ = nullptr;
+  AmbientBackgroundImageView* image_view_current_ = nullptr;
   AmbientBackgroundImageView* image_view_next_ = nullptr;
 };