Closed Bug 589395 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mattwoodrow, Assigned: jrmuizel)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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.
Attached patch Possible fixSplinter Review
Attachment #468009 - Flags: feedback?(bas.schouten)
Blocks: d3d9-layers
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)
Attachment #468009 - Flags: feedback?(bas.schouten) → feedback-
Blocks: 586459
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?
Attached patch Actual fix.Splinter Review
Disregard that.

Proper fix that works :)
Attachment #468276 - Flags: review?(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?
(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.
Assignee: nobody → jmuizelaar
Attachment #470958 - Flags: feedback?(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.
(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.
Attachment #470958 - Attachment is obsolete: true
Attachment #471177 - Flags: review?(bas.schouten)
Attachment #470958 - Flags: feedback?(bas.schouten)
Comment on attachment 471177 [details] [diff] [review]
A tested version of the patch

This looks like exactly what we need!
Attachment #471177 - Flags: review?(bas.schouten) → review+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #468276 - Flags: review?(bas.schouten)
You need to log in before you can comment on or make changes to this bug.