Skip to content

Commit

Permalink
Add preference for rastering in sRGB
Browse files Browse the repository at this point in the history
Some platforms would prefer to raster in sRGB unless there is reason
not to, such as content using a wide color gamut.t cl for

This CL adds a setting to indicate the preference of using sRGB and
short-circuits GetRasterColorSpace() to return sRGB instead.

Bug: 955158
Change-Id: I5762851ffc7cf93a50e6d46eefebf8a652ee3e6b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2045053
Commit-Queue: Chris Blume <cblume@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#741620}
  • Loading branch information
ProgramMax authored and Commit Bot committed Feb 14, 2020
1 parent 87f4f89 commit 7f752e3
Show file tree
Hide file tree
Showing 17 changed files with 169 additions and 71 deletions.
23 changes: 17 additions & 6 deletions cc/layers/picture_layer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -683,13 +683,24 @@ void PictureLayerImpl::UpdateRasterSource(

// If the MSAA sample count has changed, we need to re-raster the complete
// layer.
if (raster_source_ && raster_source_->GetDisplayItemList() &&
raster_source->GetDisplayItemList() &&
layer_tree_impl()->GetMSAASampleCountForRaster(
raster_source_->GetDisplayItemList()) !=
if (raster_source_) {
const auto& current_display_item_list =
raster_source_->GetDisplayItemList();
const auto& new_display_item_list = raster_source->GetDisplayItemList();
if (current_display_item_list && new_display_item_list) {
bool needs_full_invalidation =
layer_tree_impl()->GetMSAASampleCountForRaster(
raster_source->GetDisplayItemList())) {
new_invalidation->Union(gfx::Rect(raster_source->GetSize()));
current_display_item_list) !=
layer_tree_impl()->GetMSAASampleCountForRaster(
new_display_item_list);
needs_full_invalidation |=
current_display_item_list->discardable_image_map()
.contains_only_srgb_images() !=
new_display_item_list->discardable_image_map()
.contains_only_srgb_images();
if (needs_full_invalidation)
new_invalidation->Union(gfx::Rect(raster_source->GetSize()));
}
}
}

Expand Down
6 changes: 5 additions & 1 deletion cc/resources/resource_pool.cc
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,8 @@ ResourcePool::TryAcquireResourceForPartialRaster(
uint64_t new_content_id,
const gfx::Rect& new_invalidated_rect,
uint64_t previous_content_id,
gfx::Rect* total_invalidated_rect) {
gfx::Rect* total_invalidated_rect,
const gfx::ColorSpace& raster_color_space) {
DCHECK(new_content_id);
DCHECK(previous_content_id);
*total_invalidated_rect = gfx::Rect();
Expand All @@ -213,6 +214,9 @@ ResourcePool::TryAcquireResourceForPartialRaster(
for (auto it = unused_resources_.begin(); it != unused_resources_.end();
++it) {
PoolResource* resource = it->get();
if (resource->color_space() != raster_color_space)
continue;

if (resource->content_id() == previous_content_id) {
UpdateResourceContentIdAndInvalidation(resource, new_content_id,
new_invalidated_rect);
Expand Down
3 changes: 2 additions & 1 deletion cc/resources/resource_pool.h
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,8 @@ class CC_EXPORT ResourcePool : public base::trace_event::MemoryDumpProvider {
uint64_t new_content_id,
const gfx::Rect& new_invalidated_rect,
uint64_t previous_content_id,
gfx::Rect* total_invalidated_rect);
gfx::Rect* total_invalidated_rect,
const gfx::ColorSpace& raster_color_space);

// Gives the InUsePoolResource a |resource_id_for_export()| in order to allow
// exporting of the resource to the display compositor. This must be called
Expand Down
14 changes: 8 additions & 6 deletions cc/resources/resource_pool_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,8 @@ TEST_F(ResourcePoolTest, UpdateContentId) {
gfx::Rect invalidated_rect;
ResourcePool::InUsePoolResource reacquired_resource =
resource_pool_->TryAcquireResourceForPartialRaster(
new_content_id, new_invalidated_rect, content_id, &invalidated_rect);
new_content_id, new_invalidated_rect, content_id, &invalidated_rect,
color_space);
EXPECT_EQ(original_id, reacquired_resource.unique_id_for_testing());
EXPECT_EQ(new_invalidated_rect, invalidated_rect);
resource_pool_->ReleaseResource(std::move(reacquired_resource));
Expand All @@ -388,7 +389,7 @@ TEST_F(ResourcePoolTest, UpdateContentIdAndInvalidatedRect) {
ResourcePool::InUsePoolResource reacquired_resource =
resource_pool_->TryAcquireResourceForPartialRaster(
content_ids[1], invalidated_rect, content_ids[0],
&new_invalidated_rect);
&new_invalidated_rect, color_space);
EXPECT_FALSE(!!reacquired_resource);
EXPECT_EQ(gfx::Rect(), new_invalidated_rect);

Expand All @@ -397,7 +398,8 @@ TEST_F(ResourcePoolTest, UpdateContentIdAndInvalidatedRect) {

// Ensure that we cannot retrieve a resource based on the original content id.
reacquired_resource = resource_pool_->TryAcquireResourceForPartialRaster(
content_ids[1], invalidated_rect, content_ids[0], &new_invalidated_rect);
content_ids[1], invalidated_rect, content_ids[0], &new_invalidated_rect,
color_space);
EXPECT_FALSE(!!reacquired_resource);
EXPECT_EQ(gfx::Rect(), new_invalidated_rect);

Expand All @@ -406,7 +408,7 @@ TEST_F(ResourcePoolTest, UpdateContentIdAndInvalidatedRect) {
gfx::Rect total_invalidated_rect;
reacquired_resource = resource_pool_->TryAcquireResourceForPartialRaster(
content_ids[2], second_invalidated_rect, content_ids[1],
&total_invalidated_rect);
&total_invalidated_rect, color_space);
EXPECT_EQ(original_id, reacquired_resource.unique_id_for_testing());
EXPECT_EQ(expected_total_invalidated_rect, total_invalidated_rect);
resource_pool_->ReleaseResource(std::move(reacquired_resource));
Expand All @@ -431,7 +433,7 @@ TEST_F(ResourcePoolTest, LargeInvalidatedRect) {
ResourcePool::InUsePoolResource reacquired_resource =
resource_pool_->TryAcquireResourceForPartialRaster(
content_ids[1], large_invalidated_rect, content_ids[0],
&new_invalidated_rect);
&new_invalidated_rect, color_space);
EXPECT_FALSE(!!reacquired_resource);

// Release the original resource, returning it to the unused pool.
Expand All @@ -441,7 +443,7 @@ TEST_F(ResourcePoolTest, LargeInvalidatedRect) {
// too large to compute the area for.
resource = resource_pool_->TryAcquireResourceForPartialRaster(
content_ids[2], large_invalidated_rect, content_ids[1],
&new_invalidated_rect);
&new_invalidated_rect, color_space);
EXPECT_TRUE(!!resource);
resource_pool_->ReleaseResource(std::move(resource));
}
Expand Down
3 changes: 2 additions & 1 deletion cc/test/fake_tile_manager_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ FakeTileManagerClient::BuildEvictionQueue(TreePriority tree_priority) {
return nullptr;
}

const gfx::ColorSpace& FakeTileManagerClient::GetRasterColorSpace() const {
gfx::ColorSpace FakeTileManagerClient::GetRasterColorSpace(
gfx::ContentColorUsage /*content_color_usage*/) const {
return color_space_;
}

Expand Down
3 changes: 2 additions & 1 deletion cc/test/fake_tile_manager_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ class FakeTileManagerClient : public TileManagerClient {
std::unique_ptr<EvictionTilePriorityQueue> BuildEvictionQueue(
TreePriority tree_priority) override;
void SetIsLikelyToRequireADraw(bool is_likely_to_require_a_draw) override {}
const gfx::ColorSpace& GetRasterColorSpace() const override;
gfx::ColorSpace GetRasterColorSpace(
gfx::ContentColorUsage content_color_usage) const override;
void RequestImplSideInvalidationForCheckerImagedTiles() override {}
size_t GetFrameIndexForImage(const PaintImage& paint_image,
WhichTree tree) const override;
Expand Down
37 changes: 30 additions & 7 deletions cc/tiles/tile_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "cc/base/devtools_instrumentation.h"
#include "cc/base/histograms.h"
#include "cc/layers/picture_layer_impl.h"
#include "cc/paint/display_item_list.h"
#include "cc/raster/paint_worklet_image_provider.h"
#include "cc/raster/playback_image_provider.h"
#include "cc/raster/raster_buffer.h"
Expand Down Expand Up @@ -352,6 +353,17 @@ class DidFinishRunningAllTilesTask : public TileTask {
CompletionCb completion_cb_;
};

gfx::ContentColorUsage GetContentColorUsageForPrioritizedTile(
const PrioritizedTile& prioritized_tile) {
// TODO(cblume,ccameron): Add support for HDR.
bool contains_only_srgb_images = prioritized_tile.raster_source()
->GetDisplayItemList()
->discardable_image_map()
.contains_only_srgb_images();
return contains_only_srgb_images ? gfx::ContentColorUsage::kSRGB
: gfx::ContentColorUsage::kWideColorGamut;
}

} // namespace

RasterTaskCompletionStats::RasterTaskCompletionStats()
Expand Down Expand Up @@ -679,8 +691,6 @@ TileManager::PrioritizedWorkToSchedule TileManager::AssignGpuMemoryToTiles() {
MemoryUsage memory_usage(resource_pool_->memory_usage_bytes(),
resource_pool_->resource_count());

gfx::ColorSpace raster_color_space = client_->GetRasterColorSpace();

std::unique_ptr<RasterTilePriorityQueue> raster_priority_queue(
client_->BuildRasterQueue(global_state_.tree_priority,
RasterTilePriorityQueue::Type::ALL));
Expand Down Expand Up @@ -721,6 +731,11 @@ TileManager::PrioritizedWorkToSchedule TileManager::AssignGpuMemoryToTiles() {
continue;
}

auto content_color_usage =
GetContentColorUsageForPrioritizedTile(prioritized_tile);
const gfx::ColorSpace raster_color_space =
client_->GetRasterColorSpace(content_color_usage);

// Tiles in the raster queue should either require raster or decode for
// checker-images. If this tile does not need raster, process it only to
// build the decode queue for checkered images.
Expand Down Expand Up @@ -799,8 +814,8 @@ TileManager::PrioritizedWorkToSchedule TileManager::AssignGpuMemoryToTiles() {
} else {
// Creating the raster task here will acquire resources, but
// this resource usage has already been accounted for above.
auto raster_task = CreateRasterTask(
prioritized_tile, client_->GetRasterColorSpace(), &work_to_schedule);
auto raster_task = CreateRasterTask(prioritized_tile, raster_color_space,
&work_to_schedule);
if (!raster_task) {
continue;
}
Expand Down Expand Up @@ -836,6 +851,11 @@ TileManager::PrioritizedWorkToSchedule TileManager::AssignGpuMemoryToTiles() {
if (!prioritized_tile.should_decode_checkered_images_for_tile())
continue;

auto content_color_usage =
GetContentColorUsageForPrioritizedTile(prioritized_tile);
gfx::ColorSpace raster_color_space =
client_->GetRasterColorSpace(content_color_usage);

Tile* tile = prioritized_tile.tile();
if (tile->draw_info().is_checker_imaged() ||
tile->raster_task_scheduled_with_checker_images()) {
Expand Down Expand Up @@ -973,8 +993,6 @@ void TileManager::ScheduleTasks(PrioritizedWorkToSchedule work_to_schedule) {

graph_.Reset();

gfx::ColorSpace raster_color_space = client_->GetRasterColorSpace();

scoped_refptr<TileTask> required_for_activation_done_task =
CreateTaskSetFinishedTask(
&TileManager::DidFinishRunningTileTasksRequiredForActivation);
Expand Down Expand Up @@ -1028,6 +1046,11 @@ void TileManager::ScheduleTasks(PrioritizedWorkToSchedule work_to_schedule) {
work_to_schedule.tiles_to_process_for_images;
std::vector<DrawImage> new_locked_images;
for (const PrioritizedTile& prioritized_tile : tiles_to_process_for_images) {
auto content_color_usage =
GetContentColorUsageForPrioritizedTile(prioritized_tile);
gfx::ColorSpace raster_color_space =
client_->GetRasterColorSpace(content_color_usage);

std::vector<DrawImage> sync_decoded_images;
std::vector<PaintImage> checkered_images;
PartitionImagesForCheckering(prioritized_tile, raster_color_space,
Expand Down Expand Up @@ -1143,7 +1166,7 @@ scoped_refptr<TileTask> TileManager::CreateRasterTask(
if (UsePartialRaster(msaa_sample_count) && tile->invalidated_id()) {
resource = resource_pool_->TryAcquireResourceForPartialRaster(
tile->id(), tile->invalidated_content_rect(), tile->invalidated_id(),
&invalidated_rect);
&invalidated_rect, raster_color_space);
}

bool partial_tile_decode = false;
Expand Down
6 changes: 4 additions & 2 deletions cc/tiles/tile_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "cc/tiles/tile_draw_info.h"
#include "cc/tiles/tile_manager_settings.h"
#include "cc/tiles/tile_task_manager.h"
#include "ui/gfx/display_color_spaces.h"
#include "url/gurl.h"

namespace base {
Expand Down Expand Up @@ -80,7 +81,8 @@ class CC_EXPORT TileManagerClient {
virtual void SetIsLikelyToRequireADraw(bool is_likely_to_require_a_draw) = 0;

// Requests the color space into which tiles should be rasterized.
virtual const gfx::ColorSpace& GetRasterColorSpace() const = 0;
virtual gfx::ColorSpace GetRasterColorSpace(
gfx::ContentColorUsage content_color_usage) const = 0;

// Requests that a pending tree be scheduled to invalidate content on the
// pending on active tree. This is currently used when tiles that are
Expand Down Expand Up @@ -206,7 +208,7 @@ class CC_EXPORT TileManager : CheckerImageTrackerClient {
resource_pool_->AcquireResource(
tiles[i]->desired_texture_size(),
raster_buffer_provider_->GetResourceFormat(),
client_->GetRasterColorSpace());
client_->GetRasterColorSpace(gfx::ContentColorUsage::kSRGB));
raster_buffer_provider_->AcquireBufferForRaster(
resource, 0, 0,
/*depends_on_at_raster_decodes=*/false,
Expand Down
12 changes: 6 additions & 6 deletions cc/tiles/tile_manager_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1949,8 +1949,8 @@ TEST_F(PartialRasterTileManagerTest, CancelledTasksHaveNoContentId) {
// with its invalidated resource ID.
gfx::Rect total_invalidated_rect;
EXPECT_FALSE(host_impl()->resource_pool()->TryAcquireResourceForPartialRaster(
kInvalidatedId + 1, gfx::Rect(), kInvalidatedId,
&total_invalidated_rect));
kInvalidatedId + 1, gfx::Rect(), kInvalidatedId, &total_invalidated_rect,
gfx::ColorSpace::CreateSRGB()));
EXPECT_EQ(gfx::Rect(), total_invalidated_rect);

// Free our host_impl_ before the tile_task_manager we passed it, as it
Expand Down Expand Up @@ -2007,8 +2007,8 @@ void RunPartialRasterCheck(std::unique_ptr<LayerTreeHostImpl> host_impl,

// Ensure there's a resource with our |kInvalidatedId| in the resource pool.
ResourcePool::InUsePoolResource resource =
host_impl->resource_pool()->AcquireResource(kTileSize, viz::RGBA_8888,
gfx::ColorSpace());
host_impl->resource_pool()->AcquireResource(
kTileSize, viz::RGBA_8888, gfx::ColorSpace::CreateSRGB());

resource.set_software_backing(std::make_unique<TestSoftwareBacking>());
host_impl->resource_pool()->PrepareForExport(resource);
Expand Down Expand Up @@ -2072,8 +2072,8 @@ void RunPartialTileDecodeCheck(std::unique_ptr<LayerTreeHostImpl> host_impl,

// Ensure there's a resource with our |kInvalidatedId| in the resource pool.
ResourcePool::InUsePoolResource resource =
host_impl->resource_pool()->AcquireResource(kTileSize, viz::RGBA_8888,
gfx::ColorSpace());
host_impl->resource_pool()->AcquireResource(
kTileSize, viz::RGBA_8888, gfx::ColorSpace::CreateSRGB());
host_impl->resource_pool()->OnContentReplaced(resource, kInvalidatedId);
host_impl->resource_pool()->ReleaseResource(std::move(resource));

Expand Down
26 changes: 18 additions & 8 deletions cc/trees/layer_tree_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1808,14 +1808,21 @@ void LayerTreeHostImpl::SetIsLikelyToRequireADraw(
is_likely_to_require_a_draw_ = is_likely_to_require_a_draw;
}

const gfx::ColorSpace& LayerTreeHostImpl::GetRasterColorSpace() const {
const gfx::ColorSpace* result = nullptr;
gfx::ColorSpace LayerTreeHostImpl::GetRasterColorSpace(
gfx::ContentColorUsage content_color_usage) const {
constexpr gfx::ColorSpace srgb = gfx::ColorSpace::CreateSRGB();

if (settings_.prefer_raster_in_srgb &&
content_color_usage == gfx::ContentColorUsage::kSRGB)
return srgb;

gfx::ColorSpace result;
// The pending tree will have the most recently updated color space, so
// prefer that.
if (pending_tree_) {
result = &pending_tree_->raster_color_space();
result = pending_tree_->raster_color_space();
} else if (active_tree_) {
result = &active_tree_->raster_color_space();
result = active_tree_->raster_color_space();
}

// If we are likely to software composite the resource, we use sRGB because
Expand All @@ -1824,10 +1831,10 @@ const gfx::ColorSpace& LayerTreeHostImpl::GetRasterColorSpace() const {
// (not specifying a color space indicates that no color conversion is
// required).
if (!layer_tree_frame_sink_ || !layer_tree_frame_sink_->context_provider() ||
!result || !result->IsValid()) {
result = &default_color_space_;
!result.IsValid()) {
result = srgb;
}
return *result;
return result;
}

void LayerTreeHostImpl::RequestImplSideInvalidationForCheckerImagedTiles() {
Expand Down Expand Up @@ -3372,8 +3379,11 @@ void LayerTreeHostImpl::QueueImageDecode(int request_id,
image.GetKeyForFrame(PaintImage::kDefaultFrameIndex).ToString());
// Optimistically specify the current raster color space, since we assume that
// it won't change.
auto content_color_usage = image.isSRGB()
? gfx::ContentColorUsage::kSRGB
: gfx::ContentColorUsage::kWideColorGamut;
tile_manager_.decoded_image_tracker().QueueImageDecode(
image, GetRasterColorSpace(),
image, GetRasterColorSpace(content_color_usage),
base::BindOnce(&LayerTreeHostImpl::ImageDecodeFinished,
weak_factory_.GetWeakPtr(), request_id));
tile_manager_.checker_image_tracker().DisallowCheckeringForImage(image);
Expand Down
5 changes: 2 additions & 3 deletions cc/trees/layer_tree_host_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,8 @@ class CC_EXPORT LayerTreeHostImpl : public InputHandler,
std::unique_ptr<EvictionTilePriorityQueue> BuildEvictionQueue(
TreePriority tree_priority) override;
void SetIsLikelyToRequireADraw(bool is_likely_to_require_a_draw) override;
const gfx::ColorSpace& GetRasterColorSpace() const override;
gfx::ColorSpace GetRasterColorSpace(
gfx::ContentColorUsage content_color_usage) const override;
void RequestImplSideInvalidationForCheckerImagedTiles() override;
size_t GetFrameIndexForImage(const PaintImage& paint_image,
WhichTree tree) const override;
Expand Down Expand Up @@ -1043,8 +1044,6 @@ class CC_EXPORT LayerTreeHostImpl : public InputHandler,
// This is usually turned on only in some tests (e.g. web-tests).
const bool is_synchronous_single_threaded_;

const gfx::ColorSpace default_color_space_ = gfx::ColorSpace::CreateSRGB();

viz::ClientResourceProvider resource_provider_;

std::unordered_map<UIResourceId, UIResourceData> ui_resource_map_;
Expand Down
Loading

0 comments on commit 7f752e3

Please sign in to comment.