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

[Ilia Mirkin]

On Thu, Feb 2, 2017 at 1:58 PM, Eduardo Lima Mitev <elima at igalia.com> wrote:
> 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.

I think that a block in _mesa_GetTextureSubImage makes the most sense.
Note that it not only needs to check for multisampled, but also for
"not a buffer texture".

When would texObj->Target not be a good enough thing to check against
it being a MS target?

>
> 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
>>
>