Bug 82801

Summary: Support resetClip() on 2d canvas
Product: WebKit Reporter: Dean Jackson <dino>
Component: CanvasAssignee: 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 Flags
Patch
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
none
Patch buildbot: commit-queue-

Description Dean Jackson 2012-03-30 16:01:35 PDT
Canvas v5 API has a new resetClip() method.

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-resetclip
Comment 1 Radar WebKit Bug Importer 2012-03-30 16:01:48 PDT
<rdar://problem/11159364>
Comment 2 Rashmi Shyamasundar 2013-07-05 03:47:30 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.
Comment 3 Rashmi Shyamasundar 2013-07-10 02:44:20 PDT
Created attachment 206375 [details]
Patch
Comment 4 Build Bot 2013-07-10 07:31:25 PDT
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
Comment 5 Build Bot 2013-07-10 07:31:28 PDT
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
Comment 6 Rik Cabanier 2013-07-10 08:17:34 PDT
(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 7 Build Bot 2013-07-11 07:22:22 PDT
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
Comment 8 Build Bot 2013-07-11 07:22:27 PDT
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
Comment 9 Rashmi Shyamasundar 2013-07-12 00:34:12 PDT
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
}
Comment 10 Tim Horton 2013-07-12 00:47:11 PDT
(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...
Comment 11 Rashmi Shyamasundar 2013-07-12 00:51:22 PDT
Yes. This approach is expensive. Thought of this as a work around, until there is any support from Core Graphics for resetClip().
Comment 12 Rashmi Shyamasundar 2013-07-12 03:38:44 PDT
Created attachment 206521 [details]
Patch
Comment 13 Build Bot 2013-07-12 04:04:40 PDT
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 14 Build Bot 2013-07-12 04:16:22 PDT
Comment on attachment 206521 [details]
Patch

Attachment 206521 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1054265
Comment 15 Rik Cabanier 2013-07-12 10:10:05 PDT
(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
Comment 16 Rashmi Shyamasundar 2013-07-16 06:36:08 PDT
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.
Comment 17 Rik Cabanier 2013-07-16 09:20:45 PDT
(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 18 Build Bot 2013-07-17 16:14:17 PDT
Comment on attachment 206521 [details]
Patch

Attachment 206521 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1095300
Comment 19 Rashmi Shyamasundar 2013-07-22 04:24:43 PDT
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 .
Comment 20 Rik Cabanier 2013-07-22 22:08:04 PDT
(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?
Comment 21 Rashmi Shyamasundar 2013-07-22 23:03:11 PDT
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.
Comment 22 Rashmi Shyamasundar 2013-08-11 23:14:11 PDT
Can the cairo-implementation of the API resetClip(), be committed to WebKit? 
The corresponding CG implementation might take time.
Comment 23 Dean Jackson 2013-08-12 12:49:01 PDT
(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.
Comment 24 Simon Fraser (smfr) 2013-10-08 16:55:32 PDT
I don't think WebKit should support resetClip().
Comment 25 Rashmi Shyamasundar 2013-10-29 05:12:48 PDT
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.
Comment 26 Ian 'Hixie' Hickson 2013-11-06 19:40:49 PST
Wouldn't that apply to resetTransform() also?
Comment 27 Dirk Schulze 2013-11-06 21:35:24 PST
(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.
Comment 28 Dean Jackson 2014-09-12 16:48:37 PDT
We're not going to do this for the reasons stated above.