Summary: | Support resetClip() on 2d canvas | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dean Jackson <dino> | ||||||||||
Component: | Canvas | Assignee: | Dean Jackson <dino> | ||||||||||
Status: | RESOLVED WONTFIX | ||||||||||||
Severity: | Normal | CC: | buildbot, cabanier, cdumez, commit-queue, dongseong.hwang, d-r, eoconnor, esprehn+autocc, ian, junov, krit, mrobinson, rashmi.s2, rashmi.shyam, rniwa, simon.fraser, thorton, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=270890 | ||||||||||||
Attachments: |
|
Description
Dean Jackson
2012-03-30 16:01:35 PDT
I would like to take up this bug. I see that this bug is currently assigned to Mr. Dean Jackson. But, there is no update since "2012-09-11". Hope it is okay with everyone if I submit a patch for this bug. Created attachment 206375 [details]
Patch
Comment on attachment 206375 [details] Patch Attachment 206375 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1050214 New failing tests: inspector/profiler/canvas2d/canvas2d-api-changes.html fast/canvas/canvas-resetClip.html Created attachment 206389 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.3
(In reply to comment #2) > I would like to take up this bug. I see that this bug is currently assigned to Mr. Dean Jackson. But, there is no update since "2012-09-11". Hope it is okay with everyone if I submit a patch for this bug. The patch looks great. However, the reason this feature was held up is because we didn't know how it could be implemented in Core Graphics which is the backend that Safari uses. Could you take a look there? Comment on attachment 206375 [details] Patch Attachment 206375 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1010183 New failing tests: fast/canvas/canvas-resetClip.html inspector/profiler/canvas2d/canvas2d-api-changes.html Created attachment 206459 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
I tried the below approach for implementing resetClip() in case of CG. It is working successfully. Steps :- When resetClip() is called, 1. Save the imageData from canvas imageBuffer 2. Save the GraphicsContextState 3. Clear the canvas buffer. This will delete the buffer and GraphicsContextStateSaver. 4. Copy the saved imageData to a new imageBuffer 5. Set the graphicsContextState to the saved state. GraphicsContextState does not have information about any clip. Code :- void CanvasRenderingContext2D::resetClip() { GraphicsContext* c = drawingContext(); if (!c) return; if (!state().m_invertibleCTM) return; realizeSaves(); #if USE(CAIRO) c->resetClip(); #endif #if USE(CG) IntRect imageDataRect(0, 0, this->canvas()->width(), this->canvas()->height()); RefPtr<Uint8ClampedArray> m_platformSurfaceImage = this->canvas()->buffer()->getUnmultipliedImageData(imageDataRect); GraphicsContextState m_savedState = this->canvas()->drawingContext()->state(); this->canvas()->clearBuffers(); IntPoint destOffset(0, 0); IntSize sourceSize = this->canvas()->size(); IntRect sourceRect(0, 0, sourceSize.width(), sourceSize.height()); this->canvas()->buffer()->putByteArray(Unmultiplied, m_platformSurfaceImage.get(), sourceSize, sourceRect, destOffset); m_platformSurfaceImage.clear(); m_platformSurfaceImage = 0; this->canvas()->drawingContext()->setState(m_savedState); #endif #if ENABLE(DASHBOARD_SUPPORT) clearPathForDashboardBackwardCompatibilityMode(); #endif } (In reply to comment #9) > 4. Copy the saved imageData to a new imageBuffer Copying the image into a new buffer seems insanely expensive for an operation like resetClip... Yes. This approach is expensive. Thought of this as a work around, until there is any support from Core Graphics for resetClip(). Created attachment 206521 [details]
Patch
Comment on attachment 206521 [details] Patch Attachment 206521 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/947677 Comment on attachment 206521 [details] Patch Attachment 206521 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1054265 (In reply to comment #11) > Yes. This approach is expensive. Thought of this as a work around, until there is any support from Core Graphics for resetClip(). Reading the code, it's unclear how the clip is removed. Can you explain what is happening? Will this work: ctx.clip(); ctx.save(); ctx.resetClip(); ...; // draw with no clip ctx.restore(); ...; // draw with a clip Nope. restore() does not work for a save() that is called before the call to resetClip(). resetClip() creates a new context (graphicsContext and platformContext) which does not have any of the old saved states. (In reply to comment #16) > Nope. restore() does not work for a save() that is called before the call to resetClip(). > > resetClip() creates a new context (graphicsContext and platformContext) which does not have any of the old saved states. That's wrong. From the spec: The resetClip() method must create a new clipping region that is the rectangle with the top left corner at (0,0) and the width and height of the coordinate space. The new clipping region replaces the current clipping region. and: Each CanvasRenderingContext2D rendering context maintains a stack of drawing states. Drawing states consist of: ... The current clipping region. So, my example should work Comment on attachment 206521 [details] Patch Attachment 206521 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1095300 The current definition of "struct State" in CanvasRenderingContext2D, does not contain any clipping region. It should probably be added, right? Please refer http://trac.webkit.org/browser/trunk/Source/WebCore/html/canvas/CanvasRenderingContext2D.h . (In reply to comment #19) > The current definition of "struct State" in CanvasRenderingContext2D, does not contain any clipping region. It should probably be added, right? Please refer http://trac.webkit.org/browser/trunk/Source/WebCore/html/canvas/CanvasRenderingContext2D.h . It's hard to believe that the clip state is not tracked by save/restore. Can you prove this with a test case? Consider the below scenario - ctx.save(); ctx.clip(); ...; // draw with clip ctx.restore(); ...; // draw without a clip Any scenario like the above will work because when "ctx->save()" or "ctx->restore" is called, it will call save() or restore() on the GraphicsContext which in turn will call "restorePlatformState()" which in turn calls CGContextRestoreGState() or cairo_restore() for CG and Cairo respectively. In CG/Cairo, clipping-region is part of the context state. In other words, the platformContext-state consists of the clipping region unlike GraphicsContextState and CanvasrenderingContext2D-state. Can the cairo-implementation of the API resetClip(), be committed to WebKit? The corresponding CG implementation might take time. (In reply to comment #22) > Can the cairo-implementation of the API resetClip(), be committed to WebKit? > The corresponding CG implementation might take time. Yes. I don't think WebKit should support resetClip(). As discussed with Mr. Simon Fraser :- The reason resetClip() is a bad API is that it breaks the ability to robustly modularize drawing code. For example, say that you have the following code: context.save(); // … set up a path to clip to context.clip(); someJSLibrary.drawSomething(); context.restore(); In the absence of resetClip(), you are guaranteed that someJSLibrary.drawSomething() can’t draw outside of the clip you specified. With resetClip(), that JS library can draw wherever the heck it wants, which may totally break your canvas drawing. If you really need resetClip() in your own drawing code, you’re probably doing it wrong. Wouldn't that apply to resetTransform() also? (In reply to comment #26) > Wouldn't that apply to resetTransform() also? Implementation wise no. As written in the spec today, it is the same as setTransform(1,0,0,1,0,0) and implementations can easily recover. We're not going to do this for the reasons stated above. |