Skip to content

Commit

Permalink
Revert "[Canvas] Simplify DidDraw methods"
Browse files Browse the repository at this point in the history
This reverts commit 8a84dec.

Reason for revert: Suspected for causing FingerprintSetupTest.FingerprintEnrollLimit failures. See https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20(1)/40208/overview

Original change's description:
> [Canvas] Simplify DidDraw methods
>
> This is a code cleanup related to DidDraw methods throughout
> canvas-related code in blink. This CL removes unnecessary overloads,
> unnecessary virtuals, unnecessary argument passing. In WebGL
> rendering contexts, we remove an unnecessary recursion cycle. In
> 2D contexts, we remove an unnecessary and confusing virtual method
> multiple inherritance name conflict (method renamed to DidDraw2D
> in BaseRenderingContext2D).  This CL also removes several unnecessary
> rectangle type (float/int) conversion round trips.
>
> Bug: 1217714
> Change-Id: Ib9a4af628cf8dcb08fd3433197af69d7ea39b422
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2947051
> Reviewed-by: Fernando Serboncini <fserb@chromium.org>
> Reviewed-by: Xida Chen <xidachen@chromium.org>
> Reviewed-by: Kai Ninomiya <kainino@chromium.org>
> Commit-Queue: Justin Novosad <junov@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#891774}

Bug: 1217714
Change-Id: I838363052a075729ae1038ed85358994ae516a1f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2961608
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Kevin McNee <mcnee@google.com>
Reviewed-by: Justin Novosad <junov@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#892271}
  • Loading branch information
kjmcnee authored and Chromium LUCI CQ committed Jun 14, 2021
1 parent 91537c0 commit c988d41
Show file tree
Hide file tree
Showing 20 changed files with 115 additions and 70 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,12 @@ void CanvasRenderingContext::Dispose() {
}

void CanvasRenderingContext::DidDraw(const SkIRect& dirty_rect) {
Host()->DidDraw(dirty_rect);
Host()->DidDraw(SkRect::Make(dirty_rect));
DidDrawCommon();
}

void CanvasRenderingContext::DidDraw() {
Host()->DidDraw();
DidDrawCommon();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,11 +154,8 @@ class CORE_EXPORT CanvasRenderingContext
return nullptr;
}
virtual bool IsPaintable() const = 0;
void DidDraw(const SkIRect& dirty_rect);
void DidDraw() {
return DidDraw(Host() ? SkIRect::MakeWH(Host()->width(), Host()->height())
: SkIRect::MakeEmpty());
}
virtual void DidDraw(const SkIRect& dirty_rect);
virtual void DidDraw();

// Return true if the content is updated.
virtual bool PaintRenderingResultsToCanvas(SourceDrawingBuffer) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ class CORE_EXPORT CanvasRenderingContextHost : public CanvasResourceHost,

virtual void DetachContext() = 0;

virtual void DidDraw(const SkIRect& rect) = 0;
void DidDraw() { DidDraw(SkIRect::MakeWH(width(), height())); }
virtual void DidDraw(const FloatRect& rect) = 0;
virtual void DidDraw() = 0;

virtual void PreFinalizeFrame() = 0;
virtual void PostFinalizeFrame() = 0;
Expand Down
74 changes: 44 additions & 30 deletions third_party/blink/renderer/core/html/canvas/html_canvas_element.cc
Original file line number Diff line number Diff line change
Expand Up @@ -452,8 +452,8 @@ bool HTMLCanvasElement::IsWebGLBlocked() const {
return blocked;
}

void HTMLCanvasElement::DidDraw(const SkIRect& rect) {
if (rect.isEmpty())
void HTMLCanvasElement::DidDraw(const FloatRect& rect) {
if (rect.IsEmpty())
return;
if (GetLayoutObject() && GetLayoutObject()->PreviousVisibilityVisible() &&
GetDocument().GetPage())
Expand All @@ -463,12 +463,18 @@ void HTMLCanvasElement::DidDraw(const SkIRect& rect) {
GetLayoutObject()->SetShouldCheckForPaintInvalidation();
if (IsRenderingContext2D() && context_->ShouldAntialias() && GetPage() &&
GetPage()->DeviceScaleFactorDeprecated() > 1.0f) {
dirty_rect_.join(rect.makeOutset(1, 1));
FloatRect inflated_rect = rect;
inflated_rect.Inflate(1);
dirty_rect_.Unite(inflated_rect);
} else {
dirty_rect_.join(rect);
dirty_rect_.Unite(rect);
}
if (IsRenderingContext2D() && canvas2d_bridge_)
canvas2d_bridge_->DidDraw();
canvas2d_bridge_->DidDraw(rect);
}

void HTMLCanvasElement::DidDraw() {
DidDraw(FloatRect(0, 0, Size().Width(), Size().Height()));
}

void HTMLCanvasElement::PreFinalizeFrame() {
Expand All @@ -482,7 +488,7 @@ void HTMLCanvasElement::PreFinalizeFrame() {

// Low-latency 2d canvases produce their frames after the resource gets single
// buffered.
if (LowLatencyEnabled() && !dirty_rect_.isEmpty() &&
if (LowLatencyEnabled() && !dirty_rect_.IsEmpty() &&
GetOrCreateCanvasResourceProvider(RasterModeHint::kPreferGPU)) {
// TryEnableSingleBuffering() the first time we FinalizeFrame(). This is
// a nop if already single buffered or if single buffering is unsupported.
Expand All @@ -491,19 +497,21 @@ void HTMLCanvasElement::PreFinalizeFrame() {
}

void HTMLCanvasElement::PostFinalizeFrame() {
if (LowLatencyEnabled() && !dirty_rect_.isEmpty() &&
if (LowLatencyEnabled() && !dirty_rect_.IsEmpty() &&
GetOrCreateCanvasResourceProvider(RasterModeHint::kPreferGPU)) {
const base::TimeTicks start_time = base::TimeTicks::Now();
const scoped_refptr<CanvasResource> canvas_resource =
ResourceProvider()->ProduceCanvasResource();
SkIRect damage_rect = SkIRect::MakeWH(Size().Width(), Size().Height());
damage_rect.intersect(dirty_rect_);

const FloatRect src_rect(0, 0, Size().Width(), Size().Height());
dirty_rect_.Intersect(src_rect);
const IntRect int_dirty = EnclosingIntRect(dirty_rect_);
const SkIRect damage_rect = SkIRect::MakeXYWH(
int_dirty.X(), int_dirty.Y(), int_dirty.Width(), int_dirty.Height());
const bool needs_vertical_flip = !RenderingContext()->IsOriginTopLeft();
frame_dispatcher_->DispatchFrame(std::move(canvas_resource), start_time,
damage_rect, needs_vertical_flip,
IsOpaque());
dirty_rect_.setEmpty();
dirty_rect_ = FloatRect();
}

// If the canvas is visible, notifying listeners is taken care of in
Expand Down Expand Up @@ -541,36 +549,42 @@ void HTMLCanvasElement::SetNeedsCompositingUpdate() {
}

void HTMLCanvasElement::DoDeferredPaintInvalidation() {
DCHECK(!dirty_rect_.isEmpty());
DCHECK(!dirty_rect_.IsEmpty());
if (LowLatencyEnabled()) {
// Low latency canvas handles dirty propagation in FinalizeFrame();
return;
}
LayoutBox* layout_box = GetLayoutBox();

FloatRect content_rect;
if (layout_box) {
if (auto* replaced = DynamicTo<LayoutReplaced>(layout_box))
content_rect = FloatRect(replaced->ReplacedContentRect());
else
content_rect = FloatRect(layout_box->PhysicalContentBoxRect());
}

if (IsRenderingContext2D()) {
if (dirty_rect_.isEmpty()) // TODO(junov): Do we still need this?
return;
IntRect src_rect(0, 0, Size().Width(), Size().Height());
IntRect invalidation_rect(dirty_rect_);
invalidation_rect.Intersect(src_rect);
FloatRect src_rect(0, 0, Size().Width(), Size().Height());
dirty_rect_.Intersect(src_rect);

FloatRect invalidation_rect;
if (layout_box) {
FloatRect content_rect;
if (auto* replaced = DynamicTo<LayoutReplaced>(layout_box))
content_rect = FloatRect(replaced->ReplacedContentRect());
else
content_rect = FloatRect(layout_box->PhysicalContentBoxRect());

FloatRect mapped_invalidation_rect = MapRect(
FloatRect(invalidation_rect), FloatRect(src_rect), content_rect);
FloatRect mapped_dirty_rect =
MapRect(dirty_rect_, src_rect, content_rect);
if (context_->IsComposited()) {
// Composited 2D canvases need the dirty rect to be expressed relative
// to the content box, as opposed to the layout box.
mapped_invalidation_rect.MoveBy(-content_rect.Location());
mapped_dirty_rect.MoveBy(-content_rect.Location());
}
invalidation_rect = EnclosingIntRect(mapped_invalidation_rect);
invalidation_rect = mapped_dirty_rect;
} else {
invalidation_rect = dirty_rect_;
}

if (dirty_rect_.IsEmpty())
return;

if (canvas2d_bridge_)
canvas2d_bridge_->DoPaintInvalidation(invalidation_rect);
}
Expand All @@ -587,14 +601,14 @@ void HTMLCanvasElement::DoDeferredPaintInvalidation() {
layout_box->SetShouldDoFullPaintInvalidation();
}

dirty_rect_.setEmpty();
dirty_rect_ = FloatRect();
}

void HTMLCanvasElement::Reset() {
if (ignore_reset_)
return;

dirty_rect_.setEmpty();
dirty_rect_ = FloatRect();

bool had_resource_provider = HasResourceProvider();

Expand Down Expand Up @@ -1222,7 +1236,7 @@ void HTMLCanvasElement::SetResourceProviderForTesting(
void HTMLCanvasElement::DiscardResourceProvider() {
canvas2d_bridge_.reset();
CanvasResourceHost::DiscardResourceProvider();
dirty_rect_.setEmpty();
dirty_rect_ = FloatRect();
}

void HTMLCanvasElement::PageVisibilityChanged() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,9 @@ class CORE_EXPORT HTMLCanvasElement final
// Derived from OffscreenCanvasPlaceholder.
bool HasCanvasCapture() const final { return !listeners_.IsEmpty(); }

// Used for rendering.
void DidDraw(const SkIRect&) override;
using CanvasRenderingContextHost::DidDraw;
// Used for rendering
void DidDraw(const FloatRect&) override;
void DidDraw() override;

void Paint(GraphicsContext&,
const PhysicalRect&,
Expand Down Expand Up @@ -182,7 +182,7 @@ class CORE_EXPORT HTMLCanvasElement final

InsertionNotificationRequest InsertedInto(ContainerNode&) override;

bool IsDirty() { return !dirty_rect_.isEmpty(); }
bool IsDirty() { return !dirty_rect_.IsEmpty(); }

void DoDeferredPaintInvalidation();

Expand Down Expand Up @@ -392,7 +392,7 @@ class CORE_EXPORT HTMLCanvasElement final
bool canvas_is_clear_ = true;

bool ignore_reset_;
SkIRect dirty_rect_;
FloatRect dirty_rect_;

bool origin_clean_;
bool needs_unbuffered_input_ = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -467,8 +467,12 @@ CanvasResourceProvider* OffscreenCanvas::GetOrCreateResourceProvider() {
return ResourceProvider();
}

void OffscreenCanvas::DidDraw(const SkIRect& rect) {
if (rect.isEmpty())
void OffscreenCanvas::DidDraw() {
DidDraw(FloatRect(0, 0, Size().Width(), Size().Height()));
}

void OffscreenCanvas::DidDraw(const FloatRect& rect) {
if (rect.IsEmpty())
return;

if (HasPlaceholderCanvas()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,8 @@ class CORE_EXPORT OffscreenCanvas final
bool PushFrameIfNeeded();
bool PushFrame(scoped_refptr<CanvasResource> frame,
const SkIRect& damage_rect) override;
void DidDraw(const SkIRect&) override;
using CanvasRenderingContextHost::DidDraw;
void DidDraw(const FloatRect&) override;
void DidDraw() override;
void Commit(scoped_refptr<CanvasResource> bitmap_image,
const SkIRect& damage_rect) override;
bool ShouldAccelerate2dContext() const override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1266,12 +1266,12 @@ void BaseRenderingContext2D::clearRect(double x,
kClipFill);
c = GetPaintCanvas(); // Check overdraw may have swapped the PaintCanvas
c->drawRect(rect, clear_flags);
DidDraw2D(clip_bounds);
DidDraw(clip_bounds);
} else {
SkIRect dirty_rect;
if (ComputeDirtyRect(rect, clip_bounds, &dirty_rect)) {
c->drawRect(rect, clear_flags);
DidDraw2D(dirty_rect);
DidDraw(dirty_rect);
}
}
}
Expand Down Expand Up @@ -2138,7 +2138,7 @@ void BaseRenderingContext2D::putImageData(ImageData* data,
}
}

DidDraw2D(dest_rect);
DidDraw(dest_rect);
}

void BaseRenderingContext2D::PutByteArray(const SkPixmap& source,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ class MODULES_EXPORT BaseRenderingContext2D : public GarbageCollectedMixin,
virtual cc::PaintCanvas* GetOrCreatePaintCanvas() = 0;
virtual cc::PaintCanvas* GetPaintCanvas() const = 0;

virtual void DidDraw2D(const SkIRect& dirty_rect) = 0;
virtual void DidDraw(const SkIRect& dirty_rect) = 0;

virtual bool StateHasFilter() = 0;
virtual sk_sp<PaintFilter> StateGetFilter() = 0;
Expand Down Expand Up @@ -577,13 +577,13 @@ void BaseRenderingContext2D::DrawInternal(
(GetState().ShouldDrawShadows() &&
ShouldUseDropShadowPaintFilter(paint_type, image_type))) {
CompositedDraw(draw_func, GetPaintCanvas(), paint_type, image_type);
DidDraw2D(clip_bounds);
DidDraw(clip_bounds);
} else if (GetState().GlobalComposite() == SkBlendMode::kSrc) {
ClearCanvas(); // takes care of checkOverdraw()
const PaintFlags* flags =
GetState().GetFlags(paint_type, kDrawForegroundOnly, image_type);
draw_func(GetPaintCanvas(), flags);
DidDraw2D(clip_bounds);
DidDraw(clip_bounds);
} else {
SkIRect dirty_rect;
if (ComputeDirtyRect(bounds, clip_bounds, &dirty_rect)) {
Expand All @@ -593,7 +593,7 @@ void BaseRenderingContext2D::DrawInternal(
draw_covers_clip_bounds(clip_bounds))
CheckOverdraw(bounds, flags, image_type, kClipFill);
draw_func(GetPaintCanvas(), flags);
DidDraw2D(dirty_rect);
DidDraw(dirty_rect);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,10 @@ void CanvasRenderingContext2D::clearRect(double x,
}
}

void CanvasRenderingContext2D::DidDraw2D(const SkIRect& dirty_rect) {
void CanvasRenderingContext2D::DidDraw(const SkIRect& dirty_rect) {
if (dirty_rect.isEmpty())
return;

CanvasRenderingContext::DidDraw(dirty_rect);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,6 @@ class MODULES_EXPORT CanvasRenderingContext2D final
void ClearFilterReferences();

// BaseRenderingContext2D implementation
void DidDraw2D(const SkIRect& dirty_rect) final;
bool OriginClean() const final;
void SetOriginTainted() final;
bool WouldTaintOrigin(CanvasImageSource* source) final {
Expand All @@ -198,6 +197,7 @@ class MODULES_EXPORT CanvasRenderingContext2D final
cc::PaintCanvas* GetOrCreatePaintCanvas() final;
cc::PaintCanvas* GetPaintCanvas() const final;

void DidDraw(const SkIRect& dirty_rect) final;
scoped_refptr<StaticBitmapImage> GetImage() final;

bool StateHasFilter() final;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -279,9 +279,16 @@ cc::PaintCanvas* OffscreenCanvasRenderingContext2D::GetPaintCanvas() const {
return GetCanvasResourceProvider()->Canvas();
}

void OffscreenCanvasRenderingContext2D::DidDraw2D(const SkIRect& dirty_rect) {
void OffscreenCanvasRenderingContext2D::DidDraw() {
dirty_rect_for_commit_.setWH(Width(), Height());
Host()->DidDraw();
if (GetCanvasResourceProvider() && GetCanvasResourceProvider()->needs_flush())
FinalizeFrame();
}

void OffscreenCanvasRenderingContext2D::DidDraw(const SkIRect& dirty_rect) {
dirty_rect_for_commit_.join(dirty_rect);
Host()->DidDraw(dirty_rect_for_commit_);
Host()->DidDraw(SkRect::Make(dirty_rect_for_commit_));
if (GetCanvasResourceProvider() && GetCanvasResourceProvider()->needs_flush())
FinalizeFrame();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,8 @@ class MODULES_EXPORT OffscreenCanvasRenderingContext2D final
cc::PaintCanvas* GetOrCreatePaintCanvas() final;
cc::PaintCanvas* GetPaintCanvas() const final;

void DidDraw2D(const SkIRect& dirty_rect) final;
void DidDraw() final;
void DidDraw(const SkIRect& dirty_rect) final;

bool StateHasFilter() final;
sk_sp<PaintFilter> StateGetFilter() final;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ void PaintRenderingContext2D::InitializePaintRecorder() {
did_record_draw_commands_in_paint_recorder_ = false;
}

void PaintRenderingContext2D::DidDraw2D(const SkIRect&) {
void PaintRenderingContext2D::DidDraw(const SkIRect&) {
did_record_draw_commands_in_paint_recorder_ = true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class MODULES_EXPORT PaintRenderingContext2D : public ScriptWrappable,
cc::PaintCanvas* GetOrCreatePaintCanvas() final { return GetPaintCanvas(); }
cc::PaintCanvas* GetPaintCanvas() const final;

void DidDraw2D(const SkIRect&) final;
void DidDraw(const SkIRect&) final;

double shadowOffsetX() const final;
void setShadowOffsetX(double) final;
Expand Down

0 comments on commit c988d41

Please sign in to comment.