[Mesa-dev] [PATCH] mesa/main: condition GL_DEPTH_STENCIL on ARB_depth_texture

[Ilia Mirkin]

On Thu, Mar 13, 2014 at 11:40 AM, Ian Romanick <idr at freedesktop.org> wrote:
> On 03/13/2014 06:33 AM, Ilia Mirkin wrote:
>> On Thu, Mar 13, 2014 at 9:22 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>>> Ilia Mirkin <imirkin at alum.mit.edu> writes:
>>>
>>>> EXT_packed_depth_stencil is supported by all drivers, but
>>>> ARB_depth_texture isn't (notably nouveau_vieux). This should avoid
>>>> passing unexpected values down to ChooseTextureFormat.
>>>>
>>>> The EXT_packed_depth_stencil spec does not make any explicit references
>>>> to requiring ARB_depth_texture in order to allow textures with that
>>>> format, however if there is no dependency, ARB_depth_texture would be
>>>> practically implied by the extension.
>>>>
>>>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>>> ---
>>>>
>>>> It should also be noted that ARB_depth_texture became required in OpenGL 1.4,
>>>> so this shouldn't affect too many drivers. Looks like i915 only has it
>>>> disabled for gen2. i965 always enables it. Glancing at radeon, it seems like
>>>> the values are supported in ChooseTextureFormat, but not actually handled in
>>>> radeon_texstate or r200_texstate.
>>>>
>>>>  src/mesa/main/teximage.c | 11 +++--------
>>>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
>>>> index a6c3581..a57a535 100644
>>>> --- a/src/mesa/main/teximage.c
>>>> +++ b/src/mesa/main/teximage.c
>>>> @@ -160,6 +160,9 @@ _mesa_base_tex_format( struct gl_context *ctx, GLint internalFormat )
>>>>           case GL_DEPTH_COMPONENT24:
>>>>           case GL_DEPTH_COMPONENT32:
>>>>              return GL_DEPTH_COMPONENT;
>>>> +         case GL_DEPTH_STENCIL:
>>>> +         case GL_DEPTH24_STENCIL8:
>>>> +            return GL_DEPTH_STENCIL;
>>>>           default:
>>>>              ; /* fallthrough */
>>>>        }
>>>> @@ -301,14 +304,6 @@ _mesa_base_tex_format( struct gl_context *ctx, GLint internalFormat )
>>>>        }
>>>>     }
>>>>
>>>> -   switch (internalFormat) {
>>>> -   case GL_DEPTH_STENCIL:
>>>> -   case GL_DEPTH24_STENCIL8:
>>>> -      return GL_DEPTH_STENCIL;
>>>> -   default:
>>>> -      ; /* fallthrough */
>>>> -   }
>>>> -
>>>>     if (ctx->Extensions.EXT_texture_sRGB) {
>>>>        switch (internalFormat) {
>>>>        case GL_SRGB_EXT:
>>>> --
>>>> 1.8.3.2
>>>
>>>
>>> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
>>
>> Thanks. I'm going to wait a while before pushing this to see if others
>> have differing opinions on the spec interpretation. Ian, Eric, Brian?
>> You guys seem to be fairly in-tune with this stuff.
>
> The body of the spec may not be clear, but the overview at least is:
>
>     This extension provides a new data format, GL_DEPTH_STENCIL_EXT,
>     that can be used with the glDrawPixels, glReadPixels, and
>     glCopyPixels commands, as well as a packed data type,
>     GL_UNSIGNED_INT_24_8_EXT, that is meant to be used with
>     GL_DEPTH_STENCIL_EXT.  No other data types are supported with
>     GL_DEPTH_STENCIL_EXT.  If ARB_depth_texture or SGIX_depth_texture is
>     supported, GL_DEPTH_STENCIL_EXT/GL_UNSIGNED_INT_24_8_EXT data can
>     also be used for textures; this provides a more efficient way to
>     supply data for a 24-bit depth texture.
>
> I thought I cleaned all this up when I made EXT_packed_depth_stencil
> non-optional, but it looks like I missed it.  Thanks for taking care of
> this.
>
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

Great. Another possible interpretation would be that if
ARB_depth_texture is supported, then one is allowed to pass a
(GL_DEPTH_STENCIL_EXT, GL_UNSIGNED_INT_24_8_EXT) format/type data to a
GL_DEPTH_COMPONENT texture. But I much prefer to interpret it as "if
depth textures are supported, then they can also have a DEPTH_STENCIL
internal format", since that makes it workable on nouveau_vieux with
this simple change :)

  -ilia