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

Tue Jan 31 19:55:39 UTC 2017

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

> I think that the test is just wrong. Can you file an issue asking about 
> that?
> 
> Thanks,
> Nicolai
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev