Bug 226917 – AcceleratedImageBuffer not instantiated but objects are punned to the type

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

[Kimmo Kinnunen]

AcceleratedImageBuffer is never instantiated and not instantiable.
Objects are being punned as AcceleratedImageBuffer even though it's not possible to be correct.

The AcceleratedImageBuffer::create is not returning what the callers think it is returning.

[Kimmo Kinnunen]

Created attachment 431188 [details]
Patch

[Kimmo Kinnunen]

Created attachment 431189 [details]
Patch

[Kimmo Kinnunen]

Created attachment 431190 [details]
Patch

[Kimmo Kinnunen]

Created attachment 431191 [details]
Patch

[Kimmo Kinnunen]

Created attachment 431192 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 431192 [details]
Patch

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

> Source/WebCore/platform/graphics/PlatformImageBuffer.h:45
> +namespace Detail {

I am not sure I agree with this change especially there are similar definitions in PlatformImageBufferShareableBackend.h. Can we move this change to a operate patch since it is unrelated to what you are trying to fix?

> Source/WebCore/platform/graphics/PlatformImageBuffer.h:82
> +    IOSurfaceImageBuffer(const WebCore::ImageBufferBackend::Parameters& parameters, std::unique_ptr<ImageBufferIOSurfaceBackend>&& backend)
> +        : Base(parameters, WTFMove(backend))
> +    {
> +    }

This constructor can be replaced by:

    using Base::Base;

> Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.h:40
> +    WEBCORE_EXPORT static IntSize calculateSafeBackendSize(const Parameters&);
> +    WEBCORE_EXPORT static size_t calculateMemoryCost(const Parameters&);

Why do we need to export these functions? I guess this is because the new api test calls ConcreteImageBuffer::create()?

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:280
> +    auto outputImageBuffer = IOSurfaceImageBuffer::create(clampedSize, lastEffect.filter().filterScale(), lastEffect.resultColorSpace(), PixelFormat::BGRA8);

What is the point of using this local variable and then moving it to the member m_outputImageBuffer?

Is this because m_outputImageBuffer defined to be RefPtr<ImageBuffer> and you want to avoid casting?
     RefPtr<ImageBuffer> m_outputImageBuffer;

I think it is better to change it to be RefPtr<AcceleratedImageBuffer> or even better RefPtr<IOSurfaceImageBuffer>.

[Kimmo Kinnunen]

Created attachment 431306 [details]
Patch

[Kimmo Kinnunen]

Created attachment 431307 [details]
Patch for landing

[Kimmo Kinnunen]

(In reply to Said Abou-Hallawa from comment #6)
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:280
> > +    auto outputImageBuffer = IOSurfaceImageBuffer::create(clampedSize, lastEffect.filter().filterScale(), lastEffect.resultColorSpace(), PixelFormat::BGRA8);
> 
> What is the point of using this local variable and then moving it to the
> member m_outputImageBuffer?

1) The principle of members should not be assigned unless the function succeeds

2) The principle of big allocations should not be done before clearing up old allocations, in cases where the allocating function destroys the existing state in success and error case

Moved these to bug 226962

[EWS]

Committed r278832 (238783@main): <https://commits.webkit.org/238783@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431307 [details].

[Radar WebKit Bug Importer]

<rdar://problem/79295676>