Bug 82801 – Support resetClip() on 2d canvas

• Component: Canvas
• Status: RESOLVED WONTFIX

[Dean Jackson]

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

[Radar WebKit Bug Importer]

<rdar://problem/11159364>

[Rashmi Shyamasundar]

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.

[Rashmi Shyamasundar]

Created attachment 206375 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Rik Cabanier]

(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?

[Build Bot]

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

[Build Bot]

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

[Rashmi Shyamasundar]

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
}

[Tim Horton]

(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...

[Rashmi Shyamasundar]

Yes. This approach is expensive. Thought of this as a work around, until there is any support from Core Graphics for resetClip().

[Rashmi Shyamasundar]

Created attachment 206521 [details]
Patch

[Build Bot]

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

[Build Bot]

Comment on attachment 206521 [details]
Patch

Attachment 206521 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1054265

[Rik Cabanier]

(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

[Rashmi Shyamasundar]

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.

[Rik Cabanier]

(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

[Build Bot]

Comment on attachment 206521 [details]
Patch

Attachment 206521 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1095300

[Rashmi Shyamasundar]

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 .

[Rik Cabanier]

(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?

[Rashmi Shyamasundar]

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.

[Rashmi Shyamasundar]

Can the cairo-implementation of the API resetClip(), be committed to WebKit? 
The corresponding CG implementation might take time.

[Dean Jackson]

(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.

[Simon Fraser (smfr)]

I don't think WebKit should support resetClip().

[Rashmi Shyamasundar]

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.

[Ian 'Hixie' Hickson]

Wouldn't that apply to resetTransform() also?

[Dirk Schulze]

(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.

[Dean Jackson]

We're not going to do this for the reasons stated above.