Bug 206621 – Make ImageBuffer::getImageData() and putImageData() return and take ImageData

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Said Abou-Hallawa]

This will make the prototype of these functions match the prototype of CanvasRenderingContext2D.getImageData() and CanvasRenderingContext2D.putImageData().

[Said Abou-Hallawa]

Created attachment 388479 [details]
Patch

[Said Abou-Hallawa]

Created attachment 388487 [details]
Patch

[Said Abou-Hallawa]

Created attachment 388488 [details]
Patch

[Said Abou-Hallawa]

Created attachment 388489 [details]
Patch

[Said Abou-Hallawa]

Created attachment 388550 [details]
Patch

[Said Abou-Hallawa]

Created attachment 388556 [details]
Patch

[Said Abou-Hallawa]

Created attachment 388566 [details]
Patch

[Said Abou-Hallawa]

Windows was crashing when running the layout test ast/canvas/canvas-toDataURL-jpeg-crash.html 

PROBLEM_CLASSES: 

    ID:     [0n313]
    Type:   [@ACCESS_VIOLATION]
    Class:  Addendum
    Scope:  BUCKET_ID
    Name:   Omit
    Data:   Omit
    PID:    [Unspecified]
    TID:    [0xd48]
    Frame:  [0] : VCRUNTIME140!memcpy

    ID:     [0n285]
    Type:   [INVALID_POINTER_READ]
    Class:  Primary
    Scope:  DEFAULT_BUCKET_ID (Failure Bucket ID prefix)
            BUCKET_ID
    Name:   Add
    Data:   Omit
    PID:    [Unspecified]
    TID:    [0xd48]
    Frame:  [0] : VCRUNTIME140!memcpy

LAST_CONTROL_TRANSFER:  from 00007ffa3bb08474 to 00007ffa493c1480

STACK_TEXT:  
000000e7`772fb438 00007ffa`3bb08474 : 00000000`00001f40 000000e7`772fb570 00000000`00001f40 00000000`00000000 : VCRUNTIME140!memcpy+0x190
000000e7`772fb440 00007ffa`3bcee330 : 000002c2`fe682670 000000e7`772fb570 000002c2`ff1e66a0 000000e7`772fc90e : CoreGraphics!CGAccessSessionGetBytes+0xa4
000000e7`772fb470 00007ffa`3bca5184 : 000002c2`ff216fe0 000002c2`ba1a6d50 000000e7`772fcb88 000002c2`ff210a60 : CoreGraphics!CGImageCreateCopyWithAlphaInfo+0x462f0
000000e7`772fc970 00007ffa`14ad5038 : 000002c2`00000001 000002c2`ff210a60 000002c2`ba1a6d50 000002c2`00000008 : CoreGraphics!CGImageDestinationFinalize+0xc4
000000e7`772fc9a0 00007ffa`1434d262 : 000002c2`ff1e66a0 000000e7`00000000 00000000`00f42400 000002c2`fe6825e0 : WebKit!WebCore::encodeImage+0x138
000000e7`772fca20 00007ffa`1434d3c8 : 000002c2`ba187760 000000e7`772fcba8 000002c2`fe660580 00007ffa`146d4b5f : WebKit!WebCore::ImageBuffer::toCFData+0x4c2
000000e7`772fcb20 00007ffa`143c61fd : 000002c2`fe660580 00000000`00000000 00000000`00000000 00000000`0000000a : WebKit!WebCore::ImageBuffer::toDataURL+0x48
000000e7`772fcb70 00007ffa`1441b22d : 00000000`00000001 000002c2`ff354000 000000e7`772fcc90 00007ffa`2982b357 : WebKit!WebCore::HTMLCanvasElement::toDataURL+0x15d
000000e7`772fcc10 00007ffa`1441b0c3 : 000002c2`fdd90080 000002c2`fe68da08 000000e7`772fcc70 000002c2`bd9b11be : WebKit!WebCore::jsHTMLCanvasElementPrototypeFunctionToDataURLBody+0x14d
000000e7`772fcca0 000002c2`bd9b11be : 000000e7`772fcd90 00007ffa`29b20cfc 000002c2`ff354000 000002c2`ff2421c0 : WebKit!WebCore::jsHTMLCanvasElementPrototypeFunctionToDataURL+0x63

This was because I moved the declaration:

    RefPtr<Uint8ClampedArray> premultipliedData;

inside the if statement:

    if (CFEqual(uti.get(), jpegUTI())) {
        RefPtr<Uint8ClampedArray> premultipliedData;
    }

This caused the Uint8ClampedArray to be freed before calling encodeImage().

[Simon Fraser (smfr)]

Comment on attachment 388566 [details]
Patch

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

I think this is OK but the two apparent behavior changes worry me. Can we undo those parts?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2788
> +        auto imageData = ImageData::create(IntSize(logicalWidth, logicalHeight), makeRef(*array));

Is this a data copy? If not, can it WTFMove the array?

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:442
> +        protectedPixelArray = makeRefPtr(imageData->data());

This is not a copy, right?

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:451
> +        image = adoptCF(CGImageCreate(pixelArrayDimensions.width(), pixelArrayDimensions.height(), 8, 32, 4 * pixelArrayDimensions.width(), sRGBColorSpaceRef(), kCGBitmapByteOrderDefault | kCGImageAlphaNoneSkipLast, dataProvider.get(), 0, false, kCGRenderingIntentDefault));

This seems like a behavior change, because the returned CGImage used to be sized based on logical size, but now it's sized based on internal size.

> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:133
> +    auto* result = imageData ? imageData->data() : nullptr;
> +    if (!result)

We use "result" as the value returned by the function, but that's not the case here.

> Source/WebCore/platform/graphics/filters/FEDropShadow.cpp:102
> +    IntRect shadowArea(IntPoint(), resultImage->logicalSize());

This seems like a behavior change too. Did you test SVG blur filters on Retina?

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:522
> +    return m_unmultipliedImageResult->data();

I don't like that we pass raw pointers to these buffers around, with undetermined lifetimes.

[Said Abou-Hallawa]

Created attachment 388884 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 388566 [details]
Patch

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

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2788
>> +        auto imageData = ImageData::create(IntSize(logicalWidth, logicalHeight), makeRef(*array));
> 
> Is this a data copy? If not, can it WTFMove the array?

No it is not a data copy. So I used array.releaseNonNull() since array is a RefPtr<Uint8ClampedArray> and ImageData::create() expects Ref<Uint8ClampedArray>&&.

>> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:442
>> +        protectedPixelArray = makeRefPtr(imageData->data());
> 
> This is not a copy, right?

No it is a copy. In a previous comment the purpose of this makeRef() is to protect the Uint8ClampedArray till encodeImage() is called at the end of this function.

>> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:451
>> +        image = adoptCF(CGImageCreate(pixelArrayDimensions.width(), pixelArrayDimensions.height(), 8, 32, 4 * pixelArrayDimensions.width(), sRGBColorSpaceRef(), kCGBitmapByteOrderDefault | kCGImageAlphaNoneSkipLast, dataProvider.get(), 0, false, kCGRenderingIntentDefault));
> 
> This seems like a behavior change, because the returned CGImage used to be sized based on logical size, but now it's sized based on internal size.

I undid this part.

But it is not actually a behavior change. This function is called only from toDataURL() and toData() which are called only from the Canvas code. And the Canvas code creates its ImageBuffer with resolutionScale = 1. This means m_logicalSize == m_size == m_data.backingStoreSize == imageData->size(). The resolutionScale may not be 1 only in the filter code which will never call ImageBuffer::toCFData().

>> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:133
>> +    if (!result)
> 
> We use "result" as the value returned by the function, but that's not the case here.

Fixed.

>> Source/WebCore/platform/graphics/filters/FEDropShadow.cpp:102
>> +    IntRect shadowArea(IntPoint(), resultImage->logicalSize());
> 
> This seems like a behavior change too. Did you test SVG blur filters on Retina?

This is the only place in the filters code that uses BackingStoreCoordinateSystem. FEColorMatrix::platformApplySoftware() for example uses logical coordinates.  So I think this code was unintentionally using backing store coordinates although logical coordinates could just work.

>> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:522
>> +    return m_unmultipliedImageResult->data();
> 
> I don't like that we pass raw pointers to these buffers around, with undetermined lifetimes.

I checked the code and I found these functions are protected and are used internally by the derived classes. The returned pointers are never stored, they are just used for processing the data.

[Said Abou-Hallawa]

Created attachment 388890 [details]
Patch

[Said Abou-Hallawa]

Created attachment 388931 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/59438954>

[Tim Horton]

(In reply to Said Abou-Hallawa from comment #11)
> Comment on attachment 388566 [details]
> Patch
> >> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:522
> >> +    return m_unmultipliedImageResult->data();
> > 
> > I don't like that we pass raw pointers to these buffers around, with undetermined lifetimes.
> 
> I checked the code and I found these functions are protected and are used
> internally by the derived classes. The returned pointers are never stored,
> they are just used for processing the data.

It's still not a *great* pattern.

[Said Abou-Hallawa]

Created attachment 390978 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 390978 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageBuffer.h:118
> +    void draw(GraphicsContext&, const FloatRect& destRect, const FloatRect& srcRect = FloatRect(0, 0, -1, -1), const ImagePaintingOptions& = { });

What is the magic 0, 0, -1, -1? Can we pass on Optional<FloatRect> instead?

Not new in this patch; can fix later

> Source/WebCore/platform/graphics/ImageBuffer.h:125
> +    static void drawConsuming(std::unique_ptr<ImageBuffer>, GraphicsContext&, const FloatRect& destRect, const FloatRect& srcRect = FloatRect(0, 0, -1, -1), const ImagePaintingOptions& = { });

Ditto.

[WebKit Commit Bot]

Comment on attachment 390978 [details]
Patch

Clearing flags on attachment: 390978

Committed r256822: <https://trac.webkit.org/changeset/256822>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.