[Mesa-dev] [PATCH] mesa: Add GL_TEXTURE_CUBE_MAP to _mesa_max_texture_levels().

[Brian Paul]

On 06/05/2012 11:21 AM, Kenneth Graunke wrote:
> On 06/05/2012 07:05 AM, Brian Paul wrote:
>> On 06/05/2012 01:16 AM, Kenneth Graunke wrote:
>>> For cube maps, _mesa_generate_mipmap() calls this with
>>> GL_TEXTURE_CUBE_MAP_ARB (the gl_texture_object's Target) rather than one
>>> of the faces.  This caused _mesa_max_texture_levels() to return 0, which
>>> resulted in maxLevels == -1 and the next line's assertion to fail.
>>>
>>> This function is called from several places.  It looks like texstorage.c
>>> also wants this enum to be handled, while most callers don't care.
>>> TexImage, CompressedTexImage, FramebufferTexture, and GetTexImage
>>> already appear to filter out illegal targets, so they should be fine.
>>>
>>> However, GetTexLevelParameter does rely on this function for filtering
>>> out invalid targets, and does not accept GL_TEXTURE_CUBE_MAP.  Special
>>> case this, since it apparently supports every other target.
>>
>> I think glGet[Compressed]TexImage() needs the special case too.  For
>> example, getteximage_error_check() also uses max levels to error check
>> the target.
>>
>> It might be good to beef-up the comment on _mesa_max_texture_levels() to
>> clarify that the function is also used to error-check 'target' in
>> various places and some care should be taken when adding support for new
>> targets (GL_TEXTURE_CUBE_MAP_ARRAY someday).
>>
>> -Brian
>
> You're right, I think I missed a few cases.
>
> I'm tempted to make more functions like legal_teximage_target() and
> convert the callers to use that for their checking rather than
> implicitly relying on _mesa_max_texture_levels().  It feels like we have
> one function doing target checking for many different API calls...not
> all of which necessarily support the same set of targets.
>
> Thoughts?

Sound OK to me.  The original idea was to avoid doing several 
different switches on one target.  But I'm sure the benefit of clearer 
code would trump any miniscule increase in runtime.

-Brian