Skip to content

Commit

Permalink
Reland "Not disable acceleration in getImage if its Desynchronized"
Browse files Browse the repository at this point in the history
This reverts commit 49fa3eb.

Reason for revert: Fixing Pixel test.

Original change's description:
> Revert "Not disable acceleration in getImage if its Desynchronized"
>
> This reverts commit edce90d.
>
> Reason for revert: crbug.com/1151440 Pixel_CanvasLowLatency2DImageData
> test consistently fails on Android FYI Release builders, e.g.:
>
> https://ci.chromium.org/p/chromium/builders/ci/Android%20FYI%20Release%20%28Nexus%205%29/b8863094711783933776
>
> Original change's description:
> > Not disable acceleration in getImage if its Desynchronized
> >
> > This is a temporary fix for this issue, that will totally be fixed
> > after we land the new 2d canvas api (that will completely remove the
> > path of code causing this regression in base_rendering_context_2d.cc:
> >   if (!RuntimeEnabledFeatures::NewCanvas2DAPIEnabled()) {
> >     // GetImagedata is faster in Unaccelerated canvases
> >     // Do not disaccelerate if Desync2d Canvas on android.
> >     // Issue 1112060: putImageData() does not work for desynchronized 2D canvas
> >     // on Android
> >     if (IsAccelerated()) {
> >       DisableAcceleration();
> >       base::UmaHistogramEnumeration("Blink.Canvas.GPUFallbackToCPU",
> >                                     GPUFallbackToCPUScenario::kGetImageData);
> >     }
> >   }
> >
> > This CL specifically disables that path if the canvas is Desynchronized.
> > To do so this CL adds some infrastructural elements to be able to access
> > to the desynchronization setting inside getImage.
> >
> > This issue is only happening in a very specific corner case, which
> > proves hard to be tested. This code will be deleted once we enable
> > the new canvas api - so this is a temporary fix.
> >
> > Bug: 1112060
> > Change-Id: I60b90f716332b3969c82dfef3431e4cbd66e6500
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2541063
> > Reviewed-by: Aaron Krajeski <aaronhk@chromium.org>
> > Commit-Queue: Juanmi Huertas <juanmihd@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#829679}
>
> TBR=fserb@chromium.org,aaronhk@chromium.org,juanmihd@chromium.org
>
> Change-Id: I35417b2a319a610b7aec7123112db153cff3a806
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1112060
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2552924
> Commit-Queue: Mingjing Zhang <mjzhang@chromium.org>
> Reviewed-by: Mingjing Zhang <mjzhang@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#829801}

TBR=fserb@chromium.org,aaronhk@chromium.org,juanmihd@chromium.org,mjzhang@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1112060, 1151440
Change-Id: Ic2dae93db31fd8d6ae107db21373468b3ae7cfe5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2553699
Reviewed-by: Aaron Krajeski <aaronhk@chromium.org>
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Juanmi Huertas <juanmihd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#831120}
  • Loading branch information
Juanmihd authored and Commit Bot committed Nov 25, 2020
1 parent 2ff26b9 commit 35918ac
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1696,12 +1696,14 @@ ImageData* BaseRenderingContext2D::getImageDataInternal(

// TODO(crbug.com/1101055): Remove the check for NewCanvas2DAPI flag once
// released.
// New Canvas2D API utilizes willReadFrequently attribute that let the users
// indicate if a canvas will be read frequently through getImageData, thus
// uses CPU rendering from the start in such cases. (crbug.com/1090180)
// TODO(crbug.com/1090180): New Canvas2D API utilizes willReadFrequently
// attribute that let the users indicate if a canvas will be read frequently
// through getImageData, thus uses CPU rendering from the start in such cases.
if (!RuntimeEnabledFeatures::NewCanvas2DAPIEnabled()) {
// GetImagedata is faster in Unaccelerated canvases
if (IsAccelerated()) {
// GetImagedata is faster in Unaccelerated canvases.
// In Desynchronized canvas disabling the acceleration will break
// putImageData: crbug.com/1112060.
if (IsAccelerated() && !IsDesynchronized()) {
DisableAcceleration();
base::UmaHistogramEnumeration("Blink.Canvas.GPUFallbackToCPU",
GPUFallbackToCPUScenario::kGetImageData);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,11 @@ class MODULES_EXPORT BaseRenderingContext2D : public GarbageCollectedMixin,

virtual bool HasAlpha() const = 0;

virtual bool IsDesynchronized() const {
NOTREACHED();
return false;
}

virtual bool isContextLost() const = 0;

virtual void WillDrawImage(CanvasImageSource*) const {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,9 @@ class MODULES_EXPORT CanvasRenderingContext2D final
bool IsAccelerated() const override;
bool IsOriginTopLeft() const override;
bool HasAlpha() const override { return CreationAttributes().alpha; }
bool IsDesynchronized() const override {
return CreationAttributes().desynchronized;
}
void SetIsInHiddenPage(bool) override;
void SetIsBeingDisplayed(bool) override;
void Stop() final;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ class MODULES_EXPORT OffscreenCanvasRenderingContext2D final
void ValidateStateStackWithCanvas(const cc::PaintCanvas*) const final;

bool HasAlpha() const final { return CreationAttributes().alpha; }
bool IsDesynchronized() const final {
return CreationAttributes().desynchronized;
}
bool isContextLost() const override;

ImageBitmap* TransferToImageBitmap(ScriptState*) final;
Expand Down

0 comments on commit 35918ac

Please sign in to comment.