[webkit-reviews] review denied: [Bug 124626] Allow ImageBuffer to use a backing store that is larger than necessary : [Attachment 217977] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 124626: Allow ImageBuffer to use a backing store that is larger than
necessary
https://bugs.webkit.org/show_bug.cgi?id=124626

Attachment 217977: Patch
https://bugs.webkit.org/attachment.cgi?id=217977&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=217977&action=review


> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:101
> +static FloatSize computeSizeInUserBounds(const FloatSize& logicalSize, const
IntSize& backingStoreSize, const IntSize& internalSize)

"size in user bounds" is confusing. Size in user space? Bounds in user space?

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:132
> +    m_data.m_bytesPerRow = 4 * Checked<unsigned,
RecordOverflow>(m_data.m_backingStoreSize.width());

Don't you want to do Checked<unsigned, > after multiplying by 4?

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:170
> -	   ASSERT(!(reinterpret_cast<size_t>(m_data.m_data) & 2));
> +	   ASSERT(!(reinterpret_cast<intptr_t>(m_data.m_data) & 3));

Please explain this change.

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:203
> +static RetainPtr<CGImageRef> maybeCropToBounds(RetainPtr<CGImageRef> image,
const IntSize& bounds)

Param should just be a CGImageRef.

I think the name needs to make it clearer that it can return a new image.

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:207
> +	   return CGImageCreateWithImageInRect(image.get(), CGRectMake(0,
static_cast<int>(CGImageGetHeight(image.get())) - bounds.height(),
bounds.width(), bounds.height()));

Is this expensive? Does it blow away any gains from re-using larger buffers?

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:245
> +// The returned image might be larger than the internalSize(). If you want
the smaller
> +// image, crop the result.

Is this comment intended for callers? If so, it should go in the header. It's
also a scarey behavior change; do we need to vet all call sites? How do we find
those affected?

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:316
> +    CGContextClipToMask(platformContextToClip, FloatRect(FloatPoint(),
backingStoreSizeInUserSpace), image.get());
> +    CGContextTranslateCTM(platformContextToClip, 0,
backingStoreSizeInUserSpace.height() - rect.height());
> +    CGContextClipToRect(platformContextToClip, FloatRect(FloatPoint(),
rect.size()));

It might be faster to clip to the rect before clipping to the mask.

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:352
> +    if (sourceCopy->context()->isAcceleratedContext())
> +	   sourceCopy->flushContext();

Do you need this?

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:357
> +    if (sourceCopy->context()->isAcceleratedContext())
> +	   sourceCopy->flushContext();

Do you need this?

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:486
>	   image = copyNativeImage(CopyBackingStore);
> +	   image = maybeCropToBounds(image, internalSize());

We can't make a smaller image in one go?

> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:161
> -	   srcBytesPerRow = 4 * size.width();
> +	   srcBytesPerRow = m_bytesPerRow.unsafeGet();

Explain this change?

> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:388
> -	   destBytesPerRow = 4 * size.width();
> +	   destBytesPerRow = m_bytesPerRow.unsafeGet();

Ditto.

> LayoutTests/ChangeLog:19
> +	   * fast/canvas/script-tests/canvas-fillPath-shadow.js: Don't sample a
canvas at exactly
> +	   the corner of a drawn shape (because the corner might be
antialiased). Instead, sample
> +	   a single pixel inside the shape
> +	   * fast/canvas/script-tests/canvas-scale-shadowBlur.js: Don't sample
a canvas at exactly
> +	   the edge of the blur radius. Instead, sample a single pixel past the
blur radius.
> +	   * fast/canvas/script-tests/canvas-scale-strokePath-shadow.js:
> +	   (shouldBeAround): Allow this test to be less strict when sampling
inside a blurred region
> +	   * platform/mac/fast/canvas/canvas-scale-shadowBlur-expected.txt:
Matching update w/r/t
> +	   canvas-scale-shadowBlur.js

It's not clear to me if making these changes invalidates the purpose of the
tests.