580765 - Support different porter duff operators with D2D

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

With D2D we should really have proper support for the ADD operator and others. We have a good idea on how to approach this by rendering to a temporary surface and then blending back using D3D10 to use the operator we'd like to.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Manual composition support v0.1

I've attached the majority of the code needed to do this. The patch currently attached is not done though:

- Different BlendStates should be used to apply different operators (right now it still just does OVER, just in a more complicated way)
- Support other operations than just fill
- We may want to refactor all the static globals to go in a better place.
- The final composition operation should just be bound to the extents of the mask of the operation that was executed. Rather than to the full surface, to conserve fill-rate

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Manual composition support v0.2

Another iteration, not finished yet.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 459454 [details] [diff] [review]
Manual composition support v0.2


>     static ID3D10Device1 *mDeviceInstance;
>+    static ID3D10Effect *mSampleEffect;
>+    static ID3D10InputLayout *mInputLayout;
>+    static ID3D10Buffer *mQuadBuffer;
>+    static ID3D10RasterizerState *mRasterizerState;
>+    static ID3D10BlendState *mBlendStates[MAX_OPERATORS];

This seems like stuff that should hang off a cairo_d2d_device_t

Comment 4

[Bas Schouten (:bas.schouten)]

Attachment: Add custom operator support and cairo_d2d_device_t v1

Comment 5

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #4)
> Created attachment 463063 [details] [diff] [review]
> Add custom operator support and cairo_d2d_device_t v1

As you know, it would've been nice if these were separated out.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 463063 [details] [diff] [review]
Add custom operator support and cairo_d2d_device_t v1

>diff --git a/gfx/cairo/cairo/src/cairo-d2d-private.fx b/gfx/cairo/cairo/src/cairo-d2d-private.fx
>new file mode 100644
>--- /dev/null
>+++ b/gfx/cairo/cairo/src/cairo-d2d-private.fx
>@@ -0,0 +1,46 @@
>+cbuffer cb0
>+{
>+    float4 QuadDesc;
>+    float4 TexCoords;
>+}

Storing the vertex and texcoords ina constant buffer is a little unconventional.
How about adding a comment about why you chose this approach.

>diff --git a/gfx/cairo/cairo/src/cairo-d2d-private.h b/gfx/cairo/cairo/src/cairo-d2d-private.h
>--- a/gfx/cairo/cairo/src/cairo-d2d-private.h
>+++ b/gfx/cairo/cairo/src/cairo-d2d-private.h
>@@ -44,28 +44,49 @@
>+    RefPtr<ID3D10RasterizerState> mRasterizerState;
>+    RefPtr<ID3D10BlendState> mBlendStates[MAX_OPERATORS];
>+};
>+typedef struct _cairo_d2d_device cairo_d2d_device_t;
>+
> struct _cairo_d2d_surface {
>     _cairo_d2d_surface() : d2d_clip(NULL), clipping(false), isDrawing(false),
> 	textRenderingInit(true)
>     {
> 	_cairo_clip_init (&this->clip);
>     }
> 
>     cairo_surface_t base;
>+    /** Device used by this surface */
>+    cairo_d2d_device_t *device;

In upstream cairo the device is in surface base class. It's probably worth making a note of that here.

>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
>@@ -42,20 +42,310 @@
> extern "C" {
> #include "cairo-win32.h"
> #include "cairo-analysis-surface-private.h"
> }
> 
> // Required for using placement new.
> #include <new>
> 
>-ID2D1Factory *D2DSurfFactory::mFactoryInstance = NULL;
>-ID3D10Device1 *D3D10Factory::mDeviceInstance = NULL;
>+#define CAIRO_INT_STATUS_SUCCESS (cairo_int_status_t)CAIRO_STATUS_SUCCESS
> 
>-#define CAIRO_INT_STATUS_SUCCESS (cairo_int_status_t)CAIRO_STATUS_SUCCESS
>+struct Vertex
>+{
>+    float position[2];
>+};
>+
>+/**
>+ * Set a blending mode for an operator. This will also return a boolean that
>+ * reports if for this blend mode the entire surface needs to be blended. This
>+ * is true whenever the DEST blend is not ONE when src alpha is 0.
>+ */
>+static cairo_int_status_t
>+_cairo_d2d_set_operator(cairo_d2d_device_t *device,
>+			cairo_operator_t op,
>+			bool *require_full_blend)
>+{
>+    if (op > MAX_OPERATORS) {

Perhaps assert()?

>+	// Eep! Someone forgot to update MAX_OPERATORS probably.
>+	return CAIRO_INT_STATUS_UNSUPPORTED;
>+    }
>+
>+    if (device->mBlendStates[op]) {
>+	device->mD3D10Device->OMSetBlendState(device->mBlendStates[op], NULL, 0xffffffff);
>+	return CAIRO_INT_STATUS_SUCCESS;
>+    }
>+
>+    *require_full_blend = false;

Seems like the caller can just call _cairo_operator_bounded_by_mask (op) or something similar
instead of using the require_full_blend parameter.

>+RefPtr<ID2D1RenderTarget> _cairo_d2d_get_temp_rt(cairo_d2d_surface_t *surf, cairo_clip_t *clip)
>+{
>+    if (clip) {
>+	cairo_clip_path_t *path = clip->path;
>+	while (path) {

Add a comment about the differences between this code and the existing code for pushing a clip.

>+cairo_int_status_t _cairo_d2d_blend_temp_surface(cairo_d2d_surface_t *surf, cairo_operator_t op, ID2D1RenderTarget *rt, cairo_clip_t *clip, const cairo_rectangle_int_t *bounds)
>+{
>+    int numPaths = 0;
>+    cairo_clip_path_t *paths[128];

Where does 128 come from?

>+    ID3D10EffectVectorVariable *texCoords = effect->GetVariableByName("TexCoords")->AsVector();
>+
>+    float quadDescVal[] = { -1.0f, 1.0f, 2.0f, -2.0f };

What is 2.0?

Overall the drawing method could use some higher level documentation about how it works. The .fx file might be a good place to put it.

> static cairo_int_status_t
> _cairo_d2d_mask(void			*surface,
> 		cairo_operator_t	 op,
>@@ -2460,63 +2952,79 @@ _cairo_d2d_stroke(void			*surface,
>     RefPtr<ID2D1TransformedGeometry> trans_geom;
>-    D2DSurfFactory::Instance()->CreateTransformedGeometry(d2dpath, &inverse_mat, &trans_geom);
>+    sD2DFactory->CreateTransformedGeometry(d2dpath, &inverse_mat, &trans_geom);
> 
>-    d2dsurf->rt->SetTransform(mat);
>+    target_rt->SetTransform(mat);
> 
>     RefPtr<ID2D1Brush> brush = _cairo_d2d_create_brush_for_pattern(d2dsurf,
> 								   source);
>     if (!brush) {
> 	return CAIRO_INT_STATUS_UNSUPPORTED;
>     }
> 
>-    d2dsurf->rt->DrawGeometry(trans_geom, brush, (FLOAT)style->line_width, strokeStyle);
>+    target_rt->DrawGeometry(trans_geom, brush, (FLOAT)style->line_width, strokeStyle);
> 
>-    d2dsurf->rt->SetTransform(D2D1::Matrix3x2F::Identity());
>+    target_rt->SetTransform(D2D1::Matrix3x2F::Identity());

add a new line here

>+    if (target_rt.get() != d2dsurf->rt.get()) {
>+	D2D1_RECT_F bounds;
>+	trans_geom->GetWidenedBounds((FLOAT)style->line_width, strokeStyle, D2D1::IdentityMatrix(), &bounds);
>+	cairo_rectangle_int_t bound_rect;
>+	bound_rect.x = (int)floor(bounds.left);
>+	bound_rect.y = (int)floor(bounds.top);
>+	bound_rect.width = (int)ceil(bounds.right) - bound_rect.x;
>+	bound_rect.height = (int)ceil(bounds.bottom) - bound_rect.y;

seems like you could use a function that constructed cairo_rectangle_int_t's from floating point values.

>+	return _cairo_d2d_blend_temp_surface(d2dsurf, op, target_rt, clip, &bound_rect);
>+    }
>+
>@@ -2807,16 +3331,23 @@ cairo_d2d_surface_create(cairo_format_t 
>     sizePixels.height = height;
> 
>+    if (!sizePixels.width) {
>+	sizePixels.width = 1;
>+    }
>+    if (!sizePixels.height) {
>+	sizePixels.height = 1;
>+    }
>+

What's the relation of this change?

>     CD3D10_TEXTURE2D_DESC desc(
> 	dxgiformat,
> 	sizePixels.width,
> 	sizePixels.height
> 	);
>     desc.MipLevels = 1;
>     desc.Usage = D3D10_USAGE_DEFAULT;
>     desc.BindFlags = D3D10_BIND_RENDER_TARGET | D3D10_BIND_SHADER_RESOURCE;


> cairo_bool_t
> cairo_d2d_has_support()
> {
>-    /**
>-     * FIXME: We should be able to fix this in the near future when we pass in
>-     * a cairo_device_t to our surface creation functions.
>-     */
>-    if (!D3D10Factory::Device() || !D2DSurfFactory::Instance()) {
>-	return false;
>-    }
>     return true;
> }

This seems broken. :)

>diff --git a/gfx/cairo/cairo/src/cairo-dwrite-font.cpp b/gfx/cairo/cairo/src/cairo-dwrite-font.cpp
>--- a/gfx/cairo/cairo/src/cairo-dwrite-font.cpp
>+++ b/gfx/cairo/cairo/src/cairo-dwrite-font.cpp
>@@ -1258,41 +1258,97 @@ _cairo_dwrite_show_glyphs_on_d2d_surface
>     cairo_d2d_surface_t *dst = reinterpret_cast<cairo_d2d_surface_t*>(surface);
> 
>     /* We can only handle dwrite fonts */
>     //XXX: this is checked by at least one caller
>     if (cairo_scaled_font_get_type (scaled_font) != CAIRO_FONT_TYPE_DWRITE)
> 	return CAIRO_INT_STATUS_UNSUPPORTED;
> 
> 
>-    /* We can only handle operator SOURCE or OVER with the destination
>+    /* We can only handle operator SOURCE, OVER or ADD with the destination
>      * having no alpha */

Why only ADD?

>-    if (op != CAIRO_OPERATOR_SOURCE && op != CAIRO_OPERATOR_OVER)
>-	return CAIRO_INT_STATUS_UNSUPPORTED;
>+    if (op == CAIRO_OPERATOR_SOURCE) {
>+	return CAIRO_INT_STATUS_UNSUPPORTED;
>+    }
> 
>-    _cairo_d2d_begin_draw_state (dst);
>-    _cairo_d2d_set_clip (dst, clip);
>+    RefPtr<ID2D1RenderTarget> target_rt = dst->rt;
>+    cairo_rectangle_int_t fontArea;
>+#ifndef ALWAYS_MANUAL_COMPOSITE
>+    if (op != CAIRO_OPERATOR_OVER) {
>+#endif
>+	target_rt = _cairo_d2d_get_temp_rt(dst, clip);
>+
>+	if (!target_rt) {
>+	    return CAIRO_INT_STATUS_UNSUPPORTED;
>+	}
>+
>+	/* Needed to calculate bounding box for efficient blitting */

[snip]

Why don't we use CreateGlyphRunAnalysis for computing this area?

>+
>+#ifndef ALWAYS_MANUAL_COMPOSITE
>+    } else {
>+	_cairo_d2d_begin_draw_state(dst);
>+	status = (cairo_int_status_t)_cairo_d2d_set_clip (dst, clip);
>+
>+	if (unlikely(status))
>+	    return status;
>+    }
>+#endif
> 
>     D2D1_TEXT_ANTIALIAS_MODE cleartype = D2D1_TEXT_ANTIALIAS_MODE_CLEARTYPE;
> 
>-    if (dst->base.content != CAIRO_CONTENT_COLOR) {
>+    if (dst->base.content != CAIRO_CONTENT_COLOR || dst->rt.get() != target_rt.get()) {

probably worth adding a comment about the new condition. It might also be worth giving this condition a name.
i.e. store it in a variable with a more meaningful name.

>@@ -1344,17 +1400,17 @@ _cairo_dwrite_show_glyphs_on_d2d_surface
>diff --git a/gfx/cairo/cairo/src/cairo-win32-refptr.h b/gfx/cairo/cairo/src/cairo-win32-refptr.h
>--- a/gfx/cairo/cairo/src/cairo-win32-refptr.h
>+++ b/gfx/cairo/cairo/src/cairo-win32-refptr.h

Looks like this could be a separate patch?

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Allow dereferencing const

Comment 8

[Bas Schouten (:bas.schouten)]

Attachment: Add custom operator support and cairo_d2d_device_t v2

Updated in accordance with comments.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #7)
> Created attachment 464207 [details] [diff] [review]
> Allow dereferencing const

What's the use case for this?

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 464208 [details] [diff] [review]
Add custom operator support and cairo_d2d_device_t v2


>+++ b/gfx/cairo/cairo/src/cairo-d2d-private.fx
>@@ -0,0 +1,56 @@
>+// We store vertex coordinates and the quad shape in a constant buffer, this is
>+// easy to update and will prevent us from having to reshape the vertex buffer
>+// feeding the pipeline.

What does reshape the vertex buffer mean?

Comment 11

[Bas Schouten (:bas.schouten)]

(In reply to comment #10)
> Comment on attachment 464208 [details] [diff] [review]
> Add custom operator support and cairo_d2d_device_t v2
> 
> 
> >+++ b/gfx/cairo/cairo/src/cairo-d2d-private.fx
> >@@ -0,0 +1,56 @@
> >+// We store vertex coordinates and the quad shape in a constant buffer, this is
> >+// easy to update and will prevent us from having to reshape the vertex buffer
> >+// feeding the pipeline.
> 
> What does reshape the vertex buffer mean?

Locking it and changing the coordinates of the vertices.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #11)
> (In reply to comment #10)
> > Comment on attachment 464208 [details] [diff] [review] [details]
> > Add custom operator support and cairo_d2d_device_t v2
> > 
> > 
> > >+++ b/gfx/cairo/cairo/src/cairo-d2d-private.fx
> > >@@ -0,0 +1,56 @@
> > >+// We store vertex coordinates and the quad shape in a constant buffer, this is
> > >+// easy to update and will prevent us from having to reshape the vertex buffer
> > >+// feeding the pipeline.
> > 
> > What does reshape the vertex buffer mean?
> 
> Locking it and changing the coordinates of the vertices.

I wouldn't expect this to be any more expensive than updating a constant buffer. Do you have reason to believe that it is?

Comment 13

[Bas Schouten (:bas.schouten)]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Comment on attachment 464208 [details] [diff] [review] [details] [details]
> > > Add custom operator support and cairo_d2d_device_t v2
> > > 
> > > 
> > > >+++ b/gfx/cairo/cairo/src/cairo-d2d-private.fx
> > > >@@ -0,0 +1,56 @@
> > > >+// We store vertex coordinates and the quad shape in a constant buffer, this is
> > > >+// easy to update and will prevent us from having to reshape the vertex buffer
> > > >+// feeding the pipeline.
> > > 
> > > What does reshape the vertex buffer mean?
> > 
> > Locking it and changing the coordinates of the vertices.
> 
> I wouldn't expect this to be any more expensive than updating a constant
> buffer. Do you have reason to believe that it is?

Updating a constant buffer used to be more expensive, but it's been ages since I tested. Possibly allocating a constant buffer in D3DPOOL_DEFAULT with D3DUSAGE_DYNAMIC where the hardware allows this, would be just as fast. I might note this method also allows a slightly prettier API as it can be set in a single call rather than a combination of Lock, set all the bits after some casting, Unlock.

Comment 14

[Bas Schouten (:bas.schouten)]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> Updating a constant buffer used to be more expensive, but it's been ages since
> I tested.

Used to be less expensive, that is.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #13)
> (In reply to comment #12)
> Updating a constant buffer used to be more expensive, but it's been ages since
> I tested. Possibly allocating a constant buffer in D3DPOOL_DEFAULT with
> D3DUSAGE_DYNAMIC where the hardware allows this, would be just as fast. I might
> note this method also allows a slightly prettier API as it can be set in a
> single call rather than a combination of Lock, set all the bits after some
> casting, Unlock.

Perhaps, adjust the comment to focus more on the aesthetics then instead of "reshaping" because it's not clear that "reshaping" is something we want to avoid.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 464207 [details] [diff] [review]
Allow dereferencing const

As discussed this isn't needed anymore but doesn't hurt either.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 464208 [details] [diff] [review]
Add custom operator support and cairo_d2d_device_t v2

I only did a quick review this time through, but things seem sane enough.

Comment 18

[Bas Schouten (:bas.schouten)]

Pushed http://hg.mozilla.org/mozilla-central/rev/da79df639747.
Pushed http://hg.mozilla.org/mozilla-central/rev/55fd62eef09e.