Bug 124626 – Allow ImageBuffer to use a backing store that is larger than necessary

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Allow ImageBuffer to use an IOSurface that is larger than necessary

[Myles C. Maxfield]

Created attachment 217375 [details]
Patch

[Brent Fulgham]

Comment on attachment 217375 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217375&action=review

Looks good to me, but I think smfr or someone else smart should review.  I have some concern about that "if () ASSERT" thing.

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:104
> +    float yMagnification = 1.0 * backingStoreSize.height() / internalSize.height();

Should this be 1.0f to avoid partial math using doubles? Or is that actually desired?

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:188
> +        ASSERT(m_data.m_backingStoreSize == m_logicalSize);

Isn't this kind of weird? In a release build will the "if" slurp up the next executable line in the program?

I think it might be a little safer to do something like "ASSERT(acceleratedRendering || (m_data.m_backingStoreSize == m_logicalSize));"

[Myles C. Maxfield]

Comment on attachment 217375 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217375&action=review

>> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:104
>> +    float yMagnification = 1.0 * backingStoreSize.height() / internalSize.height();
> 
> Should this be 1.0f to avoid partial math using doubles? Or is that actually desired?

I was told previously that we don't use the f suffix, though you're right - there is a semantic difference. Does anyone else want to weigh in?

>> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:188
>> +        ASSERT(m_data.m_backingStoreSize == m_logicalSize);
> 
> Isn't this kind of weird? In a release build will the "if" slurp up the next executable line in the program?
> 
> I think it might be a little safer to do something like "ASSERT(acceleratedRendering || (m_data.m_backingStoreSize == m_logicalSize));"

Oh! You're totally right, I didn't even think of that. Fixed.

[Myles C. Maxfield]

Created attachment 217377 [details]
Patch

[Brent Fulgham]

Comment on attachment 217377 [details]
Patch

Looks great to me! I think smfr/thorton/dino should do the final r+.

[Simon Fraser (smfr)]

Comment on attachment 217377 [details]
Patch

Why limit this capability just to IOSurface-backed ImageBuffers? Sure, IOSurfaces are more expensive, but it could be useful to other platforms to re-use buffers too.

[Myles C. Maxfield]

Created attachment 217592 [details]
Patch

[Build Bot]

Comment on attachment 217592 [details]
Patch

Attachment 217592 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/32498029

[Build Bot]

Comment on attachment 217592 [details]
Patch

Attachment 217592 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/32548034

[Myles C. Maxfield]

Created attachment 217601 [details]
Patch

[Myles C. Maxfield]

Created attachment 217741 [details]
Patch

[Myles C. Maxfield]

Created attachment 217977 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 217977 [details]
Patch

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.

[Myles C. Maxfield]

Comment on attachment 217977 [details]
Patch

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?

Renamed. Done.

>> 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?

That wouldn't check the initial multiplication

>> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:170
>> +        ASSERT(!(reinterpret_cast<intptr_t>(m_data.m_data) & 3));
> 
> Please explain this change.

I thought i would clean this up while I was here. Should I split it out into a new patch?

The "&2" was suspicious, so I asked the original author and he said it should have been "&3"

>> 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.

Done.

Done.

>> 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?

This function is rarely called (only when the user explicitly wants an Image of the content (ImageBuffer::copyImage) and when creating a Data URL for the ImageBuffer (ImageBUffer::toDataURL). If you don't do those two things, you should be good. Eventually when people can start using larger buffers, we'll want this to be opt-in.

>> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:245
>> +// 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?

Done.

The function is private and CG-specific; the only callers are in ImageBufferCG.cpp. I have already vetted them.

>> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:316
>> +    CGContextClipToRect(platformContextToClip, FloatRect(FloatPoint(), rect.size()));
> 
> It might be faster to clip to the rect before clipping to the mask.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:352
>> +        sourceCopy->flushContext();
> 
> Do you need this?

No, I don't. Good catch.

>> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:357
>> +        sourceCopy->flushContext();
> 
> Do you need this?

No, I don't. Good catch.

>> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:486
>> +        image = maybeCropToBounds(image, internalSize());
> 
> We can't make a smaller image in one go?

I actually don't believe so. I don't believe there's a call that goes from an IOSurface to a CGImage that only contains a region of the IOSurface (We seem to only have wkIOSurfaceContextCreateImage). I also don't think I can modify CGImageToDataURL because it just calls CGImageDestinationAddImage with the CGImage.

>> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:161
>> +        srcBytesPerRow = m_bytesPerRow.unsafeGet();
> 
> Explain this change?

If the backing surface is wider than size.width(), the srcBytesPerRow will be greater than 4 * size.width(). Otherwise the output looks like a rainbow table. This line is simply an assumption that the backing store is exactly as wide as the image itself.

>> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:388
>> +        destBytesPerRow = m_bytesPerRow.unsafeGet();
> 
> Ditto.

Same explanation.

>> LayoutTests/ChangeLog:19
>> +        canvas-scale-shadowBlur.js
> 
> It's not clear to me if making these changes invalidates the purpose of the tests.

We talked about this offline.

[Myles C. Maxfield]

Created attachment 218245 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 218245 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218245&action=review

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:103
> +    float xMagnification = 1.0 * backingStoreSize.width() / internalSize.width();

The 1.0 * is weird. Why not static_cast<float>()?

> 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));

You still don't explain this in the changelog. When did it get added? How was it wrong?

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

This should return a RetainPtr (or some other smart pointer type). I think createCroppedImageIfNecessary() would be a slightly better name.

[Myles C. Maxfield]

Comment on attachment 218245 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218245&action=review

>> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:103
>> +    float xMagnification = 1.0 * backingStoreSize.width() / internalSize.width();
> 
> The 1.0 * is weird. Why not static_cast<float>()?

Habit. Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:170
>> +        ASSERT(!(reinterpret_cast<intptr_t>(m_data.m_data) & 3));
> 
> You still don't explain this in the changelog. When did it get added? How was it wrong?

I'm going to revert this change. It doesn't belong here, anyway. I'll submit another patch for it.

>> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:203
>> +static CGImageRef maybeCreateCroppedImage(CGImageRef image, const IntSize& bounds)
> 
> This should return a RetainPtr (or some other smart pointer type). I think createCroppedImageIfNecessary() would be a slightly better name.

Done.

[Myles C. Maxfield]

Created attachment 218260 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 218260 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218260&action=review

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:210
> +        image = CGBitmapContextCreateImage(context.get());

Please don't assign to parameters. I think this may also be leaking; you want adoptCF.

[Myles C. Maxfield]

Comment on attachment 218260 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218260&action=review

>> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:210
>> +        image = CGBitmapContextCreateImage(context.get());
> 
> Please don't assign to parameters. I think this may also be leaking; you want adoptCF.

Ah, you're totally right! :O

[Myles C. Maxfield]

Created attachment 218372 [details]
Patch

[Myles C. Maxfield]

Created attachment 218422 [details]
Patch

[Myles C. Maxfield]

I am investigating check-webkit-style to see why it incorrectly thinks a turnary statement is an initializer list

[WebKit Commit Bot]

Attachment 218422 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/canvas/script-tests/canvas-fillPath-shadow.js', u'LayoutTests/fast/canvas/script-tests/canvas-scale-shadowBlur.js', u'LayoutTests/fast/canvas/script-tests/canvas-scale-strokePath-shadow.js', u'LayoutTests/platform/mac/fast/canvas/canvas-scale-shadowBlur-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/ImageBuffer.h', u'Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp', u'Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp', u'Source/WebCore/platform/graphics/cg/ImageBufferDataCG.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:364:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:364:  Comma should be at the beggining of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:365:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:365:  Comma should be at the beggining of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:367:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:367:  Comma should be at the beggining of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:370:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:370:  Comma should be at the beggining of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:371:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:371:  Comma should be at the beggining of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 10 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Style failure was caused by r160084

[WebKit Commit Bot]

Comment on attachment 218422 [details]
Patch

Clearing flags on attachment: 218422

Committed r160121: <http://trac.webkit.org/changeset/160121>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.