ambient: synchronize images among photo_views on multiple displays

When a new display is added during ambient mode, make sure that each
image view has the same images.

Prior behavior:
  * OnImagesReady internal display initializes -> {0*, _}
  * internal display receives immediate OnImageAdded -> {0*, 1}
  * external display is connected -> {0*, 1}, {0*, _}
  * OnImageAdded for image 2 -> {2, 1*}, {0*, 2}
    - external display does not switch image because none on deck
  * OnImageAdded for image 3 -> {2*, 3}, {3, 2*}

New behavior:
  * OnImagesReady internal display initializes -> {0*, 1}
  * external display is connected -> {0*, 1}, {0*, 1}
  * OnImageAdded for image 2 -> {2, 1*}, {2, 1*}

Initialize |PhotoView| with both images stored in |AmbientBackendModel|
to guarantee they show images in the same order. When the second
image is added, call |OnImagesReady| after |OnImageAdded| so that
|PhotoView| does not receive an immediate |OnImageAdded| after
being constructed.

BUG=b:173229098

Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
Change-Id: I0f47d9a1b3748d74502080da9f0011e013897393
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2547308
Commit-Queue: Jeffrey Young <cowmoo@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837306}

ash/ambient/ambient_photo_controller.cc

@@ -602,7 +602,7 @@ void AmbientPhotoController::OnAllPhotoDecoded(bool from_downloading,
     // Try to read from cache when failure happens.
     TryReadPhotoRawData();
     return;
-  } else if (ambient_backend_model_.HashMatchesNextImage(hash)) {
+  } else if (ambient_backend_model_.IsHashDuplicate(hash)) {
     LOG(WARNING) << "Skipping loading duplicate image.";
     TryReadPhotoRawData();
     return;

ash/ambient/ambient_photo_controller_unittest.cc

@@ -41,7 +41,7 @@ class MockAmbientBackendModelObserver : public AmbientBackendModelObserver {
   ~MockAmbientBackendModelObserver() override = default;
 
   // AmbientBackendModelObserver:
-  MOCK_METHOD(void, OnImagesChanged, (), (override));
+  MOCK_METHOD(void, OnImageAdded, (), (override));
   MOCK_METHOD(void, OnImagesReady, (), (override));
 };
 
@@ -224,7 +224,7 @@ TEST_F(AmbientPhotoControllerTest, ShouldReadCacheWhenNoMoreTopics) {
   FetchImage();
   FastForwardToNextImage();
   // Topics is empty. Will read from cache, which is empty.
-  auto image = photo_controller()->ambient_backend_model()->GetNextImage();
+  auto image = photo_controller()->ambient_backend_model()->GetCurrentImage();
   EXPECT_TRUE(image.IsNull());
 
   // Save a file to check if it gets read for display.
@@ -236,7 +236,7 @@ TEST_F(AmbientPhotoControllerTest, ShouldReadCacheWhenNoMoreTopics) {
   photo_controller()->StopScreenUpdate();
   FetchImage();
   FastForwardToNextImage();
-  image = photo_controller()->ambient_backend_model()->GetNextImage();
+  image = photo_controller()->ambient_backend_model()->GetCurrentImage();
   EXPECT_FALSE(image.IsNull());
 
   // Clean up.
@@ -251,7 +251,7 @@ TEST_F(AmbientPhotoControllerTest,
   FetchImage();
   FastForwardToNextImage();
   // Topics is empty. Will read from cache, which is empty.
-  auto image = photo_controller()->ambient_backend_model()->GetNextImage();
+  auto image = photo_controller()->ambient_backend_model()->GetCurrentImage();
   EXPECT_TRUE(image.IsNull());
 
   // The initial file name to be read is 0. Save a file with 99.img to check if
@@ -264,7 +264,7 @@ TEST_F(AmbientPhotoControllerTest,
   photo_controller()->StopScreenUpdate();
   FetchImage();
   FastForwardToNextImage();
-  image = photo_controller()->ambient_backend_model()->GetNextImage();
+  image = photo_controller()->ambient_backend_model()->GetCurrentImage();
   EXPECT_FALSE(image.IsNull());
 }
 
@@ -277,7 +277,7 @@ TEST_F(AmbientPhotoControllerTest, ShouldReadCacheWhenImageDownloadingFailed) {
   // Forward a little bit time. FetchTopics() will succeed. Downloading should
   // fail. Will read from cache, which is empty.
   task_environment()->FastForwardBy(0.2 * kTopicFetchInterval);
-  auto image = photo_controller()->ambient_backend_model()->GetNextImage();
+  auto image = photo_controller()->ambient_backend_model()->GetCurrentImage();
   EXPECT_TRUE(image.IsNull());
 
   // Save a file to check if it gets read for display.
@@ -291,7 +291,7 @@ TEST_F(AmbientPhotoControllerTest, ShouldReadCacheWhenImageDownloadingFailed) {
   // Forward a little bit time. FetchTopics() will succeed. Downloading should
   // fail. Will read from cache.
   task_environment()->FastForwardBy(0.2 * kTopicFetchInterval);
-  image = photo_controller()->ambient_backend_model()->GetNextImage();
+  image = photo_controller()->ambient_backend_model()->GetCurrentImage();
   EXPECT_FALSE(image.IsNull());
 }
 
@@ -430,18 +430,18 @@ TEST_F(AmbientPhotoControllerTest, ShouldNotLoadDuplicateImages) {
   FastForwardTiny();
 
   // Should contain hash of downloaded data.
-  EXPECT_TRUE(photo_controller()->ambient_backend_model()->HashMatchesNextImage(
+  EXPECT_TRUE(photo_controller()->ambient_backend_model()->IsHashDuplicate(
       base::SHA1HashString("image data")));
   // Only one image should have been loaded.
   EXPECT_FALSE(photo_controller()->ambient_backend_model()->ImagesReady());
 
   // Now expect a call because second image is loaded.
-  EXPECT_CALL(mock_backend_observer, OnImagesChanged).Times(1);
+  EXPECT_CALL(mock_backend_observer, OnImagesReady).Times(1);
   SetDownloadPhotoData("image data 2");
   FastForwardToNextImage();
 
   // Second image should have been loaded.
-  EXPECT_TRUE(photo_controller()->ambient_backend_model()->HashMatchesNextImage(
+  EXPECT_TRUE(photo_controller()->ambient_backend_model()->IsHashDuplicate(
       base::SHA1HashString("image data 2")));
   EXPECT_TRUE(photo_controller()->ambient_backend_model()->ImagesReady());
 }

ash/ambient/model/ambient_backend_model.cc

@@ -60,22 +60,40 @@ bool AmbientBackendModel::ImagesReady() const {
 
 void AmbientBackendModel::AddNextImage(
     const PhotoWithDetails& photo_with_details) {
+  DCHECK(!photo_with_details.IsNull());
+
   ResetImageFailures();
+
+  bool should_notify_ready = false;
+
   if (current_image_.IsNull()) {
+    // If |current_image_| is null, |photo_with_details| should be the first
+    // image stored. |next_image_| should also be null.
+    DCHECK(next_image_.IsNull());
     current_image_ = photo_with_details;
   } else if (next_image_.IsNull()) {
+    // |current_image_| and |next_image_| are set.
     next_image_ = photo_with_details;
-    NotifyImagesReady();
+    should_notify_ready = true;
   } else {
+    // Cycle out the old |current_image_|.
     current_image_ = next_image_;
     next_image_ = photo_with_details;
   }
 
-  NotifyImagesChanged();
+  NotifyImageAdded();
+
+  // Observers expect |OnImagesReady| after |OnImageAdded|.
+  if (should_notify_ready)
+    NotifyImagesReady();
 }
 
-bool AmbientBackendModel::HashMatchesNextImage(const std::string& hash) const {
-  return GetNextImage().hash == hash;
+bool AmbientBackendModel::IsHashDuplicate(const std::string& hash) const {
+  // Make sure that a photo does not appear twice in a row. If |next_image_| is
+  // not null, the new image must not be identical to |next_image_|.
+  const auto& image_to_compare =
+      next_image_.IsNull() ? current_image_ : next_image_;
+  return image_to_compare.hash == hash;
 }
 
 void AmbientBackendModel::AddImageFailure() {
@@ -108,13 +126,6 @@ void AmbientBackendModel::Clear() {
   next_image_.Clear();
 }
 
-const PhotoWithDetails& AmbientBackendModel::GetNextImage() const {
-  if (!next_image_.IsNull())
-    return next_image_;
-
-  return current_image_;
-}
-
 float AmbientBackendModel::GetTemperatureInCelsius() const {
   return (temperature_fahrenheit_ - 32) * 5 / 9;
 }
@@ -136,9 +147,9 @@ void AmbientBackendModel::NotifyTopicsChanged() {
     observer.OnTopicsChanged();
 }
 
-void AmbientBackendModel::NotifyImagesChanged() {
+void AmbientBackendModel::NotifyImageAdded() {
   for (auto& observer : observers_)
-    observer.OnImagesChanged();
+    observer.OnImageAdded();
 }
 
 void AmbientBackendModel::NotifyImagesReady() {

ash/ambient/model/ambient_backend_model.h

@@ -63,8 +63,13 @@ class ASH_EXPORT AmbientBackendModel {
   // Add image to local storage.
   void AddNextImage(const PhotoWithDetails& photo);
 
-  // If the hash matches the hash of the next image to be displayed.
-  bool HashMatchesNextImage(const std::string& hash) const;
+  // Returns true if |hash| would cause an identical image to appear twice in a
+  // row. For example:
+  // {A, B} + B => true
+  // {A, B} + A => false
+  // {A, _} + B => false
+  // {A, _} + A => true
+  bool IsHashDuplicate(const std::string& hash) const;
 
   // Record that fetching an image has failed.
   void AddImageFailure();
@@ -77,7 +82,7 @@ class ASH_EXPORT AmbientBackendModel {
   void Clear();
 
   // Get images from local storage. Could be null image.
-  const PhotoWithDetails& GetNextImage() const;
+  const PhotoWithDetails& GetNextImage() const { return next_image_; }
   const PhotoWithDetails& GetCurrentImage() const { return current_image_; }
 
   // Updates the weather information and notifies observers if the icon image is
@@ -107,7 +112,7 @@ class ASH_EXPORT AmbientBackendModel {
   friend class AmbientAshTestBase;
 
   void NotifyTopicsChanged();
-  void NotifyImagesChanged();
+  void NotifyImageAdded();
   void NotifyImagesReady();
   void NotifyWeatherInfoUpdated();
 

ash/ambient/model/ambient_backend_model_observer.h

@@ -5,6 +5,7 @@
 #ifndef ASH_AMBIENT_MODEL_AMBIENT_BACKEND_MODEL_OBSERVER_H_
 #define ASH_AMBIENT_MODEL_AMBIENT_BACKEND_MODEL_OBSERVER_H_
 
+#include "ash/ambient/model/ambient_backend_model.h"
 #include "ash/public/cpp/ash_public_export.h"
 #include "base/observer_list_types.h"
 
@@ -18,10 +19,13 @@ class ASH_PUBLIC_EXPORT AmbientBackendModelObserver
   // Invoked when |topics| has been changed.
   virtual void OnTopicsChanged() {}
 
-  // Invoked when prev/current/next images changed.
-  virtual void OnImagesChanged() {}
+  // Invoked when a new image is added.
+  virtual void OnImageAdded() {}
 
-  // Invoked when enough images are loaded in memory to start ambient mode.
+  // Invoked when two images have been added. Two images are necessary to
+  // prevent screen burn in so that images can still change if further downloads
+  // fail. When the second image is added, this method is invoked after
+  // |OnImageAdded|.
   virtual void OnImagesReady() {}
 
   // Invoked when fetching images has failed and not enough images are present

ash/ambient/model/ambient_backend_model_unittest.cc

@@ -13,12 +13,14 @@
 #include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
-#include "base/scoped_observer.h"
+#include "base/scoped_observation.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_unittest_util.h"
 #include "ui/views/controls/image_view.h"
 
+#define EXPECT_NO_CALLS(args...) EXPECT_CALL(args).Times(0);
+
 namespace ash {
 
 namespace {
@@ -28,6 +30,8 @@ class MockAmbientBackendModelObserver : public AmbientBackendModelObserver {
   ~MockAmbientBackendModelObserver() override = default;
 
   MOCK_METHOD(void, OnImagesFailed, (), (override));
+  MOCK_METHOD(void, OnImagesReady, (), (override));
+  MOCK_METHOD(void, OnImageAdded, (), (override));
 };
 
 }  // namespace
@@ -94,6 +98,10 @@ class AmbientBackendModelTest : public AshTestBase {
     return ambient_backend_model_->GetNextImage();
   }
 
+  PhotoWithDetails GetCurrentImage() {
+    return ambient_backend_model_->GetCurrentImage();
+  }
+
   int failure_count() { return ambient_backend_model_->failures_; }
 
  private:
@@ -104,13 +112,15 @@ class AmbientBackendModelTest : public AshTestBase {
 TEST_F(AmbientBackendModelTest, AddFirstImage) {
   AddNTestImages(1);
 
-  EXPECT_TRUE(EqualsToTestImage(GetNextImage()));
+  EXPECT_TRUE(EqualsToTestImage(GetCurrentImage()));
+  EXPECT_TRUE(GetNextImage().IsNull());
 }
 
 // Test adding the second image.
 TEST_F(AmbientBackendModelTest, AddSecondImage) {
   AddNTestImages(1);
-  EXPECT_TRUE(EqualsToTestImage(GetNextImage()));
+  EXPECT_TRUE(EqualsToTestImage(GetCurrentImage()));
+  EXPECT_TRUE(GetNextImage().IsNull());
 
   AddNTestImages(1);
   EXPECT_TRUE(EqualsToTestImage(GetNextImage()));
@@ -140,10 +150,10 @@ TEST_F(AmbientBackendModelTest, ShouldReturnExpectedPhotoRefreshInterval) {
 TEST_F(AmbientBackendModelTest, ShouldNotifyObserversIfImagesFailed) {
   ambient_backend_model()->Clear();
   testing::NiceMock<MockAmbientBackendModelObserver> observer;
-  ScopedObserver<AmbientBackendModel, AmbientBackendModelObserver> scoped_obs{
-      &observer};
+  base::ScopedObservation<AmbientBackendModel, AmbientBackendModelObserver>
+      scoped_obs{&observer};
 
-  scoped_obs.Add(ambient_backend_model());
+  scoped_obs.Observe(ambient_backend_model());
 
   EXPECT_CALL(observer, OnImagesFailed).Times(1);
 
@@ -154,12 +164,12 @@ TEST_F(AmbientBackendModelTest, ShouldNotifyObserversIfImagesFailed) {
 
 TEST_F(AmbientBackendModelTest, ShouldResetFailuresOnAddImage) {
   testing::NiceMock<MockAmbientBackendModelObserver> observer;
-  ScopedObserver<AmbientBackendModel, AmbientBackendModelObserver> scoped_obs{
-      &observer};
+  base::ScopedObservation<AmbientBackendModel, AmbientBackendModelObserver>
+      scoped_obs{&observer};
 
-  scoped_obs.Add(ambient_backend_model());
+  scoped_obs.Observe(ambient_backend_model());
 
-  EXPECT_CALL(observer, OnImagesFailed).Times(0);
+  EXPECT_NO_CALLS(observer, OnImagesFailed);
 
   for (int i = 0; i < kMaxConsecutiveReadPhotoFailures - 1; i++) {
     ambient_backend_model()->AddImageFailure();
@@ -172,4 +182,35 @@ TEST_F(AmbientBackendModelTest, ShouldResetFailuresOnAddImage) {
   EXPECT_EQ(failure_count(), 0);
 }
 
+TEST_F(AmbientBackendModelTest, ShouldNotifyObserversOnImagesReady) {
+  testing::NiceMock<MockAmbientBackendModelObserver> observer;
+  base::ScopedObservation<AmbientBackendModel, AmbientBackendModelObserver>
+      scoped_obs{&observer};
+
+  scoped_obs.Observe(ambient_backend_model());
+
+  EXPECT_CALL(observer, OnImageAdded).Times(1);
+  EXPECT_NO_CALLS(observer, OnImagesReady);
+  AddNTestImages(1);
+
+  EXPECT_CALL(observer, OnImageAdded).Times(1);
+  EXPECT_CALL(observer, OnImagesReady).Times(1);
+  AddNTestImages(1);
+}
+
+TEST_F(AmbientBackendModelTest, ShouldNotifyObserversOnImageAdded) {
+  testing::NiceMock<MockAmbientBackendModelObserver> observer;
+  base::ScopedObservation<AmbientBackendModel, AmbientBackendModelObserver>
+      scoped_obs{&observer};
+
+  scoped_obs.Observe(ambient_backend_model());
+
+  EXPECT_CALL(observer, OnImagesReady).Times(1);
+  EXPECT_CALL(observer, OnImageAdded).Times(2);
+  AddNTestImages(2);
+
+  EXPECT_CALL(observer, OnImageAdded).Times(3);
+  AddNTestImages(3);
+}
+
 }  // namespace ash

ash/ambient/ui/glanceable_info_view.h

@@ -37,7 +37,6 @@ class GlanceableInfoView : public views::View,
 
   // AmbientBackendModelObserver:
   void OnWeatherInfoUpdated() override;
-  void OnImagesChanged() override {}
 
   void Show();
 

ash/ambient/ui/photo_view.cc

@@ -50,7 +50,7 @@ const char* PhotoView::GetClassName() const {
   return "PhotoView";
 }
 
-void PhotoView::OnImagesChanged() {
+void PhotoView::OnImageAdded() {
   // If NeedToAnimate() is true, will start transition animation and
   // UpdateImages() when animation completes. Otherwise, update images
   // immediately.
@@ -77,27 +77,32 @@ void PhotoView::Init() {
   }
 
   // Hides one image view initially for fade in animation.
-  image_views_[1]->layer()->SetOpacity(0.0f);
+  image_views_.back()->layer()->SetOpacity(0.0f);
 
   auto* model = delegate_->GetAmbientBackendModel();
   scoped_backend_model_observer_.Observe(model);
 
+  // |PhotoView::Init| is called after
+  // |AmbientBackendModelObserver::OnImagesReady| has been called.
+  // |AmbientBackendModel| has two images ready and views should be constructed
+  // for each one.
   UpdateImage(model->GetCurrentImage());
+  UpdateImage(model->GetNextImage());
 }
 
 void PhotoView::UpdateImage(const PhotoWithDetails& next_image) {
   if (next_image.photo.isNull())
     return;
 
-  image_views_[image_index_]->UpdateImage(next_image.photo,
-                                          next_image.related_photo);
-  image_views_[image_index_]->UpdateImageDetails(
-      base::UTF8ToUTF16(next_image.details));
+  image_views_.at(image_index_)
+      ->UpdateImage(next_image.photo, next_image.related_photo);
+  image_views_.at(image_index_)
+      ->UpdateImageDetails(base::UTF8ToUTF16(next_image.details));
   image_index_ = 1 - image_index_;
 }
 
 void PhotoView::StartTransitionAnimation() {
-  ui::Layer* visible_layer = image_views_[image_index_]->layer();
+  ui::Layer* visible_layer = image_views_.at(image_index_)->layer();
   {
     ui::ScopedLayerAnimationSettings animation(visible_layer->GetAnimator());
     animation.SetTransitionDuration(kAnimationDuration);
@@ -113,7 +118,7 @@ void PhotoView::StartTransitionAnimation() {
     visible_layer->SetOpacity(0.0f);
   }
 
-  ui::Layer* invisible_layer = image_views_[1 - image_index_]->layer();
+  ui::Layer* invisible_layer = image_views_.at(1 - image_index_)->layer();
   {
     ui::ScopedLayerAnimationSettings animation(invisible_layer->GetAnimator());
     animation.SetTransitionDuration(kAnimationDuration);
@@ -140,11 +145,11 @@ void PhotoView::OnImplicitAnimationsCompleted() {
 bool PhotoView::NeedToAnimateTransition() const {
   // Can do transition animation if both two images in |images_unscaled_| are
   // not nullptr. Check the image index 1 is enough.
-  return !image_views_[1]->GetCurrentImage().isNull();
+  return !image_views_.back()->GetCurrentImage().isNull();
 }
 
-const gfx::ImageSkia& PhotoView::GetCurrentImagesForTesting() {
-  return image_views_[image_index_]->GetCurrentImage();
+const gfx::ImageSkia& PhotoView::GetVisibleImageForTesting() {
+  return image_views_.at(image_index_)->GetCurrentImage();
 }
 
 }  // namespace ash

ash/ambient/ui/photo_view.h

@@ -5,6 +5,7 @@
 #ifndef ASH_AMBIENT_UI_PHOTO_VIEW_H_
 #define ASH_AMBIENT_UI_PHOTO_VIEW_H_
 
+#include <array>
 #include <memory>
 
 #include "ash/ambient/model/ambient_backend_model.h"
@@ -40,7 +41,7 @@ class ASH_EXPORT PhotoView : public views::View,
   const char* GetClassName() const override;
 
   // AmbientBackendModelObserver:
-  void OnImagesChanged() override;
+  void OnImageAdded() override;
 
   // ui::ImplicitAnimationObserver:
   void OnImplicitAnimationsCompleted() override;
@@ -57,14 +58,14 @@ class ASH_EXPORT PhotoView : public views::View,
   // Return if can start transition animation.
   bool NeedToAnimateTransition() const;
 
-  const gfx::ImageSkia& GetCurrentImagesForTesting();
+  const gfx::ImageSkia& GetVisibleImageForTesting();
 
   // Note that we should be careful when using |delegate_|, as there is no
   // strong guarantee on the life cycle.
   AmbientViewDelegate* const delegate_ = nullptr;
 
   // Image containers used for animation. Owned by view hierarchy.
-  AmbientBackgroundImageView* image_views_[2]{nullptr, nullptr};
+  std::array<AmbientBackgroundImageView*, 2> image_views_{nullptr, nullptr};
 
   // The index of |image_views_| to update the next image.
   int image_index_ = 0;