[Mesa-dev] [PATCH] texgetimage: Check that a multisample tex is not passed to GetTextureSubImage

[Eduardo Lima Mitev]

On 02/02/2017 07:38 PM, Ilia Mirkin wrote:
> On Thu, Feb 2, 2017 at 1:36 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> On Thu, Feb 2, 2017 at 1:29 PM, Eduardo Lima Mitev <elima at igalia.com> wrote:
>>> OpenGL 4.5 spec, section "8.11.4 Texture Image Queries", page 233 of
>>> the PDF states:
>>>
>>>     "An INVALID_OPERATION error is generated if texture is the name of a buffer
>>>      or multisample texture."
>>>
>>> Currently, this is not being checked and the multisample texture image is passed
>>> down to the driver hook. On i965, it is crashing the driver with an assertion:
>>>
>>> intel_mipmap_tree.c:3125: intel_miptree_map: Assertion `mt->num_samples <= 1' failed.
>>> ---
>>>  src/mesa/main/texgetimage.c | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/src/mesa/main/texgetimage.c b/src/mesa/main/texgetimage.c
>>> index d5cb1636605..6a23e4bbb8c 100644
>>> --- a/src/mesa/main/texgetimage.c
>>> +++ b/src/mesa/main/texgetimage.c
>>> @@ -1185,6 +1185,20 @@ getteximage_error_check(struct gl_context *ctx,
>>>     texImage = select_tex_image(texObj, target, level, zoffset);
>>>     assert(texImage);
>>>
>>> +   /* Check that texObj is not a buffer or multisample texture if called
>>> +    * from glGetTextureSubImage. OpenGL 4.5 spec, section "8.11.4 Texture
>>> +    * Image Queries", page 233 of the PDF states:
>>> +    *
>>> +    *     "An INVALID_OPERATION error is generated if texture is the
>>> +    *      name of a buffer or multisample texture."
>>> +    */
>>> +   if (texImage->NumSamples > 0 &&
>>> +       strcmp(caller, "glGetTextureSubImage") == 0) {
>>
>> So... glGetTextureSubImage is not OK but glGetTextureImage is OK?
>>
>> I think the issue is a missing
>>
>>    if (!legal_getteximage_target(ctx, texObj->Target, true)) {
>>       _mesa_error(ctx, GL_INVALID_ENUM, "%s", caller);
>>       return;
>>    }
>>
>> block in _mesa_GetTextureSubImage.
> 
> Or actually I guess this should get a custom block in that function
> (since it's actually different from GetTextureImage). Either way, the
> strcmp is not the way to go.
> 

Note that this is not about the target, but the texture image object
itself, which shouldn't had been initialized with glTexImage2DMultisample.

I agree that using strcmp against the caller is not very elegant, but
passing another argument to getteximage_error_check() to discriminate
caller is even worse, IMO, since we already have a "caller" arg.

So, will you be ok with moving the check to _mesa_GetTextureSubImage,
duplicating the code necessary to get the gl_texture_image object?

IMHO, both texgetimage.c and teximage.c can use a good refactoring.

Thanks for the feedback,
Eduardo

>>
>>> +      _mesa_error(ctx, GL_INVALID_OPERATION,
>>> +                  "%s(multisample texture)", caller);
>>> +      return true;
>>> +   }
>>> +
>>>     /*
>>>      * Format and type checking has been moved up to GetnTexImage and
>>>      * GetTextureImage so that it happens before getting the texImage object.
>>> --
>>> 2.11.0
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>