When Mask is called on a D2D surface with a rectangular clip and a solid color alpha surface, we should optimize it by using a FillRect call on the clip rectangle with a certain opacity.

This is an initial patch to do this. It can perhaps be improved a little.

Updated to apply to current m-c. I think this is the best way to do it. Performance differences when using D3D10, testing with IE Testdrive's SpeedReading: without: 20 seconds, average draw duration 22ms with: 14 seconds, average draw duration 16ms

Updated the other codepaths in _mask to also use this optimization to avoid the needless clip, and additionally tried to do the type conversions more sensibly.

With the patch in this bug, plus bug 638241, bug 639689 and bug 622072, I get 6s for my FF4 build and 7s for IE9 RC1.

Roc found a bug in this patch, see bug 642721.

Can this bug now be fixed with the info in bug 642721?

I'll roll that fix into this patch. But this patch still needs review.

I'm a little confused about why this patch makes such a difference? Isn't the only difference avoiding a PushAxisAlignedClip()?

Updated the patch to fix the bug roc found.

(In reply to comment #9) > Created attachment 525011 [details] [diff] [review] > Optimize rectangular clip in mask v4 > > Updated the patch to fix the bug roc found. This still lacks the comment about why it's faster.

Comment on attachment 525011 [details] [diff] [review] Optimize rectangular clip in mask v4 >@@ -3164,45 +3164,68 @@ _cairo_d2d_mask(void *surface, >+ // This function assumes atleast a single box resides at 'boxes' and the >+ // amount of boxes that reside there are passed in under num_boxes. >+ status = _cairo_clip_get_boxes(clip, &boxes, &num_boxes); >+ >+ if (!status && num_boxes == 1) { >+ box.p1.x = MAX(box.p1.x, boxes->p1.x); >+ box.p2.x = MIN(box.p2.x, boxes->p2.x); >+ box.p1.y = MAX(box.p1.y, boxes->p1.y); >+ box.p2.y = MIN(box.p2.y, boxes->p2.y); >+ >+ if (!clip || clip->path != d2dsurf->clip.path) { clip is already guaranteed to be non-null. Also, I don't understand the purpose behind the clip->path != d2dsurf->clip.path test, please add a comment. >@@ -3223,23 +3246,24 @@ _cairo_d2d_mask(void *surface, > #endif > target_rt = _cairo_d2d_get_temp_rt(d2dsurf, clip); > if (!target_rt) { > return CAIRO_INT_STATUS_UNSUPPORTED; > } > #ifndef ALWAYS_MANUAL_COMPOSITE > } else { > _begin_draw_state(d2dsurf); >- status = (cairo_int_status_t)_cairo_d2d_set_clip (d2dsurf, clip); >- >- if (unlikely(status)) >- return status; > } It seems like this code could be simpler if we just left the above hunk around or moved it up and just set 'clip' to null above instead of manually calling reset_clip everywhere.

Removed the needless check and added a comment. This comment also explains why we don't do what you mentioned earlier. In the optimal case we don't actually reset the clip when the correct clip is set. Because otherwise subsequent drawing calls might just set it again when we could just have left it there.

Added a comment about why it's faster as requested.

Updated as per discussion.