[Mesa-dev] [PATCH v2 1/2] mesa: make glFramebuffer* check immutable texture level bounds

[Nicolai Hähnle]

On 30.01.2017 19:09, Ilia Mirkin wrote:
> On Mon, Jan 30, 2017 at 1:06 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> On Mon, Jan 30, 2017 at 12:26 PM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
>>> On 30.01.2017 18:23, Ilia Mirkin wrote:
>>>>
>>>> On Mon, Jan 30, 2017 at 4:36 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>>
>>>>> On Mon, Jan 30, 2017 at 4:33 AM, Nicolai Hähnle <nhaehnle at gmail.com>
>>>>> wrote:
>>>>>>
>>>>>> On 26.01.2017 06:47, Ilia Mirkin wrote:
>>>>>>>
>>>>>>>
>>>>>>> When a texture is immutable, we can't tack on extra levels
>>>>>>> after-the-fact like we could with glTexImage. So check against that
>>>>>>> level limit and return an error if it's surpassed.
>>>>>>>
>>>>>>> The spec is a little unclear in that it says to check if "level is not
>>>>>>> a
>>>>>>> supported texture level", however that is never fully defined.
>>>>>>>
>>>>>>> This fixes:
>>>>>>> GL45-CTS.geometry_shader.layered_fbo.fb_texture_invalid_level_number
>>>>>>>
>>>>>>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>>>>>> ---
>>>>>>>
>>>>>>> v1 -> v2: use NumLevels instead of _MaxLevel.
>>>>>>>
>>>>>>> Not sure why this isn't showing up as failing in the Intel CI, but it
>>>>>>> was
>>>>>>> definitely failing here.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Maybe the Intel CI is running the GLCTS based on the last 4.5 release,
>>>>>> and I
>>>>>> guess you're running off what's been published on Github? The GLCTS on
>>>>>> Github has a bunch of new and possibly broken tests, and may still have
>>>>>> a
>>>>>> number of regressions as well (since a lot of code was moved around).
>>>>>>
>>>>>> Can you point out which specific place of the spec you're talking about
>>>>>> in
>>>>>> your comment?
>>>>>
>>>>>
>>>>> One of the errors listed for glFramebufferTexture is:
>>>>>
>>>>> """
>>>>> An INVALID_VALUE error is generated if texture is not zero and is not the
>>>>> name of a texture object, or if level is not a supported texture level
>>>>> for texture
>>>>> """
>>>>
>>>>
>>>> Curiously for glFramebufferTexture1D/2D/3D, it also says:
>>>>
>>>> """
>>>> If textarget is TEXTURE_RECTANGLE or TEXTURE_2D_MULTISAMPLE, then
>>>> level must be zero. If textarget is TEXTURE_3D, then level must be
>>>> greater than or equal to zero and less than or equal to log2 of the
>>>> value of MAX_3D_TEXTURE_- SIZE. If textarget is one of the cube map
>>>> face targets from table 8.19, then level must be greater than or equal
>>>> to zero and less than or equal to log2 of the value of
>>>> MAX_CUBE_MAP_TEXTURE_SIZE. For all other values of textarget, level
>>>> must be greater than or equal to zero and no larger than log2 of the
>>>> value of MAX_- TEXTURE_SIZE.
>>>> """
>>>>
>>>> which matches the current code. I guess this patch is withdrawn...
>>>
>>>
>>> What a coincidence, I've been staring at this for a while now and been
>>> getting increasingly confused. Which function does the CTS actually test
>>> (and possibly fail incorrectly)? Maybe it's time to open an issue against
>>> the CTS.
>>
>> https://github.com/KhronosGroup/VK-GL-CTS/blob/c9921995d8d360bd34d8672194d7c095bb376f82/external/openglcts/modules/glesext/geometry_shader/esextcGeometryShaderLayeredFBO.cpp#L1062
>
> A thought occurred to me... whereas glFramebufferTexture1D/etc talk
> about binding points, glFramebufferTexture talks about a specific
> texture object. When are the bindings resolved? at
> glFramebufferTexture2D time, or at draw time? If the latter, then the
> spec has no choice but to just check the maxima for the binding point
> ones...

The problem is that in all cases, the error table lists INVALID_ENUM 
when "level is not a supported texture level for $foo", but it's not 
explicitly stated how "supported texture level" is defined for 
FramebufferTexture.

However, I did a bit more searching, and there's language below 
FramebufferTextureLayer which matches that of FramebufferTexture1D/2D/3D:

"If texture is a three-dimensional texture, then level must be greater 
than or equal to zero and less than or equal to log 2 of the value of 
MAX_3D_TEXTURE_SIZE. If texture is a two-dimensional array texture, then 
level must be greater than or equal to zero and no larger than log 2 of 
the value of MAX_TEXTURE_SIZE."

It would be odd to have different requirements for FramebufferTexture 
and FramebufferTextureLayer.

Furthermore, like you said, I think the bindings are supposed to be 
resolved at draw time. So if you re-define a texture with 
TexStorage1D/2D/3D, the binding survives. This matches what Mesa does 
(see the various update_fbo_texture functions).

The language in the ES spec seems to match that of GL.

I think that the test is just wrong. Can you file an issue asking about 
that?

Thanks,
Nicolai