Skip to content

Commit

Permalink
Revert "Add display change observer."
Browse files Browse the repository at this point in the history
This reverts commit 7ae3611.

Reason for revert: cast_shell+browsertests failing on Cast [Audio] Linux
 First failure https://ci.chromium.org/ui/p/chromium/builders/ci/Cast%20Audio%20Linux/103652/overview

Example crash stack
BrowserTestBase received signal: Segmentation fault. Backtrace:
#0 0x55bcce76b6a9 base::debug::CollectStackTrace()
#1 0x55bcce6dfda3 base::debug::StackTrace::StackTrace()
#2 0x55bcd0fb5cac content::(anonymous namespace)::DumpStackTraceSignalHandler()
#3 0x7fd872d664c0 (/lib/x86_64-linux-gnu/libc-2.23.so+0x354bf)
#4 0x55bcccba88b0 _ZNSt3__17find_ifINS_11__wrap_iterIPKN4base8internal24UncheckedObserverAdapterEEEZNS3_23ProjectedUnaryPredicateIZNKS2_12ObserverListIN3net16MDnsListenerImplELb0ELb1ES4_E11HasObserverEPKSB_EUlRKT_E_NS2_8identityEEEDaRSF_RT0_EUlOSF_E_EESF_SF_SF_SL_
#5 0x55bcccba889e _ZN4base6ranges7find_ifINSt3__111__wrap_iterIPKNS_8internal24UncheckedObserverAdapterEEEZNKS_12ObserverListIN3net16MDnsListenerImplELb0ELb1ES5_E11HasObserverEPKSB_EUlRKT_E_NS_8identityENS2_26random_access_iterator_tagEEEDaSF_SF_T0_T1_
#6 0x55bcccba6dca base::ObserverList<>::RemoveObserver()
#7 0x55bccd28a44e chromecast::DisplayConfiguratorObserver::~DisplayConfiguratorObserver()
#8 0x55bccd26fbcd chromecast::shell::CastBrowserMainParts::~CastBrowserMainParts()
#9 0x55bccd26fc7c chromecast::shell::CastBrowserMainParts::~CastBrowserMainParts()
#10 0x55bccd3a6836 content::BrowserMainLoop::~BrowserMainLoop()
#11 0x55bccd3a6920 content::BrowserMainLoop::~BrowserMainLoop()
#12 0x55bccd3a9c06 content::BrowserMainRunnerImpl::Shutdown()
#13 0x55bccd3a65aa content::BrowserMain()
#14 0x55bcce68bc80 content::RunBrowserProcessMain()
#15 0x55bcce68c9a6 content::ContentMainRunnerImpl::RunBrowser()
#16 0x55bcce68c62c content::ContentMainRunnerImpl::Run()
#17 0x55bcce68ad93 content::RunContentProcess()
#18 0x55bcce68b49a content::ContentMain()
#19 0x55bcd0fb50e1 content::BrowserTestBase::SetUp()
#20 0x55bccc9fdf90 chromecast::WebviewTest::SetUp()
#21 0x55bcce69a89c testing::Test::Run()
#22 0x55bcce69adb8 testing::TestInfo::Run()
#23 0x55bcce69b27d testing::TestSuite::Run()
#24 0x55bcce6a2233 testing::internal::UnitTestImpl::RunAllTests()
#25 0x55bcce6a1f87 testing::UnitTest::Run()
#26 0x55bcd0fa57ef base::TestSuite::Run()
#27 0x55bccc9de2a4 chromecast::shell::CastTestLauncherDelegate::RunTestSuite()
#28 0x55bcd0fb930e content::LaunchTests()
#29 0x55bccc9de23c main
#30 0x7fd872d51840 __libc_start_main
#31 0x55bccc9de12a _start

Original change's description:
> Add display change observer.
>
> Centralize the logic to force a repaint post display changes in an
> observer. This way the complexity of refreshing the display is
> handled in the observer rather than in every piece of code that
> needs to update the display state.
>
> Bug: b/180040068
> Test: Ran cast_shell on the desktop to verify there are no crashes
> Change-Id: I022850322c8b9177463bdc0526121016ebf0f330
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2727577
> Reviewed-by: Daniel Nicoara <dnicoara@chromium.org>
> Commit-Queue: Shiv Sakhuja <shivsak@google.com>
> Cr-Commit-Position: refs/heads/master@{#873512}

Bug: b/180040068
Change-Id: Id01a6e537f2285a9c35ce90acc9eba81b0ea6c1f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2835019
Owners-Override: Olga Sharonova <olka@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Olga Sharonova <olka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#873746}
  • Loading branch information
Olga Sharonova authored and Chromium LUCI CQ committed Apr 19, 2021
1 parent 9cf88e5 commit a7c0340
Show file tree
Hide file tree
Showing 13 changed files with 60 additions and 110 deletions.
2 changes: 0 additions & 2 deletions chromecast/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -309,8 +309,6 @@ cast_source_set("browser_base") {
"cast_content_window_aura.cc",
"cast_content_window_aura.h",
"cast_web_service_aura.cc",
"display_configurator_observer.cc",
"display_configurator_observer.h",
]

deps += [
Expand Down
3 changes: 0 additions & 3 deletions chromecast/browser/cast_browser_main_parts.cc
Original file line number Diff line number Diff line change
Expand Up @@ -616,9 +616,6 @@ int CastBrowserMainParts::PreMainMessageLoopRun() {
std::make_unique<RoundedWindowCornersManager>(window_manager_.get());
}

display_change_observer_ = std::make_unique<DisplayConfiguratorObserver>(
cast_browser_process_->display_configurator(), window_manager_.get());

#if BUILDFLAG(ENABLE_CHROMECAST_EXTENSIONS)
cast_browser_process_->SetAccessibilityManager(
std::make_unique<AccessibilityManagerImpl>(window_manager_.get()));
Expand Down
2 changes: 0 additions & 2 deletions chromecast/browser/cast_browser_main_parts.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#include "base/util/memory_pressure/multi_source_memory_pressure_monitor.h"
#include "build/build_config.h"
#include "build/buildflag.h"
#include "chromecast/browser/display_configurator_observer.h"
#include "chromecast/chromecast_buildflags.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/browser_main_parts.h"
Expand Down Expand Up @@ -106,7 +105,6 @@ class CastBrowserMainParts : public content::BrowserMainParts {
std::unique_ptr<CastScreen> cast_screen_;
std::unique_ptr<CastWindowManagerAura> window_manager_;
std::unique_ptr<RoundedWindowCornersManager> rounded_window_corners_manager_;
std::unique_ptr<DisplayConfiguratorObserver> display_change_observer_;
#else
std::unique_ptr<CastWindowManager> window_manager_;
#endif // defined(USE_AURA)
Expand Down
16 changes: 0 additions & 16 deletions chromecast/browser/cast_display_configurator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,6 @@ void CastDisplayConfigurator::EnableDisplay(
config_request.push_back(std::move(display_config_params));

delegate_->Configure(config_request, std::move(callback));
NotifyObservers();
}

void CastDisplayConfigurator::DisableDisplay(
Expand All @@ -168,7 +167,6 @@ void CastDisplayConfigurator::SetColorMatrix(
if (!delegate_ || !display_)
return;
delegate_->SetColorMatrix(display_->display_id(), color_matrix);
NotifyObservers();
}

void CastDisplayConfigurator::SetGammaCorrection(
Expand All @@ -178,12 +176,6 @@ void CastDisplayConfigurator::SetGammaCorrection(
return;

delegate_->SetGammaCorrection(display_->display_id(), degamma_lut, gamma_lut);
NotifyObservers();
}

void CastDisplayConfigurator::NotifyObservers() {
for (Observer& observer : observers_)
observer.OnDisplayStateChanged();
}

void CastDisplayConfigurator::ForceInitialConfigure() {
Expand Down Expand Up @@ -273,13 +265,5 @@ void CastDisplayConfigurator::UpdateScreen(
touch_device_manager_->OnDisplayConfigured(display_id, rotation, bounds);
}

void CastDisplayConfigurator::AddObserver(Observer* observer) {
observers_.AddObserver(observer);
}

void CastDisplayConfigurator::RemoveObserver(Observer* observer) {
observers_.RemoveObserver(observer);
}

} // namespace shell
} // namespace chromecast
13 changes: 0 additions & 13 deletions chromecast/browser/cast_display_configurator.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include "base/containers/flat_map.h"
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "base/observer_list.h"
#include "ui/display/display.h"
#include "ui/display/types/display_configuration_params.h"
#include "ui/display/types/native_display_delegate.h"
Expand Down Expand Up @@ -41,12 +40,6 @@ class CastTouchDeviceManager;
// doesn't really do anything when using OzonePlatformCast.
class CastDisplayConfigurator : public display::NativeDisplayObserver {
public:
class Observer {
public:
virtual ~Observer() = default;
virtual void OnDisplayStateChanged() = 0;
};

explicit CastDisplayConfigurator(CastScreen* screen);
~CastDisplayConfigurator() override;

Expand All @@ -57,9 +50,6 @@ class CastDisplayConfigurator : public display::NativeDisplayObserver {
void EnableDisplay(display::ConfigureCallback callback);
void DisableDisplay(display::ConfigureCallback callback);

void AddObserver(Observer* observer);
void RemoveObserver(Observer* observer);

void ConfigureDisplayFromCommandLine();
void SetColorMatrix(const std::vector<float>& color_matrix);
void SetGammaCorrection(
Expand All @@ -68,7 +58,6 @@ class CastDisplayConfigurator : public display::NativeDisplayObserver {

private:
void ForceInitialConfigure();
void NotifyObservers();
void OnDisplaysAcquired(
bool force_initial_configure,
const std::vector<display::DisplaySnapshot*>& displays);
Expand All @@ -81,8 +70,6 @@ class CastDisplayConfigurator : public display::NativeDisplayObserver {
float device_scale_factor,
display::Display::Rotation rotation);

base::ObserverList<Observer>::Unchecked observers_;

std::unique_ptr<display::NativeDisplayDelegate> delegate_;
std::unique_ptr<CastTouchDeviceManager> touch_device_manager_;
display::DisplaySnapshot* display_;
Expand Down
27 changes: 0 additions & 27 deletions chromecast/browser/display_configurator_observer.cc

This file was deleted.

42 changes: 0 additions & 42 deletions chromecast/browser/display_configurator_observer.h

This file was deleted.

13 changes: 13 additions & 0 deletions chromecast/ui/display_settings/color_temperature_animation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@

#include "base/numerics/ranges.h"
#include "base/time/time.h"
#include "chromecast/graphics/cast_window_manager.h"
#include "ui/aura/window.h"
#include "ui/aura/window_tree_host.h"
#include "ui/compositor/compositor.h"
#include "ui/compositor/scoped_animation_duration_scale_mode.h"

#if defined(USE_AURA)
Expand All @@ -35,16 +39,19 @@ float Interpolate(const std::vector<float>& vec, float idx) {
} // namespace

ColorTemperatureAnimation::ColorTemperatureAnimation(
CastWindowManager* window_manager,
shell::CastDisplayConfigurator* display_configurator,
const DisplaySettingsManager::ColorTemperatureConfig& config)
: gfx::LinearAnimation(kManualAnimationDuration,
kAnimationFrameRate,
nullptr),
window_manager_(window_manager),
display_configurator_(display_configurator),
config_(config),
start_temperature_(config.neutral_temperature),
current_temperature_(config.neutral_temperature),
target_temperature_(config_.neutral_temperature) {
DCHECK(window_manager_);
#if defined(USE_AURA)
DCHECK(display_configurator_);
#endif // defined(USE_AURA)
Expand Down Expand Up @@ -116,6 +123,12 @@ void ColorTemperatureAnimation::ApplyValuesToDisplay() {
std::vector<float> color_matrix = {red_scale, 0, 0, 0, green_scale,
0, 0, 0, blue_scale};
display_configurator_->SetColorMatrix(color_matrix);
// The CTM is applied on the next swap buffers, so we need to make sure the
// root window triggers a swap buffer otherwise the content will not update.
window_manager_->GetRootWindow()
->GetHost()
->compositor()
->ScheduleFullRedraw();
#endif // defined(USE_AURA)
}
}
Expand Down
4 changes: 4 additions & 0 deletions chromecast/ui/display_settings/color_temperature_animation.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

namespace chromecast {

class CastWindowManager;

namespace shell {
class CastDisplayConfigurator;
}
Expand All @@ -22,6 +24,7 @@ class CastDisplayConfigurator;
class ColorTemperatureAnimation : public gfx::LinearAnimation {
public:
ColorTemperatureAnimation(
CastWindowManager* window_manager,
shell::CastDisplayConfigurator* display_configurator,
const DisplaySettingsManager::ColorTemperatureConfig& config);
ColorTemperatureAnimation(const ColorTemperatureAnimation&) = delete;
Expand All @@ -44,6 +47,7 @@ class ColorTemperatureAnimation : public gfx::LinearAnimation {

void ApplyValuesToDisplay();

CastWindowManager* const window_manager_;
shell::CastDisplayConfigurator* const display_configurator_;

const DisplaySettingsManager::ColorTemperatureConfig config_;
Expand Down
16 changes: 15 additions & 1 deletion chromecast/ui/display_settings/gamma_configurator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
#include <limits>

#include "chromecast/browser/cast_display_configurator.h"
#include "chromecast/graphics/cast_window_manager.h"
#include "ui/aura/window.h"
#include "ui/aura/window_tree_host.h"
#include "ui/compositor/compositor.h"

namespace chromecast {
namespace {
Expand Down Expand Up @@ -41,8 +45,11 @@ std::vector<display::GammaRampRGBEntry> InvertGammaLut(
} // namespace

GammaConfigurator::GammaConfigurator(
CastWindowManager* window_manager,
shell::CastDisplayConfigurator* display_configurator)
: display_configurator_(display_configurator) {
: window_manager_(window_manager),
display_configurator_(display_configurator) {
DCHECK(window_manager_);
DCHECK(display_configurator_);
}

Expand All @@ -62,6 +69,13 @@ void GammaConfigurator::ApplyGammaLut() {
display_configurator_->SetGammaCorrection({}, InvertGammaLut(gamma_lut_));
else
display_configurator_->SetGammaCorrection({}, gamma_lut_);

// The LUT is applied on the next swap buffers, so we need to make sure the
// root window triggers a swap buffer otherwise the content will not update.
window_manager_->GetRootWindow()
->GetHost()
->compositor()
->ScheduleFullRedraw();
}

void GammaConfigurator::SetColorInversion(bool invert) {
Expand Down
7 changes: 5 additions & 2 deletions chromecast/ui/display_settings/gamma_configurator.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@

namespace chromecast {

class CastWindowManager;

namespace shell {
class CastDisplayConfigurator;
} // namespace shell

class GammaConfigurator {
public:
explicit GammaConfigurator(
shell::CastDisplayConfigurator* display_configurator);
GammaConfigurator(CastWindowManager* window_manager,
shell::CastDisplayConfigurator* display_configurator);
GammaConfigurator(const GammaConfigurator&) = delete;
GammaConfigurator& operator=(const GammaConfigurator&) = delete;
~GammaConfigurator();
Expand All @@ -31,6 +33,7 @@ class GammaConfigurator {
private:
void ApplyGammaLut();

CastWindowManager* const window_manager_;
shell::CastDisplayConfigurator* display_configurator_;

bool is_initialized_ = false;
Expand Down
23 changes: 21 additions & 2 deletions chromecast/ui/display_settings_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
#include "chromecast/browser/cast_browser_process.h"
#include "chromecast/browser/cast_display_configurator.h"
#include "chromecast/ui/display_settings/gamma_configurator.h"
#include "ui/aura/window.h"
#include "ui/aura/window_tree_host.h"
#endif // defined(USE_AURA)

namespace chromecast {
Expand All @@ -40,13 +42,15 @@ DisplaySettingsManagerImpl::DisplaySettingsManagerImpl(
display_configurator_(
shell::CastBrowserProcess::GetInstance()->display_configurator()),
gamma_configurator_(
std::make_unique<GammaConfigurator>(display_configurator_)),
std::make_unique<GammaConfigurator>(window_manager_,
display_configurator_)),
#else
display_configurator_(nullptr),
#endif // defined(USE_AURA)
brightness_(-1.0f),
screen_power_controller_(ScreenPowerController::Create(this)),
color_temperature_animation_(std::make_unique<ColorTemperatureAnimation>(
window_manager_,
display_configurator_,
color_temperature_config)),
weak_factory_(this) {
Expand Down Expand Up @@ -96,7 +100,9 @@ void DisplaySettingsManagerImpl::AddReceiver(

void DisplaySettingsManagerImpl::SetScreenPowerOn(PowerToggleCallback callback) {
#if defined(USE_AURA)
display_configurator_->EnableDisplay(std::move(callback));
display_configurator_->EnableDisplay(
base::BindOnce(&DisplaySettingsManagerImpl::OnScreenEnabled,
weak_factory_.GetWeakPtr(), std::move(callback)));
#endif // defined(USE_AURA)
}

Expand Down Expand Up @@ -190,4 +196,17 @@ void DisplaySettingsManagerImpl::UpdateBrightness(float brightness,
brightness_animation_->AnimateToNewValue(brightness, duration);
}

void DisplaySettingsManagerImpl::OnScreenEnabled(PowerToggleCallback callback,
bool status) {
#if defined(USE_AURA)
// Force a swap buffers otherwise we might be stuck showing the modeset
// buffer.
window_manager_->GetRootWindow()
->GetHost()
->compositor()
->ScheduleFullRedraw();
#endif // defined(USE_AURA)
std::move(callback).Run(status);
}

} // namespace chromecast
2 changes: 2 additions & 0 deletions chromecast/ui/display_settings_manager_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ class DisplaySettingsManagerImpl : public DisplaySettingsManager,

void UpdateBrightness(float brightness, base::TimeDelta duration);

void OnScreenEnabled(PowerToggleCallback callback, bool status);

CastWindowManager* const window_manager_;
shell::CastDisplayConfigurator* const display_configurator_;

Expand Down

0 comments on commit a7c0340

Please sign in to comment.