Skip to content

Commit

Permalink
Revert "Add a PaintRecorder to CanvasResourceProvider"
Browse files Browse the repository at this point in the history
This reverts commit 2a3b118.

Reason for revert: crbug.com/1054408

Original change's description:
> Add a PaintRecorder to CanvasResourceProvider
> 
> This CL is the first step in creating a PaintRecord backed resource
> provider for canvas OOP-R.
> 
> Changes of ownership:
> - PaintRecorder entirely moved from Canvas2DLayerBridge to
>   CanvasResourceProvider.
> - Management of needs_flush_ for MemoryManagedPaintRecorder moved to
>   CanvasResourceProvider.
> - RestoreCanvasMatrixClipStack logic centralized to CanvasResourceHost
>   and can be accessed via callback
> - Backing SkiaPaintCanvas now private to CanvasResourceProvider with no
>   external access. Any users of the backing SkiaPaintCanvas in
>   Canvas2DLayerBridge had their functionality moved to
>   CanvasResourceProvider (ie. new RestoreBackBuffer() function)
> 
> Updates to test files:
> - Addition of explicit FlushCanvas() calls in tests that used to expect
>   SkiaPaintCanvas to immediately update.
> - Some tests expected the CanvasResourceProvider to be created on first
>   draw. This expectation has been changed since the
>   CanvasResourceProvider now gets set up on SetCanvasResourceHost().
> 
> Other new functionality:
> - Display Item List now has a bit that tracks if it contains any draw
>   operations. This is used to see if there are draw ops to flush.
> 
> Bug: 1019288
> Change-Id: I717b18e22d6699dc876d8f8121a25d147738579d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1891292
> Commit-Queue: Khushal <khushalsagar@chromium.org>
> Reviewed-by: Aaron Krajeski <aaronhk@chromium.org>
> Reviewed-by: Juanmi Huertas <juanmihd@chromium.org>
> Reviewed-by: Fernando Serboncini <fserb@chromium.org>
> Reviewed-by: Khushal <khushalsagar@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#743002}

TBR=senorblanco@chromium.org,fserb@chromium.org,khushalsagar@chromium.org,aaronhk@chromium.org,juanmihd@chromium.org,jochin@microsoft.com

Change-Id: Icd1e97d26d3b4b46598f02c03b163ba31a1c2881
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1019288
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2067239
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Jonah Chin <jochin@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#743213}
  • Loading branch information
Jonah Chin authored and Commit Bot committed Feb 20, 2020
1 parent 38d5e42 commit b94a40a
Show file tree
Hide file tree
Showing 18 changed files with 213 additions and 244 deletions.
1 change: 0 additions & 1 deletion cc/paint/display_item_list.cc
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,6 @@ void DisplayItemList::Reset() {
offsets_.shrink_to_fit();
begin_paired_indices_.clear();
begin_paired_indices_.shrink_to_fit();
has_draw_ops_ = false;
}

sk_sp<PaintRecord> DisplayItemList::ReleaseAsRecord() {
Expand Down
4 changes: 0 additions & 4 deletions cc/paint/display_item_list.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,6 @@ class CC_PAINT_EXPORT DisplayItemList
if (usage_hint_ == kTopLevelDisplayItemList)
offsets_.push_back(offset);
const T* op = paint_op_buffer_.push<T>(std::forward<Args>(args)...);
if (op->IsDrawOp())
has_draw_ops_ = true;
DCHECK(op->IsValid());
return offset;
}
Expand Down Expand Up @@ -199,7 +197,6 @@ class CC_PAINT_EXPORT DisplayItemList
int max_ops_to_analyze = 1);

std::string ToString() const;
bool has_draw_ops() const { return has_draw_ops_; }

private:
friend class DisplayItemListTest;
Expand Down Expand Up @@ -249,7 +246,6 @@ class CC_PAINT_EXPORT DisplayItemList
#endif

UsageHint usage_hint_;
bool has_draw_ops_ = false;

friend class base::RefCountedThreadSafe<DisplayItemList>;
FRIEND_TEST_ALL_PREFIXES(DisplayItemListTest, BytesUsed);
Expand Down
4 changes: 0 additions & 4 deletions cc/paint/paint_recorder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,4 @@ std::unique_ptr<RecordPaintCanvas> PaintRecorder::CreateCanvas(
return std::make_unique<RecordPaintCanvas>(list, bounds);
}

bool PaintRecorder::ListHasDrawOps() const {
return display_item_list_->has_draw_ops();
}

} // namespace cc
2 changes: 0 additions & 2 deletions cc/paint/paint_recorder.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@ class CC_PAINT_EXPORT PaintRecorder {

sk_sp<PaintRecord> finishRecordingAsPicture();

bool ListHasDrawOps() const;

protected:
virtual std::unique_ptr<RecordPaintCanvas> CreateCanvas(DisplayItemList* list,
const SkRect& bounds);
Expand Down
24 changes: 7 additions & 17 deletions third_party/blink/renderer/core/html/canvas/html_canvas_element.cc
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,11 @@ void HTMLCanvasElement::Dispose() {

if (context_) {
UMA_HISTOGRAM_BOOLEAN("Blink.Canvas.HasRendered", bool(ResourceProvider()));
if (context_->Host()) {
if (ResourceProvider()) {
UMA_HISTOGRAM_BOOLEAN("Blink.Canvas.IsComposited",
context_->IsComposited());
context_->DetachHost();
}
context_->DetachHost();
context_ = nullptr;
}

Expand Down Expand Up @@ -1506,30 +1506,20 @@ void HTMLCanvasElement::ReplaceExisting2dLayerBridge(
if (canvas2d_bridge_) {
image = canvas2d_bridge_->NewImageSnapshot(kPreferNoAcceleration);
// image can be null if allocation failed in which case we should just
// abort the surface switch to retain the old surface which is still
// abort the surface switch to reatain the old surface which is still
// functional.
if (!image)
return;
}
new_layer_bridge->SetCanvasResourceHost(this);
ReplaceResourceProvider(nullptr);
canvas2d_bridge_ = std::move(new_layer_bridge);
canvas2d_bridge_->SetCanvasResourceHost(this);

cc::PaintCanvas* canvas = canvas2d_bridge_->GetPaintCanvas();
// Paint canvas automatically has the clip re-applied. Since image already
// contains clip, it needs to be drawn before the clip stack is re-applied.
// Remove clip from canvas and restore it after the image is drawn.
canvas->restoreToCount(1);
canvas->save();

// TODO(jochin): Consider using ResourceProvider()->RestoreBackBuffer() here
// to avoid all of this clip stack manipulation.
if (image)
canvas2d_bridge_->DrawFullImage(image->PaintImageForCurrentFrame());

RestoreCanvasMatrixClipStack(canvas);
canvas2d_bridge_->DidRestoreCanvasMatrixClipStack(canvas);

RestoreCanvasMatrixClipStack(canvas2d_bridge_->GetPaintCanvas());
canvas2d_bridge_->DidRestoreCanvasMatrixClipStack(
canvas2d_bridge_->GetPaintCanvas());
UpdateMemoryUsage();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,6 @@ void BaseRenderingContext2D::RealizeSaves() {
ValidateStateStack();
if (GetState().HasUnrealizedSaves()) {
DCHECK_GE(state_stack_.size(), 1u);
// GetOrCreatePaintCanvas() can call RestoreMatrixClipStack which syncs
// canvas to state_stack_. Get the canvas before adjusting state_stack_ to
// ensure canvas is synced prior to adjusting state_stack_.
cc::PaintCanvas* canvas = GetOrCreatePaintCanvas();

// Reduce the current state's unrealized count by one now,
// to reflect the fact we are saving one state.
state_stack_.back()->Restore();
Expand All @@ -67,6 +62,7 @@ void BaseRenderingContext2D::RealizeSaves() {
// state (in turn necessary to support correct resizing and unwinding of the
// stack).
state_stack_.back()->ResetUnrealizedSaveCount();
cc::PaintCanvas* canvas = GetOrCreatePaintCanvas();
if (canvas)
canvas->save();
ValidateStateStack();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -441,14 +441,14 @@ cc::PaintCanvas* CanvasRenderingContext2D::GetOrCreatePaintCanvas() {
if (isContextLost())
return nullptr;
if (canvas()->GetOrCreateCanvas2DLayerBridge())
return canvas()->GetCanvas2DLayerBridge()->GetPaintCanvas();
return GetPaintCanvas();
return nullptr;
}

cc::PaintCanvas* CanvasRenderingContext2D::GetPaintCanvas() const {
if (isContextLost() || !canvas()->GetCanvas2DLayerBridge())
if (isContextLost())
return nullptr;
if (canvas() && canvas()->GetCanvas2DLayerBridge()->ResourceProvider())
if (canvas() && canvas()->GetCanvas2DLayerBridge())
return canvas()->GetCanvas2DLayerBridge()->GetPaintCanvas();
return nullptr;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -736,10 +736,13 @@ TEST_F(CanvasRenderingContext2DTest,
size, CanvasColorParams(), kPreferNoAcceleration);
fake_deaccelerate_surface->SetCanvasResourceHost(&host);

cc::PaintCanvas* paint_canvas_ptr =
fake_deaccelerate_surface->GetPaintCanvas();
FakeCanvas2DLayerBridge* surface_ptr = fake_deaccelerate_surface.get();

EXPECT_CALL(*fake_deaccelerate_surface, DrawFullImage(_)).Times(1);
EXPECT_CALL(*fake_deaccelerate_surface, DidRestoreCanvasMatrixClipStack(_))
EXPECT_CALL(*fake_deaccelerate_surface,
DidRestoreCanvasMatrixClipStack(paint_canvas_ptr))
.Times(1);

EXPECT_TRUE(CanvasElement().GetCanvas2DLayerBridge()->IsAccelerated());
Expand Down Expand Up @@ -1140,14 +1143,11 @@ TEST_F(CanvasRenderingContext2DTestAccelerated,
CreateContext(kNonOpaque);
IntSize size(300, 300);
std::unique_ptr<Canvas2DLayerBridge> bridge =
std::make_unique<Canvas2DLayerBridge>(
size, Canvas2DLayerBridge::kEnableAcceleration, CanvasColorParams());
MakeBridge(size, Canvas2DLayerBridge::kEnableAcceleration);
// Force hibernatation to occur in an immediate task.
bridge->DontUseIdleSchedulingForTesting();
CanvasElement().SetResourceProviderForTesting(nullptr, std::move(bridge),
size);
CanvasElement().GetCanvas2DLayerBridge()->SetCanvasResourceHost(
canvas_element_);

EXPECT_TRUE(CanvasElement().GetCanvas2DLayerBridge()->IsAccelerated());
// Take a snapshot to trigger lazy resource provider creation
Expand Down Expand Up @@ -1188,14 +1188,12 @@ TEST_F(CanvasRenderingContext2DTestAccelerated,
CreateContext(kNonOpaque);
IntSize size(300, 300);
std::unique_ptr<Canvas2DLayerBridge> bridge =
std::make_unique<Canvas2DLayerBridge>(
size, Canvas2DLayerBridge::kEnableAcceleration, CanvasColorParams());
MakeBridge(size, Canvas2DLayerBridge::kEnableAcceleration);
// Force hibernatation to occur in an immediate task.
bridge->DontUseIdleSchedulingForTesting();
CanvasElement().SetResourceProviderForTesting(nullptr, std::move(bridge),
size);
CanvasElement().GetCanvas2DLayerBridge()->SetCanvasResourceHost(
canvas_element_);

EXPECT_TRUE(CanvasElement().GetCanvas2DLayerBridge()->IsAccelerated());

EXPECT_TRUE(CanvasElement().GetLayoutBoxModelObject());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "third_party/blink/renderer/platform/fonts/text_run_paint_info.h"
#include "third_party/blink/renderer/platform/graphics/canvas_resource_provider.h"
#include "third_party/blink/renderer/platform/graphics/graphics_types.h"
#include "third_party/blink/renderer/platform/graphics/memory_managed_paint_recorder.h"
#include "third_party/blink/renderer/platform/graphics/paint/paint_canvas.h"
#include "third_party/blink/renderer/platform/graphics/skia/skia_utils.h"
#include "third_party/blink/renderer/platform/graphics/static_bitmap_image.h"
Expand Down Expand Up @@ -85,9 +86,20 @@ OffscreenCanvasRenderingContext2D::OffscreenCanvasRenderingContext2D(
bernoulli_distribution_(kUMASampleProbability) {
is_valid_size_ = IsValidImageSize(Host()->Size());

// Clear the background transparent or opaque.
// A raw pointer is safe here because the callback is only used by the
// recorder_
set_needs_flush_callback_ =
WTF::BindRepeating(&OffscreenCanvasRenderingContext2D::SetNeedsFlush,
WrapWeakPersistent(this));
StartRecording();

// Clear the background transparent or opaque. Similar code at
// CanvasResourceProvider::Clear().
if (IsCanvas2DBufferValid()) {
GetOrCreateCanvasResourceProvider()->Clear();
DCHECK(recorder_);
recorder_->getRecordingCanvas()->clear(
ColorParams().GetOpacityMode() == kOpaque ? SK_ColorBLACK
: SK_ColorTRANSPARENT);
DidDraw();
}

Expand Down Expand Up @@ -119,13 +131,29 @@ void OffscreenCanvasRenderingContext2D::commit() {
GetOffscreenFontCache().PruneLocalFontCache(kMaxCachedFonts);
}

void OffscreenCanvasRenderingContext2D::StartRecording() {
recorder_ =
std::make_unique<MemoryManagedPaintRecorder>(set_needs_flush_callback_);
cc::PaintCanvas* canvas = recorder_->beginRecording(Width(), Height());
// Always save an initial frame, to support resetting the top level matrix
// and clip.
canvas->save();
RestoreMatrixClipStack(canvas);
}

void OffscreenCanvasRenderingContext2D::FlushRecording() {
if (!have_recorded_draw_commands_)
return;

GetCanvasResourceProvider()->FlushCanvas();
{ // Make a new scope so that PaintRecord gets deleted and that gets timed
CanvasResourceProvider* resource_provider = GetCanvasResourceProvider();
cc::PaintCanvas* canvas = resource_provider->Canvas();
canvas->drawPicture(recorder_->finishRecordingAsPicture());
resource_provider->FlushSkia();
}
GetCanvasResourceProvider()->ReleaseLockedImages();

StartRecording();
have_recorded_draw_commands_ = false;
}

Expand All @@ -137,6 +165,7 @@ void OffscreenCanvasRenderingContext2D::FinalizeFrame() {
if (!GetOrCreateCanvasResourceProvider())
return;
FlushRecording();
needs_flush_ = false;
}

// BaseRenderingContext2D implementation
Expand Down Expand Up @@ -181,6 +210,7 @@ OffscreenCanvasRenderingContext2D::GetCanvasResourceProvider() const {
void OffscreenCanvasRenderingContext2D::Reset() {
Host()->DiscardResourceProvider();
BaseRenderingContext2D::Reset();
StartRecording();
// Because the host may have changed to a zero size
is_valid_size_ = IsValidImageSize(Host()->Size());
}
Expand Down Expand Up @@ -232,7 +262,11 @@ ImageBitmap* OffscreenCanvasRenderingContext2D::TransferToImageBitmap(
return nullptr;
}
}

// "Transfer" means no retained buffer. Matrix transformations need to be
// preserved though.
Host()->DiscardResourceProvider();
RestoreMatrixClipStack(recorder_->getRecordingCanvas());

return MakeGarbageCollected<ImageBitmap>(std::move(image));
}
Expand All @@ -259,31 +293,25 @@ bool OffscreenCanvasRenderingContext2D::ParseColorOrCurrentColor(
return ::blink::ParseColorOrCurrentColor(color, color_string, nullptr);
}

cc::PaintCanvas* OffscreenCanvasRenderingContext2D::GetOrCreatePaintCanvas() {
if (!is_valid_size_ || !GetOrCreateCanvasResourceProvider())
return nullptr;
return GetPaintCanvas();
}

cc::PaintCanvas* OffscreenCanvasRenderingContext2D::GetPaintCanvas() const {
if (!is_valid_size_ || !GetCanvasResourceProvider())
if (!is_valid_size_)
return nullptr;
return GetCanvasResourceProvider()->Canvas();
return recorder_->getRecordingCanvas();
}

void OffscreenCanvasRenderingContext2D::DidDraw() {
have_recorded_draw_commands_ = true;
dirty_rect_for_commit_.setWH(Width(), Height());
Host()->DidDraw();
if (GetCanvasResourceProvider() && GetCanvasResourceProvider()->needs_flush())
if (needs_flush_)
FinalizeFrame();
}

void OffscreenCanvasRenderingContext2D::DidDraw(const SkIRect& dirty_rect) {
have_recorded_draw_commands_ = true;
dirty_rect_for_commit_.join(dirty_rect);
Host()->DidDraw(SkRect::Make(dirty_rect_for_commit_));
if (GetCanvasResourceProvider() && GetCanvasResourceProvider()->needs_flush())
if (needs_flush_)
FinalizeFrame();
}

Expand Down Expand Up @@ -341,6 +369,13 @@ bool OffscreenCanvasRenderingContext2D::WritePixels(
DCHECK(IsPaintable());
FinalizeFrame();
have_recorded_draw_commands_ = false;
// Add a save to initialize the transform/clip stack and then restore it after
// the draw. This is needed because each recording initializes and the resets
// this state after every flush.
cc::PaintCanvas* canvas = GetCanvasResourceProvider()->Canvas();
PaintCanvasAutoRestore auto_restore(canvas, true);
if (GetOrCreateCanvasResourceProvider())
RestoreMatrixClipStack(canvas);

return offscreenCanvasForBinding()->ResourceProvider()->WritePixels(
orig_info, pixels, row_bytes, x, y);
Expand Down Expand Up @@ -503,7 +538,7 @@ void OffscreenCanvasRenderingContext2D::DrawTextInternal(
double y,
CanvasRenderingContext2DState::PaintType paint_type,
double* max_width) {
cc::PaintCanvas* paint_canvas = GetOrCreatePaintCanvas();
cc::PaintCanvas* paint_canvas = GetPaintCanvas();
if (!paint_canvas)
return;

Expand Down Expand Up @@ -613,4 +648,7 @@ bool OffscreenCanvasRenderingContext2D::IsCanvas2DBufferValid() const {
return false;
}

void OffscreenCanvasRenderingContext2D::SetNeedsFlush() {
needs_flush_ = true;
}
} // namespace blink
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "third_party/blink/renderer/core/html/canvas/canvas_rendering_context.h"
#include "third_party/blink/renderer/core/html/canvas/canvas_rendering_context_factory.h"
#include "third_party/blink/renderer/modules/canvas/canvas2d/base_rendering_context_2d.h"
#include "third_party/blink/renderer/platform/graphics/paint/paint_recorder.h"

namespace blink {

Expand Down Expand Up @@ -106,7 +107,7 @@ class MODULES_EXPORT OffscreenCanvasRenderingContext2D final

bool ParseColorOrCurrentColor(Color&, const String& color_string) const final;

cc::PaintCanvas* GetOrCreatePaintCanvas() final;
cc::PaintCanvas* GetOrCreatePaintCanvas() final { return GetPaintCanvas(); }
cc::PaintCanvas* GetPaintCanvas() const final;

void DidDraw() final;
Expand Down Expand Up @@ -138,6 +139,8 @@ class MODULES_EXPORT OffscreenCanvasRenderingContext2D final
int y) override;

private:
void StartRecording();
std::unique_ptr<PaintRecorder> recorder_;
bool have_recorded_draw_commands_;
void FinalizeFrame() final;
void FlushRecording();
Expand All @@ -162,6 +165,10 @@ class MODULES_EXPORT OffscreenCanvasRenderingContext2D final

std::mt19937 random_generator_;
std::bernoulli_distribution bernoulli_distribution_;

void SetNeedsFlush();
base::RepeatingClosure set_needs_flush_callback_;
bool needs_flush_ = false;
};

} // namespace blink
Expand Down
Loading

0 comments on commit b94a40a

Please sign in to comment.