Closed Bug 580109 Opened 14 years ago Closed 13 years ago

Support fast D2D surface compositing

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

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

References

Details

Attachments

(1 file, 3 obsolete files)

Optimize to GPU memcpy where available v4
10.28 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review 
Since layers landed we are doing an increased amount of compositing different D2D surfaces. These compositions, under a certain set of conditions can be achieved through simple GPU memcopies. By doing this ourselves we can probably be faster than Direct2D. Additionally this allows us to support self-copy under a certain set of condition.

I've created a patch which seems to deal with the vast amount of cases where we composite D2D surfaces and it appears to perform very well.
Attachment #458495 - Flags: review?(jmuizelaar)
fast_composite isn't a very good name.
(In reply to comment #0)
> I've created a patch which seems to deal with the vast amount of cases where we
> composite D2D surfaces and it appears to perform very well.

Do you have any numbers to quantify this appearance?
(In reply to comment #1)
> fast_composite isn't a very good name.

I agree; should it be omgwtffast_composite?
(In reply to comment #2)
> (In reply to comment #0)
> > I've created a patch which seems to deal with the vast amount of cases where we
> > composite D2D surfaces and it appears to perform very well.
> 
> Do you have any numbers to quantify this appearance?

I do not. Nor do I have a good idea on how to get those numbers. Finding a strategy to do performance measurement is on my to-do list.

ID3D10Counter might help there going forward: http://msdn.microsoft.com/en-us/library/bb173514%28v=VS.85%29.aspx

Also what's wrong with fast_composite? It's a fast way of compositing surfaces compared to the normal d2d way, aye?
Blocks: 579215
Comment on attachment 458495 [details] [diff] [review]
Optimize to GPU memcpy where available

I wonder if this type of function would be better off as an implemention of the _composite() backend function. That way you'll be able to hang of a more general version that supports more than just SOURCE.


>+    /* Region we need to clip this operation to */
>+    cairo_region_t *region = cairo_region_create_rectangle(&clip_rect);
>+    if (clip) {
>+	cairo_clip_path_t *path = clip->path;
>+	while (path) {
>+	    cairo_box_t clipBox;
>+	    if (path->flags & CAIRO_CLIP_PATH_IS_BOX) {
>+		if (!(path->flags & CAIRO_CLIP_PATH_HAS_REGION)) {
>+		    // If this does not have a region it could be none-pixel aligned.
>+		    cairo_box_t clip_box;
>+		    _cairo_path_fixed_is_box(&path->path, &clip_box);
>+		    if (!box_is_integer(&clip_box)) {
>+			// We cannot do AA.
>+			rv = CAIRO_INT_STATUS_UNSUPPORTED;
>+			goto CLEANUP;
>+		    }
>+		}
>+		cairo_region_intersect_rectangle(region, &path->extents);
>+	    } else if (path->flags & CAIRO_CLIP_PATH_HAS_REGION) {
>+		// Cairo regions are always pixel aligned rects.
>+		cairo_region_intersect(region, path->region);
>+	    } else {
>+		// We cannot support complex clipping masks.
>+		rv = CAIRO_INT_STATUS_UNSUPPORTED;
>+		goto CLEANUP;
>+	    }
>+	    path = path->prev;
>+	}
>+    }
>+

Any reason you can't use _cairo_clip_get_region() like _cairo_d2d_clear?
(In reply to comment #5)
> Comment on attachment 458495 [details] [diff] [review]
> Optimize to GPU memcpy where available
> 
> I wonder if this type of function would be better off as an implemention of the
> _composite() backend function. That way you'll be able to hang of a more
> general version that supports more than just SOURCE.

I don't think so, we'll have less fine grained control. Also, a function like this would only support SOURCE. The point of this particular function would be that it really only does memcpy. With OVER or other operators it would become more complicated and probably not much faster than D2D.

> 
> 
> >+    /* Region we need to clip this operation to */
> >+    cairo_region_t *region = cairo_region_create_rectangle(&clip_rect);
> >+    if (clip) {
> >+	cairo_clip_path_t *path = clip->path;
> >+	while (path) {
> >+	    cairo_box_t clipBox;
> >+	    if (path->flags & CAIRO_CLIP_PATH_IS_BOX) {
> >+		if (!(path->flags & CAIRO_CLIP_PATH_HAS_REGION)) {
> >+		    // If this does not have a region it could be none-pixel aligned.
> >+		    cairo_box_t clip_box;
> >+		    _cairo_path_fixed_is_box(&path->path, &clip_box);
> >+		    if (!box_is_integer(&clip_box)) {
> >+			// We cannot do AA.
> >+			rv = CAIRO_INT_STATUS_UNSUPPORTED;
> >+			goto CLEANUP;
> >+		    }
> >+		}
> >+		cairo_region_intersect_rectangle(region, &path->extents);
> >+	    } else if (path->flags & CAIRO_CLIP_PATH_HAS_REGION) {
> >+		// Cairo regions are always pixel aligned rects.
> >+		cairo_region_intersect(region, path->region);
> >+	    } else {
> >+		// We cannot support complex clipping masks.
> >+		rv = CAIRO_INT_STATUS_UNSUPPORTED;
> >+		goto CLEANUP;
> >+	    }
> >+	    path = path->prev;
> >+	}
> >+    }
> >+
> 
> Any reason you can't use _cairo_clip_get_region() like _cairo_d2d_clear?

There's not I think, I forgot about the function.
Updated to use _cairo_clip_get_region.
Attachment #458495 - Attachment is obsolete: true
Attachment #462669 - Flags: review?(jmuizelaar)
Attachment #458495 - Flags: review?(jmuizelaar)
Comment on attachment 462669 [details] [diff] [review]
Optimize to GPU memcpy where available v2

>diff --git a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
>--- a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
>+++ b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
>@@ -2359,28 +2359,157 @@ _cairo_d2d_stroke(void			*surface,
> 
>     d2dsurf->rt->DrawGeometry(trans_geom, brush, (FLOAT)style->line_width, strokeStyle);
> 
>     d2dsurf->rt->SetTransform(D2D1::Matrix3x2F::Identity());
>     return CAIRO_INT_STATUS_SUCCESS;
> }
> 
> static cairo_int_status_t
>+_cairo_d2d_fast_composite(cairo_d2d_surface_t *dst,
>+			  cairo_surface_t *src,
>+			  cairo_box_t *box,
>+			  const cairo_matrix_t *matrix,
>+			  cairo_clip_t *clip,
>+			  cairo_operator_t op)

how about we call this _cairo_d2d_try_copy_surface()
and split out the actual _cairo_d2d_copy_surface()

>+{
>+    if (op != CAIRO_OPERATOR_SOURCE &&
>+	!(op == CAIRO_OPERATOR_OVER && src->content == CAIRO_CONTENT_COLOR)) {
>+	return CAIRO_INT_STATUS_UNSUPPORTED;
>+    }
>+
>+    int tx, ty;
>+    if (!box_is_integer(box) ||
>+	!_cairo_matrix_is_integer_translation(matrix, &tx, &ty)) {
>+	return CAIRO_INT_STATUS_UNSUPPORTED;
>+    }
>+
>+    /* For now we do only D2D sources */
>+    if (src->type != CAIRO_SURFACE_TYPE_D2D) {
>+	return CAIRO_INT_STATUS_UNSUPPORTED;
>+    }
>+
>+    cairo_d2d_surface_t *d2dsrc = reinterpret_cast<cairo_d2d_surface_t*>(src);
>+
>+    D3D10_BOX rect;
>+
>+    RefPtr<IDXGISurface> dstSurface;
>+    dst->surface->QueryInterface(&dstSurface);
>+    RefPtr<IDXGISurface> srcSurface;
>+    d2dsrc->surface->QueryInterface(&srcSurface);
>+    DXGI_SURFACE_DESC srcDesc, dstDesc;
>+
>+    srcSurface->GetDesc(&srcDesc);
>+    dstSurface->GetDesc(&dstDesc);
>+
>+    cairo_rectangle_int_t clip_rect;
>+    clip_rect.x = 0;
>+    clip_rect.y = 0;
>+    clip_rect.width = dstDesc.Width;
>+    clip_rect.height = dstDesc.Height;
>+    
>+    cairo_int_status_t rv = CAIRO_INT_STATUS_SUCCESS;
>+
>+    /* Region we need to clip this operation to */
>+    cairo_region_t *region = NULL;
>+    int num_rects = 1;
>+    if (clip) {
>+	_cairo_clip_get_region(clip, &region);
>+
>+	if (!region) {
>+	    return CAIRO_INT_STATUS_UNSUPPORTED;
>+	}
>+
>+	num_rects = cairo_region_num_rectangles(region);
>+    }
>+
>+    _cairo_d2d_flush(dst);
>+    ID3D10Resource *srcResource = d2dsrc->surface;
>+    if (d2dsrc->surface.get() == dst->surface.get()) {
>+	// Self-copy
>+	srcResource = _cairo_d2d_get_buffer_texture(dst);
>+	D3D10Factory::Device()->CopyResource(srcResource, dst->surface);

Can we use src instead of dst here to make it more clear that we're making a copy of the source.
Otherwise it looks like we might be copying the source to the dst or something...

>+    } else {
>+	// Need to flush the source too if it's a different surface.
>+        _cairo_d2d_flush(d2dsrc);
>+    }
>+
>+    // One copy for each rectangle in the final clipping region.
>+    for (int i = 0; i < num_rects; i++) {
>+	// If we have no region we have one rectangle - clip_rect.
>+	if (region) {
>+	    cairo_region_get_rectangle(region, i, &clip_rect);
>+	}
>+
>+	int posx, posy, width, height;
>+	posx = MAX(_cairo_fixed_integer_part(box->p1.x), clip_rect.x);
>+	posy = MAX(_cairo_fixed_integer_part(box->p1.y), clip_rect.y);
>+	width = MIN(_cairo_fixed_integer_part(box->p2.x) - posx, (int)clip_rect.width - posx + clip_rect.x);
>+	height = MIN(_cairo_fixed_integer_part(box->p2.y) - posy, (int)clip_rect.height - posy + clip_rect.y);

This seems like a rectangle intersection. Can you use _cairo_rectangle_intersect

>+
>+	if (width <= 0 || height <= 0) {
>+	    // Nothing to be done.
>+	    continue;
>+	}
>+
>+	if (posx + tx < 0 || posy + ty < 0 ||
>+	    (posx + tx + width) > (int)srcDesc.Width || (posy + ty + height) > (int)srcDesc.Height) {

This looks a test if rectangle is contained in another. We don't have a helper for this. But it's probably worth writing one.


>+	    /* We cannot do any sort of extend, in the future a little bit of extra code could
>+	     * allow us to support EXTEND_NONE.
>+	     */
>+	    rv = CAIRO_INT_STATUS_UNSUPPORTED;
>+	    break;
>+	}
>+
>+	rect.front = 0;
>+	rect.back = 1;
>+	rect.left = posx + tx;
>+	rect.top = posy + ty;
>+	rect.right = posx + tx + width;
>+	rect.bottom = posy + ty + height;
>+
>+	D3D10Factory::Device()->CopySubresourceRegion(dst->surface,
>+						      0,
>+						      posx,
>+						      posy,
>+						      0,
>+						      srcResource,
>+						      0,
>+						      &rect);
>+    }
>+
>+    return rv;
>+}
>+
>+static cairo_int_status_t
> _cairo_d2d_fill(void			*surface,
> 		cairo_operator_t	 op,
> 		const cairo_pattern_t	*source,
> 		cairo_path_fixed_t	*path,
> 		cairo_fill_rule_t	 fill_rule,
> 		double			 tolerance,
> 		cairo_antialias_t	 antialias,
> 		cairo_clip_t		*clip)
> {
>     cairo_int_status_t status;
> 
>     cairo_d2d_surface_t *d2dsurf = static_cast<cairo_d2d_surface_t*>(surface);
>+    cairo_box_t box;
>+    bool is_box = _cairo_path_fixed_is_box(path, &box);
>+
>+    if (is_box && source->type == CAIRO_PATTERN_TYPE_SURFACE) {
>+	const cairo_surface_pattern_t *surf_pattern = 
>+	    reinterpret_cast<const cairo_surface_pattern_t*>(source);
>+	cairo_int_status_t rv = _cairo_d2d_fast_composite(d2dsurf, surf_pattern->surface,
>+							  &box, &source->matrix, clip, op);
>+
>+	if (rv != CAIRO_INT_STATUS_UNSUPPORTED) {
>+	    return rv;
>+	}
>+    }
> 
>     op = _cairo_d2d_simplify_operator(op, source);
> 
>     if (op != CAIRO_OPERATOR_OVER && op != CAIRO_OPERATOR_ADD &&
> 	op != CAIRO_OPERATOR_CLEAR) {
> 	/** 
> 	 * We don't really support ADD yet. True ADD support requires getting
> 	 * the tesselated mesh from D2D, and blending that using D3D which has
>@@ -2397,27 +2526,25 @@ _cairo_d2d_fill(void			*surface,
> 
> 
>     if (antialias == CAIRO_ANTIALIAS_NONE) {
> 	d2dsurf->rt->SetAntialiasMode(D2D1_ANTIALIAS_MODE_ALIASED);
>     } else {
> 	d2dsurf->rt->SetAntialiasMode(D2D1_ANTIALIAS_MODE_PER_PRIMITIVE);
>     }
> 
>-    cairo_box_t box;
>-
>     if (op == CAIRO_OPERATOR_CLEAR) {
> 	if (_cairo_path_fixed_is_box(path, &box)) {
> 	    return _cairo_d2d_clear_box (d2dsurf, clip, &box);
> 	} else {
> 	    return CAIRO_INT_STATUS_UNSUPPORTED;
> 	}
>     }
> 
>-    if (_cairo_path_fixed_is_box(path, &box)) {
>+    if (is_box) {
> 	float x1 = _cairo_fixed_to_float(box.p1.x);
> 	float y1 = _cairo_fixed_to_float(box.p1.y);    
> 	float x2 = _cairo_fixed_to_float(box.p2.x);    
> 	float y2 = _cairo_fixed_to_float(box.p2.y);
> 	RefPtr<ID2D1Brush> brush = _cairo_d2d_create_brush_for_pattern(d2dsurf,
> 								       source);
> 	if (!brush) {
> 	    return CAIRO_INT_STATUS_UNSUPPORTED;
Attachment #462669 - Flags: review?(jmuizelaar) → review-
(In reply to comment #8)
> Comment on attachment 462669 [details] [diff] [review]
> Optimize to GPU memcpy where available v2
> 
> >diff --git a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
> >--- a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
> >+++ b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
> >@@ -2359,28 +2359,157 @@ _cairo_d2d_stroke(void			
> > static cairo_int_status_t
> >+_cairo_d2d_fast_composite(cairo_d2d_surface_t *dst,
> >+			  cairo_surface_t *src,
> >+			  cairo_box_t *box,
> >+			  const cairo_matrix_t *matrix,
> >+			  cairo_clip_t *clip,
> >+			  cairo_operator_t op)
> 
> how about we call this _cairo_d2d_try_copy_surface()
> and split out the actual _cairo_d2d_copy_surface()

I'm not sure .. is there a lot of value to that? I doubt we'll be reusing d2d_copy_surface a lot in that case. And we'd just be passing the huge stack of parameters around.

> >+    _cairo_d2d_flush(dst);
> >+    ID3D10Resource *srcResource = d2dsrc->surface;
> >+    if (d2dsrc->surface.get() == dst->surface.get()) {
> >+	// Self-copy
> >+	srcResource = _cairo_d2d_get_buffer_texture(dst);
> >+	D3D10Factory::Device()->CopyResource(srcResource, dst->surface);
> 
> Can we use src instead of dst here to make it more clear that we're making a
> copy of the source.
> Otherwise it looks like we might be copying the source to the dst or
> something...

I could create an intermediate stack variable to make it more obvious, alternatively I can just add a comment. Which do you prefer?

> >+
> >+	if (width <= 0 || height <= 0) {
> >+	    // Nothing to be done.
> >+	    continue;
> >+	}
> >+
> >+	if (posx + tx < 0 || posy + ty < 0 ||
> >+	    (posx + tx + width) > (int)srcDesc.Width || (posy + ty + height) > (int)srcDesc.Height) {
> 
> This looks a test if rectangle is contained in another. We don't have a helper
> for this. But it's probably worth writing one.
> 

Not entirely, it tests if the rectangle is -not- contained. And the test needs to pass if the two rectangles are equal, a 'contained' function would generally return true if the rectangles are equal, so this would become a _is_contained_and_not_equal or a _is_not_contained_or_equal, both of which have little re-use value perhaps?
(In reply to comment #9)
> (In reply to comment #8)
> > Comment on attachment 462669 [details] [diff] [review] [details]
> > Optimize to GPU memcpy where available v2
> > 
> > >diff --git a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
> > >--- a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
> > >+++ b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
> > >@@ -2359,28 +2359,157 @@ _cairo_d2d_stroke(void			
> > > static cairo_int_status_t
> > >+_cairo_d2d_fast_composite(cairo_d2d_surface_t *dst,
> > >+			  cairo_surface_t *src,
> > >+			  cairo_box_t *box,
> > >+			  const cairo_matrix_t *matrix,
> > >+			  cairo_clip_t *clip,
> > >+			  cairo_operator_t op)
> > 
> > how about we call this _cairo_d2d_try_copy_surface()
> > and split out the actual _cairo_d2d_copy_surface()
> 
> I'm not sure .. is there a lot of value to that? I doubt we'll be reusing
> d2d_copy_surface a lot in that case. And we'd just be passing the huge stack of
> parameters around.
> 

It keeps the functions simple. The stack shouldn't be that huge, just dst, src, region and maybe box. (Although you may be able to just intersect the box with the region)


> > >+    _cairo_d2d_flush(dst);
> > >+    ID3D10Resource *srcResource = d2dsrc->surface;
> > >+    if (d2dsrc->surface.get() == dst->surface.get()) {
> > >+	// Self-copy
> > >+	srcResource = _cairo_d2d_get_buffer_texture(dst);
> > >+	D3D10Factory::Device()->CopyResource(srcResource, dst->surface);
> > 
> > Can we use src instead of dst here to make it more clear that we're making a
> > copy of the source.
> > Otherwise it looks like we might be copying the source to the dst or
> > something...
> 
> I could create an intermediate stack variable to make it more obvious,
> alternatively I can just add a comment. Which do you prefer?

you can't just use d2dsrc?
 
> > >+
> > >+	if (width <= 0 || height <= 0) {
> > >+	    // Nothing to be done.
> > >+	    continue;
> > >+	}
> > >+
> > >+	if (posx + tx < 0 || posy + ty < 0 ||
> > >+	    (posx + tx + width) > (int)srcDesc.Width || (posy + ty + height) > (int)srcDesc.Height) {
> > 
> > This looks a test if rectangle is contained in another. We don't have a helper
> > for this. But it's probably worth writing one.
> > 
> 
> Not entirely, it tests if the rectangle is -not- contained. And the test needs
> to pass if the two rectangles are equal, a 'contained' function would generally
> return true if the rectangles are equal, so this would become a
> _is_contained_and_not_equal or a _is_not_contained_or_equal, both of which have
> little re-use value perhaps?


I don't understand why you can't just do: 

if (!rectangle_contains(surface_rect, transformed_rect))

where rectangle_contains returns true if the rectangles are equal and false if transformed_rect is not contained by surface_rect
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Comment on attachment 462669 [details] [diff] [review] [details] [details]
> > > Optimize to GPU memcpy where available v2
> > > 
> > > >diff --git a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
> > > >--- a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
> > > >+++ b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
> > > >@@ -2359,28 +2359,157 @@ _cairo_d2d_stroke(void			
> 
> > > >+    _cairo_d2d_flush(dst);
> > > >+    ID3D10Resource *srcResource = d2dsrc->surface;
> > > >+    if (d2dsrc->surface.get() == dst->surface.get()) {
> > > >+	// Self-copy
> > > >+	srcResource = _cairo_d2d_get_buffer_texture(dst);
> > > >+	D3D10Factory::Device()->CopyResource(srcResource, dst->surface);
> > > 
> > > Can we use src instead of dst here to make it more clear that we're making a
> > > copy of the source.
> > > Otherwise it looks like we might be copying the source to the dst or
> > > something...
> > 
> > I could create an intermediate stack variable to make it more obvious,
> > alternatively I can just add a comment. Which do you prefer?
> 
> you can't just use d2dsrc?

This copies the contents of the -destination- to the intermediate texture (the new source). The first parameter is destination, second is source.

> 
> 
> I don't understand why you can't just do: 
> 
> if (!rectangle_contains(surface_rect, transformed_rect))
> 
> where rectangle_contains returns true if the rectangles are equal and false if
> transformed_rect is not contained by surface_rect

So the problem is, if transformed_rect is not contained by surface rect, we're all good and we can continue. If they're equal, we can -also- continue. Only if the transformed_rect is smaller than the surface_rect we can not continue...
Updated a bit.
Attachment #462669 - Attachment is obsolete: true
Attachment #462991 - Flags: review?(jmuizelaar)
This fixes an issue where it could try to draw a region which was partially off the destination surface.
Attachment #462991 - Attachment is obsolete: true
Attachment #463062 - Flags: review?(jmuizelaar)
Attachment #462991 - Flags: review?(jmuizelaar)
Comment on attachment 463062 [details] [diff] [review]
Optimize to GPU memcpy where available v4

>diff --git a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
>--- a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
>+++ b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp
> 
>+static cairo_int_status_t
>+_cairo_d2d_copy(cairo_d2d_surface_t *dst,
>+		cairo_d2d_surface_t *src,
>+		cairo_point_int_t *translation,
>+		cairo_region_t *region)

copy_surface()

>+{
>+    D3D10_BOX rect;
> 

Move the declaration into the loop that uses it.
Attachment #463062 - Flags: review?(jmuizelaar) → review+
This works better now, but sometimes the scrolling still jerks when the mouse pointer goes over a hover link or other item that changes in response to being moused over.
This was fixed with the push on here.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: