On Thu, Feb 2, 2017 at 1:58 PM, Eduardo Lima Mitev <el...@igalia.com> wrote:
> On 02/02/2017 07:38 PM, Ilia Mirkin wrote:
>> On Thu, Feb 2, 2017 at 1:36 PM, Ilia Mirkin <imir...@alum.mit.edu> wrote:
>>> On Thu, Feb 2, 2017 at 1:29 PM, Eduardo Lima Mitev <el...@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@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
