Bug 50591 – Adopt new CG API for canvas

• Component: Canvas
• Status: RESOLVED FIXED

[Matthew Delaney]

Adopt new CG API for <canvas>

[Matthew Delaney]

Created attachment 76131 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 76131 [details]
Patch

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

r=me but please address command and/or file followup bugs.

> WebCore/html/HTMLCanvasElement.cpp:374
> +    m_imageBuffer = ImageBuffer::create(size, ColorSpaceDeviceRGB, true);

We try to avoid boolean params that are hard to understand at the call site. It's difficult to know what "true" means here. Enums are preferred.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:56
> +RetainPtr<IOSurfaceRef> createIOSurface(unsigned width, unsigned height)

Use IntSize.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:62
> +#if TARGET_RT_LITTLE_ENDIAN
> +    unsigned pixelFormat = 'BGRA';
> +#else
> +    unsigned pixelFormat = 32;
> +#endif

These seem like magic.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:65
> +    unsigned long bytesPerRow = IOSurfaceAlignProperty(kIOSurfaceBytesPerRow, width*bytesPerElement);
> +    unsigned long allocSize = IOSurfaceAlignProperty(kIOSurfaceAllocSize, height*bytesPerRow);

Are these prone to overflow? What happens when someone tries to make a huge canvas?

Spaces around the * please.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:90
> +    RetainPtr<IOSurfaceRef> surface(AdoptCF, IOSurfaceCreate(dict.get()));
> +    return surface;

You can just
return RetainPtr<IOSurfaceRef> surface(AdoptCF....)

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:146
> +#if !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)
> +    else {

Maybe ASSERT(! m_accelerateRendering) if on earlier systems. Or just force m_accelerateRendering to false.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:205
> +        RefPtr<Image> copy = copyImage();

You should add a comment about why always copying is OK.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:224
>          RefPtr<Image> copy = copyImage();

Ditto.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:305
> +                uint32_t pixel = ((uint32_t*)(srcRows + basex))[0]; // (aRGB)
> +                unsigned char alpha = (pixel & 0xff000000U) >> 24;
> +                if (multiplied == Unmultiplied && alpha) {
> +                    pixel = ((pixel & 0xff000000U)
> +                          | (((((pixel >> 16) & 255) * 255) / alpha))
> +                          | (((((pixel >> 8) & 255) * 255) / alpha) << 8)
> +                          | ((((pixel  & 255) * 255) / alpha) << 16));
> +                } else
> +                    pixel = ((pixel & 0xff000000U)
> +                          | ((pixel >> 16) & 255)
> +                          | (((pixel >> 8) & 255) << 8)
> +                          | ((pixel & 255) << 16));
> +                ((uint32_t*)(destRows + basex))[0] = pixel;
> +            }

Does this work for little- and big-endian? Do we have tests that can be used to determine this?

> WebCore/rendering/RenderLayerCompositor.cpp:307
> +#if PLATFORM(MAC) && PLATFORM(CA)
> +            if (layer->renderer()->isCanvas()) {
> +                HTMLCanvasElement* canvas = static_cast<HTMLCanvasElement*>(layer->renderer()->node());
> +                if (canvas->renderingContext() && canvas->renderingContext()->isAccelerated())
> +                    layer->backing()->graphicsLayer()->setAcceleratesDrawing(true);
> +            }
> +#endif

We still need to do this elsewhere as well.

[Eric Seidel (no email)]

Attachment 76131 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6919033

[Build Bot]

Attachment 76131 [details] did not build on win:
Build output: http://queues.webkit.org/results/6935014

[Matthew Delaney]

Created attachment 76243 [details]
Patch

[Build Bot]

Attachment 76243 [details] did not build on win:
Build output: http://queues.webkit.org/results/7021021

[Simon Fraser (smfr)]

Comment on attachment 76243 [details]
Patch

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

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:56
> +namespace {
> +
> +RetainPtr<IOSurfaceRef> createIOSurface(IntSize size)

Test

[Simon Fraser (smfr)]

Comment on attachment 76243 [details]
Patch

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

>> WebCore/platform/graphics/cg/ImageBufferCG.cpp:56
>> +RetainPtr<IOSurfaceRef> createIOSurface(IntSize size)
> 
> Test

Make this method static.

It should be #ifdeffed with the same #ifdef we use to enable the feature.

[Matthew Delaney]

Created attachment 76283 [details]
Patch

[Eric Seidel (no email)]

Attachment 76283 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6938059

[Matthew Delaney]

Created attachment 76354 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 76354 [details]
Patch

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

r=me with commenets addressed

> WebCore/html/canvas/CanvasRenderingContext2D.cpp:148
> +#if PLATFORM(MAC) && PLATFORM(CA) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)
> +    return true;

Why not #if defined(USE_IOSURFACE)?

> WebCore/platform/graphics/ImageBuffer.h:126
> +        ImageBuffer(const IntSize&, ColorSpace colorSpace, bool accelerateRendering, bool& success);

I think the constructor should use RenderingMode. create methods almost always just pass their arguments directly to the constructor.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:114
>      success = false;  // Make early return mean failure.

You should assert that accelerateRendering is Unaccelerated if USE_IOSURFACE is not defined.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:230
>          RefPtr<Image> copy = copyImage();

Either add a comment to say why copying is OK, or file a bug to investigate.

[Matthew Delaney]

Committed r73925: <http://trac.webkit.org/changeset/73925>

[WebKit Review Bot]

http://trac.webkit.org/changeset/73925 might have broken Leopard Intel Release (Build) and Leopard Intel Debug (Build)

[Matthew Delaney]

Created attachment 76419 [details]
Patch

[Matthew Delaney]

New patch + http://trac.webkit.org/changeset/73933 should address all comments and fix all builds.

[Matthew Delaney]

> Why not #if defined(USE_IOSURFACE)?
Done.

> > WebCore/platform/graphics/ImageBuffer.h:126
> > +        ImageBuffer(const IntSize&, ColorSpace colorSpace, bool accelerateRendering, bool& success);
> 
> I think the constructor should use RenderingMode. create methods almost always just pass their arguments directly to the constructor.
Done.

> 
> > WebCore/platform/graphics/cg/ImageBufferCG.cpp:114
> >      success = false;  // Make early return mean failure.
> 
> You should assert that accelerateRendering is Unaccelerated if USE_IOSURFACE is not defined.
> 
Done.

> > WebCore/platform/graphics/cg/ImageBufferCG.cpp:230
> >          RefPtr<Image> copy = copyImage();
> 
> Either add a comment to say why copying is OK, or file a bug to investigate.
rdar://problem/8762683

[Simon Fraser (smfr)]

(In reply to comment #17)

> > > WebCore/platform/graphics/ImageBuffer.h:126
> > > +        ImageBuffer(const IntSize&, ColorSpace colorSpace, bool accelerateRendering, bool& success);
> > 
> > I think the constructor should use RenderingMode. create methods almost always just pass their arguments directly to the constructor.
> Done.

This was not done.

> > 
> > > WebCore/platform/graphics/cg/ImageBufferCG.cpp:114
> > >      success = false;  // Make early return mean failure.
> > 
> > You should assert that accelerateRendering is Unaccelerated if USE_IOSURFACE is not defined.
> > 
> Done.

I don't see this assertion in the commit. I'd expect it in the ImageBuffer ctor.

[Simon Fraser (smfr)]

(In reply to comment #18)

> This was not done.

> I don't see this assertion in the commit. I'd expect it in the ImageBuffer ctor.

I see these in the new patch. Please address review comments before committing next time.

[Matthew Delaney]

Committed r73949: <http://trac.webkit.org/changeset/73949>