[Mesa-dev] [PATCH v2 2/3] mesa: validate size parameters for glTexStorage*Multisample
Tapani Pälli
tapani.palli at intel.com
Tue Aug 18 00:16:00 PDT 2015
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?
>>
>>> 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/091588.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,
More information about the mesa-dev
mailing list