589395 - [D3D9] layout/reftests/bugs/488692-1.html test failure

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

This draws 1 pixel to the upper left of the reference image.

It appears to be a rounding error involving the 0.5 pixel shift in CanvasLayerD3D9.cpp (may be related to the 2x zoom factor for the test - Not sure how these work).

Adjusting the shift to be 0.4 instead fixes the test (and doesn't regress any others). Doesn't feel like a great solution though.

Open to suggestions of better ways to debug what coordinates the shader is writing to. The difference doesn't appear to visible when rendering these pages in browser.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Possible fix

Comment 2

[Bas Schouten (:bas.schouten)]

Comment on attachment 468009 [details] [diff] [review]
Possible fix

Although I'm very happy you found this to be the cause, I don't think this is really a solution! Let's figure out how the 2x zoom is affecting this. Probably your earlier comment that the 0.5 shift should be the last transformation has something to do with it. We could leave it out of the quad transform and move it into the vertex shader. You could experiment with that.

(The Vertex Shaders are recompiled by using the 'fxc' tool in the DirectX SDK, outputing shader code using /Fh, and manually inserting it into the Shader .h file)

Comment 3

[Matt Woodrow (:mattwoodrow)]

The 2x zoom factor appears to be unrelated. Removing it doesn't fix the bug, and none of the transform matrices are multiplied by two.

Looks like we're hitting the precision limit of a float when scaling the texture up to required size, removing 0.5 and then scaling back to a 0,1 viewport.

Is there any way to debug the shader output?

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Actual fix.

Disregard that.

Proper fix that works :)

Comment 5

[Bas Schouten (:bas.schouten)]

(In reply to comment #4)
> Created attachment 468276 [details] [diff] [review]
> Actual fix.
> 
> Disregard that.
> 
> Proper fix that works :)

This bug won't just affect CanvasLayers, we should try doing the proper fix, just doing -0.5 offset on x and y in the vertex shaders at the end, and removing all the -0.5s everywhere else. Is there something I'm missing why this can't work or it won't fix the problem?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #5)
> (In reply to comment #4)
> > Created attachment 468276 [details] [diff] [review] [details]
> > Actual fix.
> > 
> > Disregard that.
> > 
> > Proper fix that works :)
> 
> This bug won't just affect CanvasLayers, we should try doing the proper fix,
> just doing -0.5 offset on x and y in the vertex shaders at the end, and
> removing all the -0.5s everywhere else. Is there something I'm missing why this
> can't work or it won't fix the problem?

I think this is probably the right approach as it will let use use the same vertex positions in d3d9 and d3d10 where the -.5 isn't needed.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Attachment: Do the offset for d3d9 pixel shaders in the vertex shader.

Comment 8

[Bas Schouten (:bas.schouten)]

Comment on attachment 470958 [details] [diff] [review]
Do the offset for d3d9 pixel shaders in the vertex shader.

Looking at what the compiled to isn't it just subtracting 0.5 from the x coordinate with this patch? And not from the y? We also don't want it to subtract 0.5 from coordinates I think. We also need to make sure the -0.5 only happens in the x and y components I believe.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #8)
> Comment on attachment 470958 [details] [diff] [review]
> Do the offset for d3d9 pixel shaders in the vertex shader.
> 
> Looking at what the compiled to isn't it just subtracting 0.5 from the x
> coordinate with this patch? And not from the y? We also don't want it to
> subtract 0.5 from coordinates I think. We also need to make sure the -0.5 only
> happens in the x and y components I believe.

The patch currently subtracts -.5 from all components. You are right that it should only subtract from the xy components.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Attachment: A tested version of the patch

Comment 11

[Bas Schouten (:bas.schouten)]

Comment on attachment 471177 [details] [diff] [review]
A tested version of the patch

This looks like exactly what we need!

Comment 12

[Jeff Muizelaar [:jrmuizel]]

http://hg.mozilla.org/mozilla-central/rev/d35f97d3fe5c