Summary: | Adopt new CG API for canvas | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matthew Delaney <mdelaney7> | ||||||||||||
Component: | Canvas | Assignee: | 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
Matthew Delaney
2010-12-06 14:42:31 PST
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> |