Closed Bug 600760 Opened 11 years ago Closed 11 years ago

[D2D] Optimize mask with a rectangular clip

Categories

(Core :: Graphics, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

Details

Attachments

(1 file, 6 obsolete files)

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
Attachment #479687 - Attachment is obsolete: true
Attachment #482626 - Flags: review?(jmuizelaar)
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.
Attachment #482626 - Attachment is obsolete: true
Attachment #482657 - Flags: review?(jmuizelaar)
Attachment #482626 - Flags: review?(jmuizelaar)
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.
Depends on: 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.
Attachment #482657 - Attachment is obsolete: true
Attachment #525011 - Flags: review?(jmuizelaar)
Attachment #482657 - Flags: review?(jmuizelaar)
(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.
Attachment #525011 - Flags: review?(jmuizelaar) → review-
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.
Attachment #525011 - Attachment is obsolete: true
Attachment #525045 - Flags: review?(jmuizelaar)
Added a comment about why it's faster as requested.
Attachment #525045 - Attachment is obsolete: true
Attachment #525046 - Flags: review?(jmuizelaar)
Attachment #525045 - Flags: review?(jmuizelaar)
Updated as per discussion.
Attachment #525046 - Attachment is obsolete: true
Attachment #525160 - Flags: review?(jmuizelaar)
Attachment #525046 - Flags: review?(jmuizelaar)
Attachment #525160 - Flags: review?(jmuizelaar) → review+
http://hg.mozilla.org/mozilla-central/rev/e29f195869ad
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 657141
Depends on: 718453
You need to log in before you can comment on or make changes to this bug.