Bug 61560 – Canvas performance regression with `clip`

• Component: Canvas
• Status: RESOLVED INVALID

[kangax]

Canvas' `clip` coupled with one of drawing methods (e.g. `stroke`, `strokeRect`, etc.) results in continuous, reproducible performance loss.

The test page includes the following code:

setTimeout(function animate() {
  
  ctx.beginPath();
  ctx.moveTo(10, 10);
  ctx.lineTo(150, 150);
  ctx.lineTo(220, 20);
  ctx.closePath();
  ctx.stroke();
  ctx.clip();
  
  setTimeout(animate, 10);
}, 10);

On my system (Mac OS X) browser starts consuming ~80% CPU and continuously rises up to 90% and more. The test page also includes an FPS counter; the FPS count keeps dropping as more and more invocations of `stroke` + `clip` occur.

This doesn't happen in either Firefox (4), Opera (11.11), or IE (9). 

It DOES happen in other webkit-based browsers — Chrome (13), Safari (5).

Here's a screenshot showing activity monitor (with webkit process consuming ~90% CPU) and dropping FPS counter — http://twitpic.com/52w4ya

This problem seems to be going as far back as Safari 3.0.4

[Jarred Nicholls]

This is certainly identical symptoms on Chromium and Mac (WebKit Nightly), but interestingly enough, not an issue in Qt port.  Albeit, Qt leaks memory slowly w/ clip() and Chrome/Mac do not.  I'll check into it this weekend and see what's up.

[Oliver Hunt]

Random (non-sample based, entirely hypothetical) guess: we're applying they same clip over and over again so each frame accumulates another clip region to check against.  logically we should simply be intersecting the clip, but i would have thought that that would happen internally anyway.

[kangax]

Tried Oliver's suggestion to add `context.save()` & `context.restore()` around clipping code and performance loss was gone. Using this as a temporary workaround for now, but would love to see this "fixed" without additional "wrapping".

[Jarred Nicholls]

The clipping path intersection is not being detected by CGContextClip nor CGContextEOClip it would seem.  Surrounding the CG clip w/ save/restore graphics state certainly does fix the issue.  Bug in CG?

GraphicsContextCG.cpp
@@ -1095,13 +1095,15 @@ void GraphicsContext::clip(const Path& path)
 
     // CGContextClip does nothing if the path is empty, so in this case, we
     // instead clip against a zero rect to reduce the clipping region to
     // nothing - which is the intended behavior of clip() if the path is empty.
     if (path.isEmpty())
         CGContextClipToRect(context, CGRectZero);
     else {
+        CGContextSaveGState(context);
         CGContextBeginPath(context);
         CGContextAddPath(context, path.platformPath());
         CGContextClip(context);
+        CGContextRestoreGState(context);
     }
     m_data->clip(path);
 }

[Dirk Schulze]

(In reply to comment #4)
> The clipping path intersection is not being detected by CGContextClip nor CGContextEOClip it would seem.  Surrounding the CG clip w/ save/restore graphics state certainly does fix the issue.  Bug in CG?
> 
> GraphicsContextCG.cpp
> @@ -1095,13 +1095,15 @@ void GraphicsContext::clip(const Path& path)
> 
>      // CGContextClip does nothing if the path is empty, so in this case, we
>      // instead clip against a zero rect to reduce the clipping region to
>      // nothing - which is the intended behavior of clip() if the path is empty.
>      if (path.isEmpty())
>          CGContextClipToRect(context, CGRectZero);
>      else {
> +        CGContextSaveGState(context);
>          CGContextBeginPath(context);
>          CGContextAddPath(context, path.platformPath());
>          CGContextClip(context);
> +        CGContextRestoreGState(context);
>      }
>      m_data->clip(path);
>  }

This is incorrect. The path should not be cleared after clipping.