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

[Eduardo Lima Mitev]

On 12/08/2016 02:23 PM, Alejandro Piñeiro wrote:
> 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
> 

Yes, this answer from Curro seems relevant to this issue too. I will try
to work around it on the failing test itself, not in Mesa, at least
while the correct implementation is clarified and/or the spec corrected.

Thanks for the pointers!

Eduardo

>>
>> 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;
> 
> 
>