2D Canvas: Refactor code re-attempting to allocate an imageBuffer.

2D Canvas: Refactor code re-attempting to allocate an imageBuffer.

m_hasCreatedImageBuffer prevents HTMLCanvasElement::buffer()
from continuously re-attempting to allocate an imageBuffer after the first
attempt failed. This has huge implications for performance in low memory
conditions.

m_hasCreatedImageBuffer was misnamed, so it ended-up being misused in several
places.

This CL renames m_hasCreatedImageBuffer to m_didFailToCreateImageBuffer, and
then m_didFailToCreateImageBuffer is used in only relevant places.

TEST=fast/canvas/canvas-extremely-large-dimensions.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162330

[aandrey, 2013-10-25 20:03:35]

https://codereview.chromium.org/44253005/diff/40001/Source/core/html/HTMLCanv...
File Source/core/html/HTMLCanvasElement.h (left):

https://codereview.chromium.org/44253005/diff/40001/Source/core/html/HTMLCanv...
Source/core/html/HTMLCanvasElement.h:172: // m_createdImageBuffer means we tried
to malloc the buffer.  We didn't necessarily get it.
is this comment still relevant?

[dshwang, 2013-10-25 20:41:05]

On 2013/10/25 20:03:35, aandrey wrote:
>
https://codereview.chromium.org/44253005/diff/40001/Source/core/html/HTMLCanv...
> File Source/core/html/HTMLCanvasElement.h (left):
> 
>
https://codereview.chromium.org/44253005/diff/40001/Source/core/html/HTMLCanv...
> Source/core/html/HTMLCanvasElement.h:172: // m_createdImageBuffer means we
tried
> to malloc the buffer.  We didn't necessarily get it.
> is this comment still relevant?

Thank you. It's stale comment. I removed it.

[Justin Novosad, 2013-10-25 21:51:18]

not lgtm

m_hasCreatedImageBuffer is very useful.  It prevents HTMLCanvasElement::buffer()
from continuously re-attempting to allocate an imageBuffer after the first
attempt failed.  This has huge implications for performance in low memory
conditions.

I think m_hasCreatedImageBuffer was misnamed. Because it was misnamed, it
ended-up being misused in a lot of places.  Those are things that should be
fixed.

[Justin Novosad, 2013-10-25 21:54:20]

On 2013/10/25 21:51:18, junov wrote:
> This has huge implications for performance in low memory
> conditions.
It also prevents the browser from displaying incomplete or corrupted frames. The
canvas contents are either correct or blank depending on whether the initial
allocation succeeded.  We must retain that behavior.

[dshwang, 2013-10-25 23:53:47]

On 2013/10/25 21:54:20, junov wrote:
> On 2013/10/25 21:51:18, junov wrote:
> > This has huge implications for performance in low memory
> > conditions.
> It also prevents the browser from displaying incomplete or corrupted frames.
The
> canvas contents are either correct or blank depending on whether the initial
> allocation succeeded.  We must retain that behavior.

Thank you for explanation! I need to check it more carefully.

[Justin Novosad, 2013-10-28 14:21:20]

On 2013/10/25 23:53:47, dshwang wrote:
> On 2013/10/25 21:54:20, junov wrote:
> > On 2013/10/25 21:51:18, junov wrote:
> > > This has huge implications for performance in low memory
> > > conditions.
> > It also prevents the browser from displaying incomplete or corrupted frames.
> The
> > canvas contents are either correct or blank depending on whether the initial
> > allocation succeeded.  We must retain that behavior.
> 
> Thank you for explanation! I need to check it more carefully.

Furthermore, since this change fixes a bug, please add a test (unit or layout). 
If you need trigger a canvas allocation failure on purpose for a layout test,
just try creating one with an insanely large size.  It would be great to have
better test coverage for cases that challenge stability.

[dshwang, 2013-10-28 14:41:36]

On 2013/10/28 14:21:20, junov wrote:
> Furthermore, since this change fixes a bug, please add a test (unit or
layout). 
> If you need trigger a canvas allocation failure on purpose for a layout test,
> just try creating one with an insanely large size.  It would be great to have
> better test coverage for cases that challenge stability.

Thank you for good suggestion. I'll do that soon :)

[dshwang, 2013-11-18 19:51:07]

Hi, long time no see :)

I update this CL as junov@ advised. and reuse junov@'s great comments in commit
log.

1. retain the behavior.
2. rename m_hasCreatedImageBuffer to m_didFailToCreateImageBuffer to avoid
misuse.
3. get rid of misuse cases.

Could you review again? Thanks in advance.

[Justin Novosad, 2013-11-18 20:01:48]

On 2013/11/18 19:51:07, dshwang wrote:
> Hi, long time no see :)
> 
> I update this CL as junov@ advised. and reuse junov@'s great comments in
commit
> log.
> 
> 1. retain the behavior.
> 2. rename m_hasCreatedImageBuffer to m_didFailToCreateImageBuffer to avoid
> misuse.
> 3. get rid of misuse cases.
> 
> Could you review again? Thanks in advance.

Yes!

I just have one minor change to suggest: "insanely" -> "extremely"
The only thing I see here that is insane is that we weren't handling ImageBuffer
creation failures correctly until now :-)

Thanks.

non-owner lgtm

[dshwang, 2013-11-18 20:28:07]

On 2013/11/18 20:01:48, junov wrote:
> I just have one minor change to suggest: "insanely" -> "extremely"
> The only thing I see here that is insane is that we weren't handling
ImageBuffer
> creation failures correctly until now :-)

Thank you for quick review! Done.

> non-owner lgtm

senorblanco@, could you review?

[Stephen White, 2013-11-18 21:42:13]

Thanks for the patch. My comments are mostly just spelling/grammar nits.

https://codereview.chromium.org/44253005/diff/230001/LayoutTests/fast/canvas/...
File LayoutTests/fast/canvas/script-tests/canvas-extremely-large-dimensions.js
(right):

https://codereview.chromium.org/44253005/diff/230001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/script-tests/canvas-extremely-large-dimensions.js:10:
debug("check whether crash or not with extremely large size.");
Nit: check for crash on extremely large canvas size.

https://codereview.chromium.org/44253005/diff/230001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/script-tests/canvas-extremely-large-dimensions.js:19:
debug("check whether crash after reset to the same size.");
Nit: check for crash after resetting to the same size.

https://codereview.chromium.org/44253005/diff/230001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/script-tests/canvas-extremely-large-dimensions.js:30:
debug("check whether crash after resize to googol.");
Nit: check for crash after resizing to googol.

(heh.. googol)

https://codereview.chromium.org/44253005/diff/230001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/script-tests/canvas-extremely-large-dimensions.js:40:
debug("check whether crash after reset to the same size.");
Nit: check for crash after resetting to the same size.

https://codereview.chromium.org/44253005/diff/230001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/script-tests/canvas-extremely-large-dimensions.js:49:
debug("check again whether crash or not with extremely large size.");
Nit: check again for crash with extremely large size.

https://codereview.chromium.org/44253005/diff/230001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/script-tests/canvas-extremely-large-dimensions.js:62:
for(var i = 0; i < 10000; i++) {
I'm a little nervous about 10,000 calls to getImageData(). How fast does this
test run, in virtual and real GPU mode?

https://codereview.chromium.org/44253005/diff/230001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/script-tests/canvas-extremely-large-dimensions.js:76:
debug("after resize to normal size, canvas must be in valid state.");
Nit: after resizing to normal size, the canvas must be in a valid state.

[dshwang, 2013-11-19 10:05:07]

On 2013/11/18 21:42:13, Stephen White wrote:
> Thanks for the patch. My comments are mostly just spelling/grammar nits.
> 
>
https://codereview.chromium.org/44253005/diff/230001/LayoutTests/fast/canvas/...
> LayoutTests/fast/canvas/script-tests/canvas-extremely-large-dimensions.js:10:
> debug("check whether crash or not with extremely large size.");
> Nit: check for crash on extremely large canvas size.

Thank you a lot for rephrasing my bad sentences!! Done.

>
https://codereview.chromium.org/44253005/diff/230001/LayoutTests/fast/canvas/...
> LayoutTests/fast/canvas/script-tests/canvas-extremely-large-dimensions.js:62:
> for(var i = 0; i < 10000; i++) {
> I'm a little nervous about 10,000 calls to getImageData(). How fast does this
> test run, in virtual and real GPU mode?

It's good point. I agree with you. I change 10,000 to 100, and width and height
to 1.

In detail, in this case, getImageData() is not related to GPU memory, because
canvas()->buffer() is null.
The code creates 1 by 1 ImageData per loop. I think creating 100 ImageDatas is
cheaper than other tests with complex dom structure.

PassRefPtr<ImageData>
CanvasRenderingContext2D::getImageData(ImageBuffer::CoordinateSystem
coordinateSystem, float sx, float sy, float sw, float sh, ExceptionState&
exceptionState) const
{
    ...
    ImageBuffer* buffer = canvas()->buffer();
    if (!buffer)
        return createEmptyImageData(imageDataRect.size());
    ...
}

[dshwang, 2013-11-19 10:47:10]

I found more related to this CL.

We don't create extremely big image buffer already.

static const int MaxCanvasArea = 32768 * 8192;

void HTMLCanvasElement::createImageBuffer()
{
    ...
    IntSize deviceSize = convertLogicalToDevice(size());
    if (deviceSize.width() * deviceSize.height() > MaxCanvasArea)
        return;
    ...
}


It means code already does not re-attempt allocation of a image buffer
implicitly.
m_didFailToCreateImageBuffer makes code more explicit, but in theory,
m_didFailToCreateImageBuffer is not necessary.

Which is better: retaining m_didFailToCreateImageBuffer or removing
m_didFailToCreateImageBuffer?

[dshwang, 2013-11-19 12:33:37]

On 2013/11/19 10:47:10, dshwang wrote:
> Which is better: retaining m_didFailToCreateImageBuffer or removing
> m_didFailToCreateImageBuffer?

After rethinking, m_didFailToCreateImageBuffer is still needed, because we can
encounter OOM although we need pixels less than 32768 * 8192.

On the other hands, new test can not create a image buffer internally although
there is plenty of memory, because required pixels is bigger than 32768 * 8192.
The new test is valid from Web API's perspective view. but the test can not
check if m_didFailToCreateImageBuffer is valid or not.

In conclusion, I think
1. code change looks good.
2. test can not check code change. but it is not bad to retain it.

what do you think?

[dshwang, 2013-11-19 13:08:49]

Oops, sorry. I correct my mentions again.

The test is vaild.

regardless of why failing to create an image buffer, we set
m_didFailToCreateImageBuffer to true.
32768 * 8192 is just one of the reasons.

In conclusion,
1. code is good.
2. test is good.

[Stephen White, 2013-11-19 14:12:54]

On 2013/11/19 13:08:49, dshwang wrote:
> Oops, sorry. I correct my mentions again.
> 
> The test is vaild.
> 
> regardless of why failing to create an image buffer, we set
> m_didFailToCreateImageBuffer to true.
> 32768 * 8192 is just one of the reasons.
> 
> In conclusion,
> 1. code is good.
> 2. test is good.

The new test seems to be failing on Windows and Mac:

https://storage.googleapis.com/chromium-layout-test-archives/win_blink_rel/17...
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/11...

(Note to Justin: we seem to be getting a crash on Windows; it seems to be
failing to create a GDI-backed bitmap, even for the accelerated path, which
seems quite odd.)

[dshwang, 2013-11-19 14:21:53]

On 2013/11/19 14:12:54, Stephen White wrote:
> On 2013/11/19 13:08:49, dshwang wrote:
> > Oops, sorry. I correct my mentions again.
> > 
> > The test is vaild.
> > 
> > regardless of why failing to create an image buffer, we set
> > m_didFailToCreateImageBuffer to true.
> > 32768 * 8192 is just one of the reasons.
> > 
> > In conclusion,
> > 1. code is good.
> > 2. test is good.
> 
> The new test seems to be failing on Windows and Mac:
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/win_blink_rel/17...
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/11...

I think if https://codereview.chromium.org/74533004/ is landed, this crash will
be resolved.
currently, new test tries to create -2147483648 * -2147483648 image buffer.

could you review 74533004? and then I will kick try-bot again.


> (Note to Justin: we seem to be getting a crash on Windows; it seems to be
> failing to create a GDI-backed bitmap, even for the accelerated path, which
> seems quite odd.)

Yes, it is because we fallback if size is bigger than (perhaps) 4096 * 4096.

[Stephen White, 2013-11-19 14:34:57]

On 2013/11/19 14:21:53, dshwang wrote:
> On 2013/11/19 14:12:54, Stephen White wrote:
> > On 2013/11/19 13:08:49, dshwang wrote:
> > > Oops, sorry. I correct my mentions again.
> > > 
> > > The test is vaild.
> > > 
> > > regardless of why failing to create an image buffer, we set
> > > m_didFailToCreateImageBuffer to true.
> > > 32768 * 8192 is just one of the reasons.
> > > 
> > > In conclusion,
> > > 1. code is good.
> > > 2. test is good.
> > 
> > The new test seems to be failing on Windows and Mac:
> > 
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/win_blink_rel/17...
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/11...
> 
> I think if https://codereview.chromium.org/74533004/ is landed, this crash
will
> be resolved.
> currently, new test tries to create -2147483648 * -2147483648 image buffer.
> 
> could you review 74533004? and then I will kick try-bot again.
> 
> 
> > (Note to Justin: we seem to be getting a crash on Windows; it seems to be
> > failing to create a GDI-backed bitmap, even for the accelerated path, which
> > seems quite odd.)
> 
> Yes, it is because we fallback if size is bigger than (perhaps) 4096 * 4096.

IIRC, it should be trying to create a non-platform canvas (ie., non-GDI).

[dshwang, 2013-11-19 17:23:38]

On 2013/11/19 14:34:57, Stephen White wrote:
> IIRC, it should be trying to create a non-platform canvas (ie., non-GDI).

Don't worry, Window create a non-platform canvas. I explain what happen.

logical width 2147483647 becomes device width -2147483648
Window fails to create a non-platform canvas with -2147483648 * -2147483648
dimension.
And crash during creating a platform canvas through
skia::TryCreateBitmapCanvas().

ImageBuffer::ImageBuffer(const IntSize& size, float resolutionScale,
RenderingMode renderingMode, OpacityMode opacityMode, int
acceleratedSampleCount, bool& success)
    : m_size(size)
    , m_logicalSize(size)
    , m_resolutionScale(resolutionScale)
{
    ...
    if (renderingMode == UnacceleratedNonPlatformBuffer)
        m_canvas = createNonPlatformCanvas(size);

    if (!m_canvas)
        m_canvas = adoptRef(skia::TryCreateBitmapCanvas(size.width(),
size.height(), false));
    ...
}


Patch Set 7 is merged with https://codereview.chromium.org/74533004/, so this
bug is fixed.

[Stephen White, 2013-11-19 19:02:12]

LGTM

[dshwang, 2013-11-19 19:12:03]

On 2013/11/19 19:02:12, Stephen White wrote:
> LGTM

thank you for review

[commit-bot: I haz the power, 2013-11-19 19:12:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/44253005/420001

[commit-bot: I haz the power, 2013-11-20 04:37:16]

Change committed as 162330