On 13 May 2013 04:10, Chris Forbes <chr...@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 <stereotype...@gmail.com> Patches 4-7 are: Acked-by: Paul Berry <stereotype...@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
