[Mesa-dev] [PATCH v2 2/3] mesa: validate size parameters for glTexStorage*Multisample

Tue Aug 18 01:13:53 PDT 2015

On 08/18/2015 11:11 AM, Timothy Arceri wrote:
> On Tue, 2015-08-18 at 10:16 +0300, Tapani Pälli wrote:
>>
>> On 08/18/2015 12:30 AM, Timothy Arceri wrote:
>>> On Mon, 2015-08-17 at 08:12 +0300, Tapani Pälli wrote:
>>>>
>>>> On 08/14/2015 04:01 PM, Timothy Arceri wrote:
>>>>> On Fri, 2015-08-14 at 08:55 +0300, Tapani Pälli wrote:
>>>>>>
>>>>>> On 08/13/2015 11:54 AM, Timothy Arceri wrote:
>>>>>>> I've sent a couple of follow-up patches I notice when
>>>>>>> reviewing
>>>>>>> this.
>>>>>>>
>>>>>>>
>>>>>>> On Thu, 2015-08-13 at 09:30 +0300, Tapani Pälli wrote:
>>>>>>>> v2: code cleanup
>>>>>>>>
>>>>>>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>>>>>>> ---
>>>>>>>>      src/mesa/main/teximage.c | 24
>>>>>>>> ++++++++++++++++++++++++
>>>>>>>>      1 file changed, 24 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/src/mesa/main/teximage.c
>>>>>>>> b/src/mesa/main/teximage.c
>>>>>>>> index d35dc12..add7438 100644
>>>>>>>> --- a/src/mesa/main/teximage.c
>>>>>>>> +++ b/src/mesa/main/teximage.c
>>>>>>>> @@ -5782,6 +5782,18 @@ _mesa_TexImage3DMultisample(GLenum
>>>>>>>> target,
>>>>>>>> GLsizei
>>>>>>>> samples,
>>>>>>>>
>>>>>>>>   "glTexImage3DMultisample");
>>>>>>>>      }
>>>>>>>>
>>>>>>>> +static bool
>>>>>>>> +valid_texstorage_ms_parameters(GLsizei width, GLsizei
>>>>>>>> height,
>>>>>>>> GLsizei
>>>>>>>> depth,
>>>>>>>> +                               GLsizei samples, unsigned
>>>>>>>> dims)
>>>>>>>> +{
>>>>>>>> +   GET_CURRENT_CONTEXT(ctx);
>>>>>>>> +
>>>>>>>> +   if (_mesa_tex_storage_invalid_dim(width, height,
>>>>>>>> depth) ||
>>>>>>>> samples < 1)
>>>>>>>
>>>>>>>
>>>>>>> Rather than do the samples < 1 check here you should move
>>>>>>> it to
>>>>>>>      _mesa_texture_image_multisample as the spec says "An
>>>>>>> INVALID_VALUE error is
>>>>>>> generated ifsamplesis zero." for glTexImage*Multisample
>>>>>>> too.
>>>>>>>
>>>>>>> With that change you could change all the calls below to
>>>>>>> something
>>>>>>> like
>>>>>>>
>>>>>>>        if (_mesa_tex_storage_invalid_dim(width, height,
>>>>>>> depth)) {
>>>>>>>           _mesa_error(ctx, GL_INVALID_VALUE,
>>>>>>> "glTexStorage2DMultisample()"
>>>>>>>           return;
>>>>>>>        }
>>>>>>
>>>>>> Would it be OK if we do possible unification for checking
>>>>>> samples
>>>>>> parameter after these patches? My goal here is to do check
>>>>>> for
>>>>>> glTexStorage*Multisample only. I don't feel comfortable
>>>>>> changing
>>>>>> checks
>>>>>> for other functions at the same time, that should deserve
>>>>>> it's own
>>>>>> commit so it's easier to bisect later.
>>>>>>
>>>>>
>>>>> There is no reason to split the change into two patches. The
>>>>> correct
>>>>> change is to update _mesa_texture_image_multisample (which I
>>>>> will
>>>>> rename texture_image_multisample once your patch lands) to do
>>>>> the
>>>>> samples check. The only other user is image*Multisample and the
>>>>> spec
>>>>> clearly states that this is also an error when 0. That spec
>>>>> says it
>>>>> undefined what happens when this is negative but we already
>>>>> throw an
>>>>> error via _mesa_check_sample_count().
>>>>
>>>> OK, how about land your patch first so that we can test for
>>>> regressions.
>>>> If nothing regresses after your changes then I can safely land
>>>> mine.
>>>
>>> The changes are pushed to master :)
>>
>> OK nice, I've updated my patch to not check samples (only
>> dimensions).
>> Do you want me to send it separately or are you ok giving r-b with
>> these
>> changes?
>>
>
> Sorry just one more nit, it would be nice to print width, height, depth
> as part of the error message:
>
>
> _mesa_error(ctx, GL_INVALID_VALUE,
> "glTexStorage%uDMultisample(width=%d,height=%d,depth=%d)", dims, width,
> height, depth);

Makes sense, thanks!

> With that feel free to add my r-b.
>
>
>>
>>>>
>>>>> You don't have to touch _mesa_check_sample_count() that change
>>>>> should
>>>>> be in another patch which I already sent [1] feel free not to
>>>>> review it
>>>>> :)
>>>>>
>>>>> Doing it the way your suggesting is code churn for no gain, it
>>>>> just
>>>>> means your code needs to be deleted and replaced with the
>>>>> change I'm
>>>>> suggesting later anyway, so why not just fix it right the first
>>>>> time.
>>>>
>>>> Yes, maybe I'm too pessimistic that nothing would break. It's
>>>> just that
>>>> typically something does and I've been really hard trying to not
>>>> break
>>>> ES 3.0 conformance (which includes ES 2.0 tests too) while making
>>>> ES 3.1
>>>> changes. We are very close to being ES 3.0 conformant (just one
>>>> test
>>>> failing ATM).
>>>
>>> Nice work. I can see where your coming from but if we are going to
>>> break
>>> things now is the time to do it while people like yourself are
>>> keeping a close
>>> eye on things :)
>>>
>>>>
>>>>> Also feel free to keep your valid_texstorage_ms_parameters()
>>>>> function I
>>>>> was just suggesting an alternative to having another function
>>>>> call.
>>>>>
>>>>>
>>>>> [1]
>>>>> http://lists.freedesktop.org/archives/mesa-dev/2015-August/0915
>>>>> 88.html
>>>>>>>> {
>>>>>>>> +      _mesa_error(ctx, GL_INVALID_VALUE,
>>>>>>>> "glTexStorage%uDMultisample()",
>>>>>>>> dims);
>>>>>>>> +      return false;
>>>>>>>> +   }
>>>>>>>> +   return true;
>>>>>>>> +}
>>>>>>>>      void GLAPIENTRY
>>>>>>>>      _mesa_TexStorage2DMultisample(GLenum target, GLsizei
>>>>>>>> samples,
>>>>>>>> @@ -5795,6 +5807,9 @@
>>>>>>>> _mesa_TexStorage2DMultisample(GLenum
>>>>>>>> target, GLsizei
>>>>>>>> samples,
>>>>>>>>         if (!texObj)
>>>>>>>>            return;
>>>>>>>>
>>>>>>>> +   if (!valid_texstorage_ms_parameters(width, height, 1,
>>>>>>>> samples, 2))
>>>>>>>> +      return;
>>>>>>>> +
>>>>>>>>         _mesa_texture_image_multisample(ctx, 2, texObj,
>>>>>>>> target,
>>>>>>>> samples,
>>>>>>>>                                         internalformat,
>>>>>>>> width,
>>>>>>>> height, 1,
>>>>>>>>
>>>>>>>>   fixedsamplelocations,
>>>>>>>> GL_TRUE,
>>>>>>>> @@ -5814,6 +5829,9 @@
>>>>>>>> _mesa_TexStorage3DMultisample(GLenum
>>>>>>>> target, GLsizei
>>>>>>>> samples,
>>>>>>>>         if (!texObj)
>>>>>>>>            return;
>>>>>>>>
>>>>>>>> +   if (!valid_texstorage_ms_parameters(width, height,
>>>>>>>> depth,
>>>>>>>> samples, 3))
>>>>>>>> +      return;
>>>>>>>> +
>>>>>>>>         _mesa_texture_image_multisample(ctx, 3, texObj,
>>>>>>>> target,
>>>>>>>> samples,
>>>>>>>>                                         internalformat,
>>>>>>>> width,
>>>>>>>> height, depth,
>>>>>>>>
>>>>>>>>   fixedsamplelocations,
>>>>>>>> GL_TRUE,
>>>>>>>> @@ -5834,6 +5852,9 @@
>>>>>>>> _mesa_TextureStorage2DMultisample(GLuint
>>>>>>>> texture,
>>>>>>>> GLsizei samples,
>>>>>>>>         if (!texObj)
>>>>>>>>            return;
>>>>>>>>
>>>>>>>> +   if (!valid_texstorage_ms_parameters(width, height, 1,
>>>>>>>> samples, 2))
>>>>>>>> +      return;
>>>>>>>> +
>>>>>>>>         _mesa_texture_image_multisample(ctx, 2, texObj,
>>>>>>>> texObj
>>>>>>>> ->Target, samples,
>>>>>>>>                                         internalformat,
>>>>>>>> width,
>>>>>>>> height, 1,
>>>>>>>>
>>>>>>>>   fixedsamplelocations,
>>>>>>>> GL_TRUE,
>>>>>>>> @@ -5855,6 +5876,9 @@
>>>>>>>> _mesa_TextureStorage3DMultisample(GLuint
>>>>>>>> texture,
>>>>>>>> GLsizei samples,
>>>>>>>>         if (!texObj)
>>>>>>>>            return;
>>>>>>>>
>>>>>>>> +   if (!valid_texstorage_ms_parameters(width, height,
>>>>>>>> depth,
>>>>>>>> samples, 3))
>>>>>>>> +      return;
>>>>>>>> +
>>>>>>>>         _mesa_texture_image_multisample(ctx, 3, texObj,
>>>>>>>> texObj
>>>>>>>> ->Target, samples,
>>>>>>>>                                         internalformat,
>>>>>>>> width,
>>>>>>>> height, depth,
>>>>>>>>
>>>>>>>>   fixedsamplelocations,
>>>>>>>> GL_TRUE,