[M87 merge] Reland "ambient: start when two images loaded"
This is a reland of 581c91ec3cd79a1b88d161276aadd144771d012e
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 <[email protected]>
> Reviewed-by: Tao Wu <[email protected]>
> Reviewed-by: Xiaohui Chen <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#814711}
(cherry picked from commit 01cf328d5f1824d7e11d3f4c407c3b41084e9dac)
Bug: b:167332126
Change-Id: I7fe0cca3ecb15262666eee1067f57bc21b150468
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2459399
Reviewed-by: Xiaohui Chen <[email protected]>
Commit-Queue: Jeffrey Young <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#815206}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2465064
Reviewed-by: Tao Wu <[email protected]>
Cr-Commit-Position: refs/branch-heads/4280@{#282}
Cr-Branched-From: ea420fb963f9658c9969b6513c56b8f47efa1a2a-refs/heads/master@{#812852}
diff --git a/ash/ambient/ambient_controller.cc b/ash/ambient/ambient_controller.cc
index 35d6b1b..7e95dfa 100644
--- a/ash/ambient/ambient_controller.cc
+++ b/ash/ambient/ambient_controller.cc
@@ -532,9 +532,8 @@
return ambient_photo_controller_.ambient_backend_model();
}
-void AmbientController::OnImagesChanged() {
- if (!container_view_)
- CreateAndShowWidget();
+void AmbientController::OnImagesReady() {
+ CreateAndShowWidget();
}
std::unique_ptr<AmbientContainerView> AmbientController::CreateContainerView() {
diff --git a/ash/ambient/ambient_controller.h b/ash/ambient/ambient_controller.h
index 7926945..0b38734 100644
--- a/ash/ambient/ambient_controller.h
+++ b/ash/ambient/ambient_controller.h
@@ -126,7 +126,7 @@
void DismissUI();
// AmbientBackendModelObserver overrides:
- void OnImagesChanged() override;
+ void OnImagesReady() override;
// Initializes the |container_view_|. Called in |CreateWidget()| to create the
// contents view.
diff --git a/ash/ambient/ambient_photo_controller.cc b/ash/ambient/ambient_photo_controller.cc
index bc5d11e..797ba5a 100644
--- a/ash/ambient/ambient_photo_controller.cc
+++ b/ash/ambient/ambient_photo_controller.cc
@@ -397,14 +397,8 @@
}
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()));
}
@@ -464,7 +458,6 @@
}
return;
}
-
fetch_topic_retry_backoff_.InformOfRequest(/*succeeded=*/true);
ambient_backend_model_.AppendTopics(screen_update.next_topics);
StartDownloadingWeatherConditionIcon(screen_update.weather_info);
@@ -516,11 +509,7 @@
}
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) {
@@ -545,6 +534,10 @@
// 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(
@@ -569,12 +562,19 @@
--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 @@
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 @@
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 @@
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 is null";
if (from_downloading)
@@ -697,6 +699,10 @@
// 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 @@
detailed_photo.photo = image_;
detailed_photo.related_photo = related_image_;
detailed_photo.details = *image_details_;
+ detailed_photo.hash = hash;
ResetImageData();
diff --git a/ash/ambient/ambient_photo_controller.h b/ash/ambient/ambient_photo_controller.h
index d8ef32a2..6bce4e88 100644
--- a/ash/ambient/ambient_photo_controller.h
+++ b/ash/ambient/ambient_photo_controller.h
@@ -166,7 +166,7 @@
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);
diff --git a/ash/ambient/ambient_photo_controller_unittest.cc b/ash/ambient/ambient_photo_controller_unittest.cc
index c56d81c..e86d68b 100644
--- a/ash/ambient/ambient_photo_controller_unittest.cc
+++ b/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, 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 @@
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, 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());
diff --git a/ash/ambient/model/ambient_backend_model.cc b/ash/ambient/model/ambient_backend_model.cc
index c9f7374..3e3b0f34 100644
--- a/ash/ambient/model/ambient_backend_model.cc
+++ b/ash/ambient/model/ambient_backend_model.cc
@@ -56,9 +56,8 @@
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 @@
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 @@
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 @@
observer.OnImagesChanged();
}
+void AmbientBackendModel::NotifyImagesReady() {
+ for (auto& observer : observers_)
+ observer.OnImagesReady();
+}
+
void AmbientBackendModel::NotifyWeatherInfoUpdated() {
for (auto& observer : observers_)
observer.OnWeatherInfoUpdated();
diff --git a/ash/ambient/model/ambient_backend_model.h b/ash/ambient/model/ambient_backend_model.h
index bdad83d..27ae662 100644
--- a/ash/ambient/model/ambient_backend_model.h
+++ b/ash/ambient/model/ambient_backend_model.h
@@ -36,6 +36,8 @@
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 @@
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 @@
// 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 @@
void NotifyTopicsChanged();
void NotifyImagesChanged();
+ void NotifyImagesReady();
void NotifyWeatherInfoUpdated();
std::vector<AmbientModeTopic> topics_;
@@ -103,9 +110,6 @@
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;
diff --git a/ash/ambient/model/ambient_backend_model_observer.h b/ash/ambient/model/ambient_backend_model_observer.h
index 6fea709..2468235 100644
--- a/ash/ambient/model/ambient_backend_model_observer.h
+++ b/ash/ambient/model/ambient_backend_model_observer.h
@@ -21,6 +21,9 @@
// 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() {}
diff --git a/ash/ambient/test/ambient_ash_test_base.cc b/ash/ambient/test/ambient_ash_test_base.cc
index 9b86a31..4d66fe3b 100644
--- a/ash/ambient/test/ambient_ash_test_base.cc
+++ b/ash/ambient/test/ambient_ash_test_base.cc
@@ -50,11 +50,14 @@
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 @@
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_;
};
diff --git a/ash/ambient/ui/photo_view.cc b/ash/ambient/ui/photo_view.cc
index 6b5cd5a..c566f16 100644
--- a/ash/ambient/ui/photo_view.cc
+++ b/ash/ambient/ui/photo_view.cc
@@ -35,7 +35,6 @@
base::UmaHistogramPercentage(kPhotoTransitionSmoothness, value);
}
-
} // namespace
// PhotoView ------------------------------------------------------------------
@@ -62,7 +61,7 @@
return;
}
- UpdateImages();
+ UpdateImage(delegate_->GetAmbientBackendModel()->GetNextImage());
}
void PhotoView::Init() {
@@ -82,12 +81,13 @@
// 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::OnImplicitAnimationsCompleted() {
- UpdateImages();
+ UpdateImage(delegate_->GetAmbientBackendModel()->GetNextImage());
delegate_->OnPhotoTransitionAnimationCompleted();
}
diff --git a/ash/ambient/ui/photo_view.h b/ash/ambient/ui/photo_view.h
index 457b043..9ecdf88 100644
--- a/ash/ambient/ui/photo_view.h
+++ b/ash/ambient/ui/photo_view.h
@@ -22,6 +22,7 @@
class AmbientBackgroundImageView;
class AmbientViewDelegate;
+struct PhotoWithDetails;
// View to display photos in ambient mode.
class ASH_EXPORT PhotoView : public views::View,
@@ -47,7 +48,7 @@
void Init();
- void UpdateImages();
+ void UpdateImage(const PhotoWithDetails& image);
void StartTransitionAnimation();
diff --git a/ash/public/cpp/ambient/fake_ambient_backend_controller_impl.cc b/ash/public/cpp/ambient/fake_ambient_backend_controller_impl.cc
index f54c394..ed52158 100644
--- a/ash/public/cpp/ambient/fake_ambient_backend_controller_impl.cc
+++ b/ash/public/cpp/ambient/fake_ambient_backend_controller_impl.cc
@@ -83,14 +83,17 @@
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_) {
