<div dir="ltr">On 13 May 2013 04:10, Chris Forbes <span dir="ltr"><<a href="mailto:chrisf@ijw.co.nz" target="_blank">chrisf@ijw.co.nz</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">


Replaces: "[PATCH] mesa: Fix meta smashing stencil reference value on restore"<br>
<br>
This series changes the way stencil reference values are clamped. Previously,<br>
the values would be clamped according to the bit depth of the stencil buffer<br>
bound _when the the value was specified_. This looked correct in most cases, but fell apart<br>
in some cases where the stencil buffer validated against is not the same as the buffer that<br>
would be rendered into.<br>
<br>
<br>
A simple example is this sequence, which would produce a reference value of zero,<br>
even though the user probably meant otherwise:<br>
<br>
1. BindFramebuffer( .. fbo with NO stencil )<br>
2. StencilFuncSeparate( .. 0xff .. )<br>
3. BindFramebuffer( .. fbo with stencil )<br>
<br>
<br>
A more realistic case involves a meta op restoring the stencil state, which is done by calling<br>
the StencilFuncSeparate() API just the same as the user:<br>
<br>
1. BindFramebuffer( .. fbo with stencil )<br>
2. StencilFuncSeparate( .. 0xff .. )<br>
3. BindFramebuffer( .. fbo with NO stencil .. )<br>
4. <some meta op><br>
5. BindFramebuffer( .. fbo with stencil )<br>
<br>
In this case, the stencil reference value would be revalidated against zero stencil<br>
bits at (4) and so smashed to zero. This sequence is from Portal.<br></blockquote><div><br></div><div>Two minor comments:<br></div><div><br>1. This is a subtle enough bug that it really deserves piglit testing to make sure that it doesn't regress in the future.  Would you mind writing piglit tests to validate that (a) the stencil reference value read back with a GL query is correct, and (b) the behaviour of a stencil operation is correct?  IMHO it would be sufficient to test just the first of the above two sequences of operations.<br>

<br></div><div>2. I didn't notice the spec quoted anywhere in the patch series.  It would be nice to quote it somewhere in either some of the commit messages or in the code itself (perhaps in a comment above _mesa_get_stencil_ref), so that if someone goes digging later they will see why the new behaviour is correct.  I believe the relevant spec quote is (from GL 4.3 17.3.5 "Stencil Test"): "Stencil comparison operations and queries of ref clamp its value to the range [0, 2s − 1], where s is the number of bits in the stencil buffer attached to the draw framebuffer."<br>

</div><div>
<br></div><div>In any case, the patches look correct, and the fact that they fix Portal is pretty convincing.  Patches 1-3 and 8 are:<br><br>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>><br>


<br>Patches 4-7 are:<br><br></div><div>Acked-by: Paul Berry <<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>> <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">



<br>
<br>
This series moves the clamping to the point of use, which is consistent with the 3.2 spec,<br>
and the behavior of NVIDIA's driver.<br>
<br>
Fixes broken rendering in Portal.<br>
<br>
Thanks to Ken for pointing out my misinterpretation of the spec in my original patch.<br>
<br>
-- Chris<br>
<br>
<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</blockquote></div><br></div></div>