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

Wed Feb 1 05:37:42 UTC 2017

On Tue, Jan 31, 2017 at 2:55 PM, Antía Puentes <apuentes at igalia.com> wrote:
> On mar, 2017-01-31 at 12:09 +0100, Nicolai Hähnle wrote:
>> 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  wrote:
>> > > >
>> > > > On 30.01.2017 18:23, Ilia Mirkin wrote:
>> > > > >
>> > > > >
>> > > > > On Mon, Jan 30, 2017 at 4:36 AM, Ilia Mirkin  wrote:
>> > > > > >
>> > > > > >
>> > > > > > On Mon, Jan 30, 2017 at 4:33 AM, Nicolai Hähnle
>> > > > > > 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.
>
> There was an update in the OpenGL ES 3.2 specification (November 3,
> 2016) addressing this:
>
> - In section "9.2.8 Attaching Texture Images to a Framebuffer",
> FramebufferTexture2D (page 241) and FramebufferTextureLayer (page 242)
> descriptions:
>
> "<level> specifies the mipmap level of the texture image to be attached
> to the framebuffer, and must satisfy the following conditions:
>
> • If texture refers to an immutable-format texture, level must be
> greater than or equal to zero and smaller than the value of
> TEXTURE_IMMUTABLE_LEVELS for texture."
>
> https://cvs.khronos.org/bugzilla/show_bug.cgi?id=15946

Interesting, OK. And the test that's failing is a "glesext" test, even
though it's being run in a GL context. Does that bug indicate whether
such a change also applies to desktop GL? (I don't have access to the
bug tracker.)

  -ilia