Adopt new CG API for <canvas>
Created attachment 76131 [details] Patch
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.
Attachment 76131 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6919033
Attachment 76131 [details] did not build on win: Build output: http://queues.webkit.org/results/6935014
Created attachment 76243 [details] Patch
Attachment 76243 [details] did not build on win: Build output: http://queues.webkit.org/results/7021021
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 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.
Created attachment 76283 [details] Patch
Attachment 76283 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6938059
Created attachment 76354 [details] Patch
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.
Committed r73925: <http://trac.webkit.org/changeset/73925>
http://trac.webkit.org/changeset/73925 might have broken Leopard Intel Release (Build) and Leopard Intel Debug (Build)
Created attachment 76419 [details] Patch
New patch + http://trac.webkit.org/changeset/73933 should address all comments and fix all builds.
> 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
(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.
(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.
Committed r73949: <http://trac.webkit.org/changeset/73949>