Bug 50591

Summary: Adopt new CG API for canvas
Product: WebKit Reporter: Matthew Delaney <mdelaney7>
Component: CanvasAssignee: Matthew Delaney <mdelaney7>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, buildbot, charles-webkit, dglazkov, eric, hyatt, mdelaney7, oliver, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.6   
Bug Depends on: 50717    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch simon.fraser: review+

Description Matthew Delaney 2010-12-06 14:42:31 PST
Adopt new CG API for <canvas>
Comment 1 Matthew Delaney 2010-12-09 16:11:02 PST
Created attachment 76131 [details]
Patch
Comment 2 Simon Fraser (smfr) 2010-12-09 16:25:50 PST
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.
Comment 3 Eric Seidel (no email) 2010-12-09 19:00:31 PST
Attachment 76131 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6919033
Comment 4 Build Bot 2010-12-09 19:06:14 PST
Attachment 76131 [details] did not build on win:
Build output: http://queues.webkit.org/results/6935014
Comment 5 Matthew Delaney 2010-12-10 12:59:32 PST
Created attachment 76243 [details]
Patch
Comment 6 Build Bot 2010-12-10 13:35:55 PST
Attachment 76243 [details] did not build on win:
Build output: http://queues.webkit.org/results/7021021
Comment 7 Simon Fraser (smfr) 2010-12-10 14:28:11 PST
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
Comment 8 Simon Fraser (smfr) 2010-12-10 14:28:55 PST
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.
Comment 9 Matthew Delaney 2010-12-10 17:14:57 PST
Created attachment 76283 [details]
Patch
Comment 10 Eric Seidel (no email) 2010-12-11 03:50:30 PST
Attachment 76283 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6938059
Comment 11 Matthew Delaney 2010-12-12 23:26:50 PST
Created attachment 76354 [details]
Patch
Comment 12 Simon Fraser (smfr) 2010-12-13 10:09:29 PST
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.
Comment 13 Matthew Delaney 2010-12-13 10:14:43 PST
Committed r73925: <http://trac.webkit.org/changeset/73925>
Comment 14 WebKit Review Bot 2010-12-13 10:23:52 PST
http://trac.webkit.org/changeset/73925 might have broken Leopard Intel Release (Build) and Leopard Intel Debug (Build)
Comment 15 Matthew Delaney 2010-12-13 12:12:59 PST
Created attachment 76419 [details]
Patch
Comment 16 Matthew Delaney 2010-12-13 12:13:51 PST
New patch + http://trac.webkit.org/changeset/73933 should address all comments and fix all builds.
Comment 17 Matthew Delaney 2010-12-13 12:15:48 PST
> 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
Comment 18 Simon Fraser (smfr) 2010-12-13 12:20:42 PST
(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.
Comment 19 Simon Fraser (smfr) 2010-12-13 12:22:28 PST
(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.
Comment 20 Matthew Delaney 2010-12-13 12:36:00 PST
Committed r73949: <http://trac.webkit.org/changeset/73949>