Reland "ambient: start when two images loaded"

This is a reland of 581c91ec

Original change's description:
> ambient: start when two images loaded
>
> Construct ambient widget when two images are in memory to prevent burn
> in.
>
> BUG=b:167332126
>
> Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
> Change-Id: I520ada00429125d35bda9ef66089ca9a9c8d4440
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2440306
> Commit-Queue: Jeffrey Young <cowmoo@chromium.org>
> Reviewed-by: Tao Wu <wutao@chromium.org>
> Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#814711}

Bug: b:167332126
Change-Id: I7fe0cca3ecb15262666eee1067f57bc21b150468
Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2459399Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Commit-Queue: Jeffrey Young <cowmoo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815206}

ash/ambient/ambient_controller.cc

@@ -519,9 +519,8 @@ AmbientBackendModel* AmbientController::GetAmbientBackendModel() {
   return ambient_photo_controller_.ambient_backend_model();
 }
 
-void AmbientController::OnImagesChanged() {
-  if (!container_view_)
-    CreateAndShowWidget();
+void AmbientController::OnImagesReady() {
+  CreateAndShowWidget();
 }
 
 std::unique_ptr<AmbientContainerView> AmbientController::CreateContainerView() {

ash/ambient/ambient_controller.h

@@ -126,7 +126,7 @@ class ASH_EXPORT AmbientController
   void DismissUI();
 
   // AmbientBackendModelObserver overrides:
-  void OnImagesChanged() override;
+  void OnImagesReady() override;
 
   // Initializes the |container_view_|. Called in |CreateWidget()| to create the
   // contents view.

ash/ambient/ambient_photo_controller.cc

@@ -397,14 +397,8 @@ void AmbientPhotoController::ScheduleFetchTopics(bool backoff) {
 }
 
 void AmbientPhotoController::ScheduleRefreshImage() {
-  base::TimeDelta refresh_interval;
-  if (!ambient_backend_model_.ShouldFetchImmediately())
-    refresh_interval = kPhotoRefreshInterval;
-
-  // |photo_refresh_timer_| will start immediately if ShouldFetchImmediately()
-  // is true.
   photo_refresh_timer_.Start(
-      FROM_HERE, refresh_interval,
+      FROM_HERE, ambient_backend_model_.GetPhotoRefreshInterval(),
       base::BindOnce(&AmbientPhotoController::FetchPhotoRawData,
                      weak_factory_.GetWeakPtr()));
 }
@@ -466,7 +460,6 @@ void AmbientPhotoController::OnScreenUpdateInfoFetched(
     }
     return;
   }
-
   fetch_topic_retry_backoff_.InformOfRequest(/*succeeded=*/true);
   ambient_backend_model_.AppendTopics(screen_update.next_topics);
   StartDownloadingWeatherConditionIcon(screen_update.weather_info);
@@ -518,11 +511,7 @@ void AmbientPhotoController::FetchPhotoRawData() {
 }
 
 void AmbientPhotoController::TryReadPhotoRawData() {
-  auto on_done =
-      base::BindRepeating(&AmbientPhotoController::OnAllPhotoRawDataAvailable,
-                          weak_factory_.GetWeakPtr(),
-                          /*from_downloading=*/false);
-
+  ResetImageData();
   // Stop reading from cache after the max number of retries.
   if (retries_to_read_from_cache_ == 0) {
     if (backup_retries_to_read_from_cache_ == 0) {
@@ -547,6 +536,10 @@ void AmbientPhotoController::TryReadPhotoRawData() {
     // Try to read a backup image.
     auto photo_data = std::make_unique<std::string>();
     auto* photo_data_ptr = photo_data.get();
+    auto on_done = base::BindRepeating(
+        &AmbientPhotoController::OnAllPhotoRawDataAvailable,
+        weak_factory_.GetWeakPtr(), /*from_downloading=*/false);
+
     task_runner_->PostTaskAndReply(
         FROM_HERE,
         base::BindOnce(
@@ -571,12 +564,19 @@ void AmbientPhotoController::TryReadPhotoRawData() {
 
   --retries_to_read_from_cache_;
   std::string file_name = base::NumberToString(cache_index_for_display_);
+
   ++cache_index_for_display_;
   if (cache_index_for_display_ == kMaxNumberOfCachedImages)
     cache_index_for_display_ = 0;
 
   auto photo_data = std::make_unique<std::string>();
   auto photo_details = std::make_unique<std::string>();
+
+  auto on_done =
+      base::BindRepeating(&AmbientPhotoController::OnAllPhotoRawDataAvailable,
+                          weak_factory_.GetWeakPtr(),
+                          /*from_downloading=*/false);
+
   task_runner_->PostTaskAndReply(
       FROM_HERE,
       base::BindOnce(
@@ -639,7 +639,8 @@ void AmbientPhotoController::OnAllPhotoRawDataAvailable(bool from_downloading) {
   auto on_done = base::BarrierClosure(
       num_callbacks,
       base::BindOnce(&AmbientPhotoController::OnAllPhotoDecoded,
-                     weak_factory_.GetWeakPtr(), from_downloading));
+                     weak_factory_.GetWeakPtr(), from_downloading,
+                     /*hash=*/base::SHA1HashString(*image_data_)));
 
   task_runner_->PostTaskAndReply(
       FROM_HERE,
@@ -673,7 +674,7 @@ void AmbientPhotoController::DecodePhotoRawData(
   image_decoder_->Decode(
       image_bytes, base::BindOnce(&AmbientPhotoController::OnPhotoDecoded,
                                   weak_factory_.GetWeakPtr(), from_downloading,
-                                  is_related_image, on_done));
+                                  is_related_image, std::move(on_done)));
 }
 
 void AmbientPhotoController::OnPhotoDecoded(bool from_downloading,
@@ -688,7 +689,8 @@ void AmbientPhotoController::OnPhotoDecoded(bool from_downloading,
   std::move(on_done).Run();
 }
 
-void AmbientPhotoController::OnAllPhotoDecoded(bool from_downloading) {
+void AmbientPhotoController::OnAllPhotoDecoded(bool from_downloading,
+                                               const std::string& hash) {
   if (image_.isNull()) {
     LOG(WARNING) << "Image decoding failed";
     if (from_downloading)
@@ -697,6 +699,10 @@ void AmbientPhotoController::OnAllPhotoDecoded(bool from_downloading) {
     // Try to read from cache when failure happens.
     TryReadPhotoRawData();
     return;
+  } else if (ambient_backend_model_.HashMatchesNextImage(hash)) {
+    LOG(WARNING) << "Skipping loading duplicate image.";
+    TryReadPhotoRawData();
+    return;
   }
 
   retries_to_read_from_cache_ = kMaxNumberOfCachedImages;
@@ -709,6 +715,7 @@ void AmbientPhotoController::OnAllPhotoDecoded(bool from_downloading) {
   detailed_photo.photo = image_;
   detailed_photo.related_photo = related_image_;
   detailed_photo.details = *image_details_;
+  detailed_photo.hash = hash;
 
   ResetImageData();
 

ash/ambient/ambient_photo_controller.h

@@ -166,7 +166,7 @@ class ASH_EXPORT AmbientPhotoController : public AmbientBackendModelObserver {
                       base::RepeatingClosure on_done,
                       const gfx::ImageSkia& image);
 
-  void OnAllPhotoDecoded(bool from_downloading);
+  void OnAllPhotoDecoded(bool from_downloading, const std::string& hash);
 
   void StartDownloadingWeatherConditionIcon(
       const base::Optional<WeatherInfo>& weather_info);

ash/ambient/ambient_photo_controller_unittest.cc

@@ -10,6 +10,7 @@
 #include "ash/ambient/ambient_constants.h"
 #include "ash/ambient/ambient_controller.h"
 #include "ash/ambient/model/ambient_backend_model.h"
+#include "ash/ambient/model/ambient_backend_model_observer.h"
 #include "ash/ambient/test/ambient_ash_test_base.h"
 #include "ash/public/cpp/ambient/ambient_backend_controller.h"
 #include "ash/public/cpp/ambient/fake_ambient_backend_controller_impl.h"
@@ -24,13 +25,28 @@
 #include "base/hash/sha1.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
+#include "base/scoped_observer.h"
 #include "base/system/sys_info.h"
 #include "base/test/bind_test_util.h"
 #include "base/timer/timer.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "ui/gfx/image/image_skia.h"
 
 namespace ash {
 
+namespace {
+class MockAmbientBackendModelObserver : public AmbientBackendModelObserver {
+ public:
+  MockAmbientBackendModelObserver() = default;
+  ~MockAmbientBackendModelObserver() override = default;
+
+  // AmbientBackendModelObserver:
+  MOCK_METHOD(void, OnImagesChanged, (), (override));
+  MOCK_METHOD(void, OnImagesReady, (), (override));
+};
+
+}  // namespace
+
 class AmbientPhotoControllerTest : public AmbientAshTestBase {
  public:
   // AmbientAshTestBase:
@@ -144,30 +160,32 @@ TEST_F(AmbientPhotoControllerTest, ShouldUpdatePhotoPeriodically) {
 TEST_F(AmbientPhotoControllerTest, ShouldSaveImagesOnDisk) {
   base::FilePath ambient_image_path = GetCacheDir();
 
-  // Start to refresh images. It will download a test image and write it in
-  // |ambient_image_path| in a delayed task.
+  // Start to refresh images. It will download two images immediately and write
+  // them in |ambient_image_path|. It will also download one more image after
+  // fast forward. It will also download the related images and not cache them.
   photo_controller()->StartScreenUpdate();
   FastForwardToNextImage();
 
-  // Count files and directories in ambient_image_path. There should only be
-  // four files that were just created to save image files for this ambient mode
+  // Count files and directories in ambient_image_path. There should be six
+  // files that were just created to save image files for this ambient mode
   // session.
   EXPECT_TRUE(base::PathExists(ambient_image_path));
   auto file_paths = GetFilePathsInDir(ambient_image_path);
-  // Two image files and two attribution files.
-  EXPECT_EQ(file_paths.size(), 4u);
+  // Three image files and three attribution files.
+  EXPECT_EQ(file_paths.size(), 6u);
   for (auto& path : file_paths) {
     // No sub directories.
     EXPECT_FALSE(base::DirectoryExists(path));
   }
 }
 
-// Test that image is save and will be deleted when stopping ambient mode.
+// Test that image is save and will not be deleted when stopping ambient mode.
 TEST_F(AmbientPhotoControllerTest, ShouldNotDeleteImagesOnDisk) {
   base::FilePath ambient_image_path = GetCacheDir();
 
-  // Start to refresh images. It will download a test image and write it in
-  // |ambient_image_path| in a delayed task.
+  // Start to refresh images. It will download two images immediately and write
+  // them in |ambient_image_path|. It will also download one more image after
+  // fast forward. It will also download the related images and not cache them.
   photo_controller()->StartScreenUpdate();
   FastForwardToNextImage();
 
@@ -186,13 +204,13 @@ TEST_F(AmbientPhotoControllerTest, ShouldNotDeleteImagesOnDisk) {
   image = photo_controller()->ambient_backend_model()->GetNextImage();
   EXPECT_TRUE(image.IsNull());
 
-  // Count files and directories in ambient_image_path. There should only be
-  // four files that were just created to save image files for the prior ambient
-  // mode session.
+  // Count files and directories in ambient_image_path. There should be six
+  // files that were just created to save image files for the prior ambient mode
+  // session.
   EXPECT_TRUE(base::PathExists(ambient_image_path));
   auto file_paths = GetFilePathsInDir(ambient_image_path);
-  // Two image files and two attribution files.
-  EXPECT_EQ(file_paths.size(), 4u);
+  // Three image files and three attribution files.
+  EXPECT_EQ(file_paths.size(), 6u);
   for (auto& path : file_paths) {
     // No sub directories.
     EXPECT_FALSE(base::DirectoryExists(path));
@@ -397,6 +415,37 @@ TEST_F(AmbientPhotoControllerTest,
   }
 }
 
+TEST_F(AmbientPhotoControllerTest, ShouldNotLoadDuplicateImages) {
+  testing::NiceMock<MockAmbientBackendModelObserver> mock_backend_observer;
+  ScopedObserver<AmbientBackendModel, AmbientBackendModelObserver>
+      scoped_observer{&mock_backend_observer};
+
+  scoped_observer.Add(photo_controller()->ambient_backend_model());
+
+  // All images downloaded will be identical.
+  SetUrlLoaderData(std::make_unique<std::string>("image data"));
+
+  photo_controller()->StartScreenUpdate();
+  // Run the clock so the first photo is loaded.
+  FastForwardTiny();
+
+  // Should contain hash of downloaded data.
+  EXPECT_TRUE(photo_controller()->ambient_backend_model()->HashMatchesNextImage(
+      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);
+  SetUrlLoaderData(std::make_unique<std::string>("image data 2"));
+  FastForwardToNextImage();
+
+  // Second image should have been loaded.
+  EXPECT_TRUE(photo_controller()->ambient_backend_model()->HashMatchesNextImage(
+      base::SHA1HashString("image data 2")));
+  EXPECT_TRUE(photo_controller()->ambient_backend_model()->ImagesReady());
+}
+
 TEST_F(AmbientPhotoControllerTest, ShouldStartToRefreshWeather) {
   auto* model = photo_controller()->ambient_backend_model();
   EXPECT_FALSE(model->show_celsius());

ash/ambient/model/ambient_backend_model.cc

@@ -56,9 +56,8 @@ void AmbientBackendModel::AppendTopics(
   NotifyTopicsChanged();
 }
 
-bool AmbientBackendModel::ShouldFetchImmediately() const {
-  // Prefetch one image |next_image_| for photo transition animation.
-  return current_image_.IsNull() || next_image_.IsNull();
+bool AmbientBackendModel::ImagesReady() const {
+  return !current_image_.IsNull() && !next_image_.IsNull();
 }
 
 void AmbientBackendModel::AddNextImage(
@@ -67,6 +66,7 @@ void AmbientBackendModel::AddNextImage(
     current_image_ = photo_with_details;
   } else if (next_image_.IsNull()) {
     next_image_ = photo_with_details;
+    NotifyImagesReady();
   } else {
     current_image_ = next_image_;
     next_image_ = photo_with_details;
@@ -75,8 +75,12 @@ void AmbientBackendModel::AddNextImage(
   NotifyImagesChanged();
 }
 
+bool AmbientBackendModel::HashMatchesNextImage(const std::string& hash) const {
+  return GetNextImage().hash == hash;
+}
+
 base::TimeDelta AmbientBackendModel::GetPhotoRefreshInterval() {
-  if (ShouldFetchImmediately())
+  if (!ImagesReady())
     return base::TimeDelta();
 
   return photo_refresh_interval_;
@@ -125,6 +129,11 @@ void AmbientBackendModel::NotifyImagesChanged() {
     observer.OnImagesChanged();
 }
 
+void AmbientBackendModel::NotifyImagesReady() {
+  for (auto& observer : observers_)
+    observer.OnImagesReady();
+}
+
 void AmbientBackendModel::NotifyWeatherInfoUpdated() {
   for (auto& observer : observers_)
     observer.OnWeatherInfoUpdated();

ash/ambient/model/ambient_backend_model.h

@@ -36,6 +36,8 @@ struct ASH_EXPORT PhotoWithDetails {
   gfx::ImageSkia photo;
   gfx::ImageSkia related_photo;
   std::string details;
+  // Hash of this image data. Used for de-duping images.
+  std::string hash;
 };
 
 // Stores necessary information fetched from the backdrop server to render
@@ -54,12 +56,15 @@ class ASH_EXPORT AmbientBackendModel {
   void AppendTopics(const std::vector<AmbientModeTopic>& topics);
   const std::vector<AmbientModeTopic>& topics() const { return topics_; }
 
-  // Prefetch one more image for ShowNextImage animations.
-  bool ShouldFetchImmediately() const;
+  // If enough images are loaded to start ambient mode.
+  bool ImagesReady() const;
 
   // 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;
+
   // Get/Set the photo refresh interval.
   base::TimeDelta GetPhotoRefreshInterval();
   void SetPhotoRefreshInterval(base::TimeDelta interval);
@@ -69,6 +74,7 @@ class ASH_EXPORT AmbientBackendModel {
 
   // Get images from local storage. Could be null image.
   const PhotoWithDetails& GetNextImage() const;
+  const PhotoWithDetails& GetCurrentImage() const { return current_image_; }
 
   // Updates the weather information and notifies observers if the icon image is
   // not null.
@@ -95,6 +101,7 @@ class ASH_EXPORT AmbientBackendModel {
 
   void NotifyTopicsChanged();
   void NotifyImagesChanged();
+  void NotifyImagesReady();
   void NotifyWeatherInfoUpdated();
 
   std::vector<AmbientModeTopic> topics_;
@@ -103,9 +110,6 @@ class ASH_EXPORT AmbientBackendModel {
   PhotoWithDetails current_image_;
   PhotoWithDetails next_image_;
 
-  // The index of currently shown image.
-  int current_image_index_ = 0;
-
   // Current weather information.
   gfx::ImageSkia weather_condition_icon_;
   float temperature_fahrenheit_ = 0.0f;

ash/ambient/model/ambient_backend_model_observer.h

@@ -21,6 +21,9 @@ class ASH_PUBLIC_EXPORT AmbientBackendModelObserver
   // Invoked when prev/current/next images changed.
   virtual void OnImagesChanged() {}
 
+  // Invoked when enough images are loaded in memory to start ambient mode.
+  virtual void OnImagesReady() {}
+
   // Invoked when the weather info (condition icon or temperature) stored in the
   // model has been updated.
   virtual void OnWeatherInfoUpdated() {}

ash/ambient/test/ambient_ash_test_base.cc

@@ -50,11 +50,14 @@ class TestAmbientURLLoaderImpl : public AmbientURLLoader {
   void Download(
       const std::string& url,
       network::SimpleURLLoader::BodyAsStringCallback callback) override {
+    // Reply with a unique string each time to avoid check to skip loading
+    // duplicate images.
+    std::unique_ptr<std::string> data = std::make_unique<std::string>(
+        data_ ? *data_ : base::StringPrintf("test_image_%i", count_));
+    count_++;
     // Pretend to respond asynchronously.
     base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE,
-        base::BindOnce(std::move(callback),
-                       std::make_unique<std::string>(data_ ? *data_ : "test")),
+        FROM_HERE, base::BindOnce(std::move(callback), std::move(data)),
         base::TimeDelta::FromMilliseconds(1));
   }
   void DownloadToFile(
@@ -86,6 +89,7 @@ class TestAmbientURLLoaderImpl : public AmbientURLLoader {
     base::ScopedBlockingCall blocking(FROM_HERE, base::BlockingType::MAY_BLOCK);
     return base::WriteFile(file_path, data);
   }
+  int count_ = 0;
   // If not null, will return this data.
   std::unique_ptr<std::string> data_;
 };

ash/ambient/ui/photo_view.cc

@@ -35,7 +35,6 @@ void ReportSmoothness(int value) {
   base::UmaHistogramPercentage(kPhotoTransitionSmoothness, value);
 }
 
-
 }  // namespace
 
 // PhotoView ------------------------------------------------------------------
@@ -62,7 +61,7 @@ void PhotoView::OnImagesChanged() {
     return;
   }
 
-  UpdateImages();
+  UpdateImage(delegate_->GetAmbientBackendModel()->GetNextImage());
 }
 
 void PhotoView::Init() {
@@ -82,12 +81,13 @@ void PhotoView::Init() {
   // Hides one image view initially for fade in animation.
   image_views_[1]->layer()->SetOpacity(0.0f);
 
-  delegate_->GetAmbientBackendModel()->AddObserver(this);
+  auto* model = delegate_->GetAmbientBackendModel();
+  model->AddObserver(this);
+
+  UpdateImage(model->GetCurrentImage());
 }
 
-void PhotoView::UpdateImages() {
-  auto* model = delegate_->GetAmbientBackendModel();
-  auto& next_image = model->GetNextImage();
+void PhotoView::UpdateImage(const PhotoWithDetails& next_image) {
   if (next_image.photo.isNull())
     return;
 
@@ -135,7 +135,7 @@ void PhotoView::StartTransitionAnimation() {
 }
 
 void PhotoView::OnImplicitAnimationsCompleted() {
-  UpdateImages();
+  UpdateImage(delegate_->GetAmbientBackendModel()->GetNextImage());
   delegate_->OnPhotoTransitionAnimationCompleted();
 }
 

ash/ambient/ui/photo_view.h

@@ -22,6 +22,7 @@ namespace ash {
 
 class AmbientBackgroundImageView;
 class AmbientViewDelegate;
+struct PhotoWithDetails;
 
 // View to display photos in ambient mode.
 class ASH_EXPORT PhotoView : public views::View,
@@ -47,7 +48,7 @@ class ASH_EXPORT PhotoView : public views::View,
 
   void Init();
 
-  void UpdateImages();
+  void UpdateImage(const PhotoWithDetails& image);
 
   void StartTransitionAnimation();
 

ash/public/cpp/ambient/fake_ambient_backend_controller_impl.cc

@@ -83,14 +83,17 @@ FakeAmbientBackendControllerImpl::~FakeAmbientBackendControllerImpl() = default;
 void FakeAmbientBackendControllerImpl::FetchScreenUpdateInfo(
     int num_topics,
     OnScreenUpdateInfoFetchedCallback callback) {
-  ash::AmbientModeTopic topic;
-  topic.url = kFakeUrl;
-  topic.details = kFakeDetails;
-  topic.related_image_url = kFakeUrl;
-  topic.topic_type = AmbientModeTopicType::kCulturalInstitute;
-
   ash::ScreenUpdate update;
-  update.next_topics.emplace_back(topic);
+
+  for (int i = 0; i < num_topics; i++) {
+    ash::AmbientModeTopic topic;
+    topic.url = kFakeUrl;
+    topic.details = kFakeDetails;
+    topic.related_image_url = kFakeUrl;
+    topic.topic_type = AmbientModeTopicType::kCulturalInstitute;
+
+    update.next_topics.emplace_back(topic);
+  }
 
   // Only respond weather info when there is no active weather testing.
   if (!weather_info_) {