550723 - [D2D] Canvascape repaints sky and ground improperly with D2D enabled

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Benjamin Xiao]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a3pre) Gecko/20100306 Minefield/3.7a3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a3pre) Gecko/20100306 Minefield/3.7a3pre

When traveling and rotating in the Canvascape demo, the walls leave behind a trail of clones of itself in the sky and on the ground. This only occurs with D2D enabled.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.benjoffe.com/code/demos/canvascape/
2. Press the arrow keys to turn and travel
3. Look at the sky and ground, they are filled with the trails the walls leave behind.
Actual Results:  
Sky and ground are not repainted properly when you move around. Smeared with trails left behind by the walls.

Expected Results:  
Sky and ground are repainted correctly.

Comment 1

[Benjamin Xiao]

Attachment: Screenshot of the problem

I forgot to include "Enable D2D" in the steps to reproduce. Here are the steps again.

Steps to Reproduce:
1. Enable mozilla.widget.render-mode = 6 and gfx.font_rendering.directwrite.enabled = false and restart browser.
2. Go to http://www.benjoffe.com/code/demos/canvascape/
3. Press the arrow keys to turn and travel
4. Look at the sky and ground, they are filled with trails left behind by the walls.

Comment 2

[Jim Jeffery not reading bug-mail 1/2/11]

Confirming, also the game runs much faster it seems with Directwrite and D2D disabled. 

@Ben, 
just FYI, its been noted by the devs that the pref:
gfx.font_rendering.directwrite.enabled   must be set to 'true' to enable D2D functionality.

Comment 3

[Benjamin Xiao]

Yes gfx.font_rendering.directwrite.enabled = true was what i meant to type. I had D2D disabled when I was writing this bug and I copied the value from about:config without changing it. Sorry for the misunderstanding.

Also from my experience I feel that the game is faster with D2D on rather than off. It's just that it doesn't repaint properly.

Comment 4

[u88484]

I see the same thing:

NVIDIA GeForce 7150M / nForce 630M
Drivers: 7.15.11.7967

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a3pre) Gecko/20100306 Minefield/3.7a3pre ID:20100306035513

Benjamin, what graphics card and drivers are you using?

Comment 5

[Bas Schouten (:bas.schouten)]

I suspect the reason it's slow is the same reason it's not working right. It's probably using cairo surface fallbacks, I'll need to look into which and why the fallbacks aren't working right.

Comment 6

[Bas Schouten (:bas.schouten)]

Ok, it does not seem to fall back. I do see the issue and I'll work on fixing it. It's actually faster for me than without D2D though.

Comment 7

[Benjamin Xiao]

@Kurt

Nvidia Geforce 9600M GS
Drivers: 196.21

Comment 8

[Bas Schouten (:bas.schouten)]

It appears this is a regression from bug 549652. It appears our clear method is not working correctly.

Comment 9

[Bas Schouten (:bas.schouten)]

Alright, so it appears my assumptions about how clear combines with layers were wrong. When we clear with a geometric mask pushes, that layer will be cleared, but pop layer will only composite that layer onto the background. When doing this the 'clear' will not be transferred to the background. This will need a more complex fix.

Comment 10

[Bas Schouten (:bas.schouten)]

Attachment: Fix CLEAR operator behavior

So, we really can't do a clear with purely D2D. But by using our D3D storage buffer we can copy our surface content, and then copy it back with a clipmask. This makes the complex case of clear quite a bit more expensive.

For axis aligned rectangular clips -and- axis aligned rectangular geometries we can use D2D clear directly, I've optimized these cases. It seems the non-optimized codepath is never used, so this is good.

Comment 11

[Bas Schouten (:bas.schouten)]

Attachment: Fix CLEAR operator behavior v2

Whitespace fix.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 431090 [details] [diff] [review]
Fix CLEAR operator behavior 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
>@@ -352,18 +352,20 @@ _cairo_d2d_color_from_cairo_color(const 
>  *
>  * \param surface D2D surface.
>  * \return Buffer texture
>  */
> static ID3D10Texture2D*
> _cairo_d2d_get_buffer_texture(cairo_d2d_surface_t *surface) 
> {
>     if (!surface->bufferTexture) {
>+	IDXGISurface *surf;
> 	DXGI_SURFACE_DESC surfDesc;
>-	surface->backBuf->GetDesc(&surfDesc);
>+	surface->surface->QueryInterface(&surf);
>+	surf->GetDesc(&surfDesc);
> 	CD3D10_TEXTURE2D_DESC softDesc(surfDesc.Format, surfDesc.Width, surfDesc.Height);

What's the change here for?

>         softDesc.MipLevels = 1;
> 	softDesc.Usage = D3D10_USAGE_DEFAULT;
> 	softDesc.BindFlags = D3D10_BIND_RENDER_TARGET | D3D10_BIND_SHADER_RESOURCE;
> 	D3D10Factory::Device()->CreateTexture2D(&softDesc, NULL, &surface->bufferTexture);
>     }
>     return surface->bufferTexture;
> }
>@@ -1133,34 +1135,124 @@ _cairo_d2d_create_path_geometry_for_path
> 	sink->EndFigure(D2D1_FIGURE_END_OPEN);
>     }
>     sink->Close();
>     sink->Release();
>     return d2dpath;
> }
> 
<aside>
Overall, I'm a little worried about state management in d2d backend. We seem
to getting a fair amount of state and don't have any real unified strategy for
managing it. I'm afraid that it will be easy for us to have hard to find bugs.
</aside>


> /**
>- * We use this to clear out a certain geometry on a surface. This will respect
>+ * We use this to clear out a certain path on a surface. This will respect
>  * the existing clip.
>  *
>  * \param d2dsurf Surface we clear
>- * \param geometry Geometry of the area to clear
>+ * \param geometry Geometry of the area to clear, NULL means entire surface.
>  */
> static void _cairo_d2d_clear_geometry(cairo_d2d_surface_t *d2dsurf,
>-				      ID2D1Geometry *geometry)
>+				      ID2D1Geometry *pathGeometry)
> {

Why the name change to pathGeometry?

This function isn't my favorite but it seems to do what need. Perhaps in the future we can get a setup where we can rasterize the path to a mask and then use d3d blend modes to implement the other operators.

Comment 13

[Bas Schouten (:bas.schouten)]

(In reply to comment #12)
> (From update of attachment 431090 [details] [diff] [review])
> >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
> >@@ -352,18 +352,20 @@ _cairo_d2d_color_from_cairo_color(const 
> >  *
> >  * \param surface D2D surface.
> >  * \return Buffer texture
> >  */
> > static ID3D10Texture2D*
> > _cairo_d2d_get_buffer_texture(cairo_d2d_surface_t *surface) 
> > {
> >     if (!surface->bufferTexture) {
> >+	IDXGISurface *surf;
> > 	DXGI_SURFACE_DESC surfDesc;
> >-	surface->backBuf->GetDesc(&surfDesc);
> >+	surface->surface->QueryInterface(&surf);
> >+	surf->GetDesc(&surfDesc);
> > 	CD3D10_TEXTURE2D_DESC softDesc(surfDesc.Format, surfDesc.Width, surfDesc.Height);
> 
> What's the change here for?

In the past this function was only used for window surfaces. Now it's also used for normal surfaces. Therefor this had to be changed to work off surface, which always exists, unlike backBuf.

> 
> >         softDesc.MipLevels = 1;
> > 	softDesc.Usage = D3D10_USAGE_DEFAULT;
> > 	softDesc.BindFlags = D3D10_BIND_RENDER_TARGET | D3D10_BIND_SHADER_RESOURCE;
> > 	D3D10Factory::Device()->CreateTexture2D(&softDesc, NULL, &surface->bufferTexture);
> >     }
> >     return surface->bufferTexture;
> > }
> >@@ -1133,34 +1135,124 @@ _cairo_d2d_create_path_geometry_for_path
> > 	sink->EndFigure(D2D1_FIGURE_END_OPEN);
> >     }
> >     sink->Close();
> >     sink->Release();
> >     return d2dpath;
> > }
> > 
> <aside>
> Overall, I'm a little worried about state management in d2d backend. We seem
> to getting a fair amount of state and don't have any real unified strategy for
> managing it. I'm afraid that it will be easy for us to have hard to find bugs.
> </aside>
The main state we have is drawing vs. non-drawing, and clipped vs. non-clipped. But I agree the state management could, and should be improved. Once we iron out all the bugs it could probably do with some refactoring.

> 
> 
> > /**
> >- * We use this to clear out a certain geometry on a surface. This will respect
> >+ * We use this to clear out a certain path on a surface. This will respect
> >  * the existing clip.
> >  *
> >  * \param d2dsurf Surface we clear
> >- * \param geometry Geometry of the area to clear
> >+ * \param geometry Geometry of the area to clear, NULL means entire surface.
> >  */
> > static void _cairo_d2d_clear_geometry(cairo_d2d_surface_t *d2dsurf,
> >-				      ID2D1Geometry *geometry)
> >+				      ID2D1Geometry *pathGeometry)
> > {
> 
> Why the name change to pathGeometry?

To avoid confusion with the clipMask geometry, and the eventual geometry of the area being cleared.

> 
> This function isn't my favorite but it seems to do what need. Perhaps in the
> future we can get a setup where we can rasterize the path to a mask and then
> use d3d blend modes to implement the other operators.
Yes, this is definitely a good option. This was part of the reason we have D3D textures backing everything.

Comment 14

[Bas Schouten (:bas.schouten)]

Pushed http://hg.mozilla.org/mozilla-central/rev/665c2a7bede3.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #13)
> The main state we have is drawing vs. non-drawing, and clipped vs.
> non-clipped. But I agree the state management could, and should be improved.
> Once we iron out all the bugs it could probably do with some refactoring.

We need tests for the bugs.

Comment 16

[u88484]

Verified fixed using the Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a3pre) Gecko/20100311 Minefield/3.7a3pre nightly build