[Mesa-dev] [PATCH 0/8] Clamp stencil reference value at use, not at specification time.

[Paul Berry]

On 13 May 2013 04:10, Chris Forbes <chrisf at ijw.co.nz> wrote:

> Replaces: "[PATCH] mesa: Fix meta smashing stencil reference value on
> restore"
>
> This series changes the way stencil reference values are clamped.
> Previously,
> the values would be clamped according to the bit depth of the stencil
> buffer
> bound _when the the value was specified_. This looked correct in most
> cases, but fell apart
> in some cases where the stencil buffer validated against is not the same
> as the buffer that
> would be rendered into.
>
>
> A simple example is this sequence, which would produce a reference value
> of zero,
> even though the user probably meant otherwise:
>
> 1. BindFramebuffer( .. fbo with NO stencil )
> 2. StencilFuncSeparate( .. 0xff .. )
> 3. BindFramebuffer( .. fbo with stencil )
>
>
> A more realistic case involves a meta op restoring the stencil state,
> which is done by calling
> the StencilFuncSeparate() API just the same as the user:
>
> 1. BindFramebuffer( .. fbo with stencil )
> 2. StencilFuncSeparate( .. 0xff .. )
> 3. BindFramebuffer( .. fbo with NO stencil .. )
> 4. <some meta op>
> 5. BindFramebuffer( .. fbo with stencil )
>
> In this case, the stencil reference value would be revalidated against
> zero stencil
> bits at (4) and so smashed to zero. This sequence is from Portal.
>

Two minor comments:

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.

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."

In any case, the patches look correct, and the fact that they fix Portal is
pretty convincing.  Patches 1-3 and 8 are:

Reviewed-by: Paul Berry <stereotype441 at gmail.com>

Patches 4-7 are:

Acked-by: Paul Berry <stereotype441 at gmail.com>


>
>
> This series moves the clamping to the point of use, which is consistent
> with the 3.2 spec,
> and the behavior of NVIDIA's driver.
>
> Fixes broken rendering in Portal.
>
> Thanks to Ken for pointing out my misinterpretation of the spec in my
> original patch.
>
> -- Chris
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130513/acd73aae/attachment.html>