[Mesa-dev] [RFC] main/copyimage: Add check for incomplete src or dst textures

[Alejandro Piñeiro]

On 08/12/16 10:59, Eduardo Lima Mitev wrote:
> From OpenGL 4.5 Core Profile spec, section 18.3.2 (Copying Between Images),
> page 517 of the PDF:
>
>    "An INVALID_OPERATION error is generated if either object is a texture
>     and the texture is not complete (as defined in section 8.17), if the
>     source and destination internal formats are not compatible (see below),
>     or if the number of samples do not match."
>
> The check for texture completeness is currently missing.
>
> Fixes CTS test 'GL45-CTS.copy_image.incomplete_tex'.

Image-texture relationship when textures are incomplete is somewhat
fuzzy by spec. Some time ago I worked on this test:

GL45-CTS.shader_image_load_store.incomplete_textures


And I even send a similar patch to the list:
https://lists.freedesktop.org/archives/mesa-dev/2016-July/123212.html

My patch added texture-completeness checks when the image is accessed,
yours is adding similar checks when copying images.

You can read the thread for the details, but the conclusion was that the
definition of image unit completeness needed extra details. Khronos bug
here:
https://www.khronos.org/bugzilla/show_bug.cgi?id=1916

>
> However, this fix brings up a number of regressions for other CTS and
> Piglit tests related with glCopyImageSubData, because textures that were
> previously assumed complete by a test, don't pass the check now.
>
> For example, Piglit test
> 'piglit.object namespace pollution.program with glcopyimagesubdata'
> $ bin/object-namespace-pollution glCopyImageSubData program -auto -fbo
>
> Analyzing all regressions, my conclusion is that this fix seems legit,
> and the tests' textures are incomplete for different reasons:
>
> For example, the Piglit test above simply creates two textures and perform
> a glCopyImageSubData between them. But by default these textures are
> incomplete because:
>
> * The default minifying filter is NEAREST_MIPMAP_LINEAR (per OpenGL 4.5
> spec, section 8.22. 'TEXTURE STATE AND PROXY STATE', page 269 of the PDF).
>
> * Mesa set the default texture MaxLevel to 1000.
>
> The combination of these two values renders the texture mipmap-incomplete.
> But if I modify the test and add:
>
> glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
>
> or
>
> glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAX_LEVEL, 0);
>
> Then the texture becomes mipmap-complete, passes the check and the test
> passes again.
>
> My confusion comes from the source and destination level parameters of
> glCopyImageSubData. When these levels are zero, do we still consider the
> texture incomplete if it is not mipmap-complete? The spec suggests that
> the levels don't affect that check.
>
> So, my questions are:
>
> * Does this patch looks legit? We are definitely not checking texture
>   completeness in glCopyImageSubData right now, so at least a variation
>   of this patch is necessary.
>
> * Do the level parameters of glCopyImageSubData, assuming they are in
>   range, affect the check for mipmap-completeness of a texture?

I didn't go into all the details of your problem/patch, but I think that
is tightly related with what I mentioned before. If that is the case, I
think that it would be better to wait to khronos spec bug to get resolved.

>
> cheers,
>
> Eduardo
> ---
>  src/mesa/main/copyimage.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/src/mesa/main/copyimage.c b/src/mesa/main/copyimage.c
> index cf25159..9fb5ca9 100644
> --- a/src/mesa/main/copyimage.c
> +++ b/src/mesa/main/copyimage.c
> @@ -580,6 +580,25 @@ _mesa_CopyImageSubData(GLuint srcName, GLenum srcTarget, GLint srcLevel,
>        return;
>     }
>  
> +   if (srcTarget != GL_RENDERBUFFER) {
> +      assert(srcTexImage);
> +      if (!_mesa_is_texture_complete(srcTexImage->TexObject,
> +                                     &srcTexImage->TexObject->Sampler)) {
> +         _mesa_error(ctx, GL_INVALID_OPERATION,
> +                     "glCopyImageSubData(incomplete src texture)");
> +      }
> +   }
> +
> +   if (dstTarget != GL_RENDERBUFFER) {
> +      assert(dstTexImage);
> +      if (!_mesa_is_texture_complete(dstTexImage->TexObject,
> +                                     &dstTexImage->TexObject->Sampler)) {
> +         _mesa_error(ctx, GL_INVALID_OPERATION,
> +                     "glCopyImageSubData(incomplete dst texture)");
> +         return;
> +      }
> +   }
> +
>     /* loop over 2D slices/faces/layers */
>     for (i = 0; i < srcDepth; ++i) {
>        int newSrcZ = srcZ + i;