[Mesa-dev] [PATCH 12/23] copytexture: update error checking for GLES3

[Ian Romanick]

On 01/09/2013 12:54 AM, Anuj Phogat wrote:
> On Wed, Jan 9, 2013 at 3:38 AM, Jordan Justen <jljusten at gmail.com> wrote:
>> On Mon, Jan 7, 2013 at 12:32 AM, Anuj Phogat <anuj.phogat at gmail.com> wrote:
>>> On Sat, Jan 5, 2013 at 8:11 AM, Jordan Justen <jordan.l.justen at intel.com> wrote:
>>>> Changes based on GTF/gles3 conformance test suite.
>>>>
>>>> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
>>>> ---
>>>>   src/mesa/main/teximage.c |   62 +++++++++++++++++++++++++++++++++-------------
>>>>   1 file changed, 45 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
>>>> index 169e768..cb0084a 100644
>>>> --- a/src/mesa/main/teximage.c
>>>> +++ b/src/mesa/main/teximage.c
>>>> @@ -2370,6 +2370,9 @@ copytexture_error_check( struct gl_context *ctx, GLuint dimensions,
>>>>                            GLint width, GLint height, GLint border )
>>>>   {
>>>>      GLint baseFormat;
>>>> +   GLint rb_base_format;
>>>> +   struct gl_renderbuffer *rb;
>>>> +   GLenum rb_internal_format;
>>>>
>>>>      /* check target */
>>>>      if (!legal_texsubimage_target(ctx, dimensions, target)) {
>>>> @@ -2414,31 +2417,56 @@ copytexture_error_check( struct gl_context *ctx, GLuint dimensions,
>>>>         return GL_TRUE;
>>>>      }
>>>>
>>>> -   /* OpenGL ES 1.x and OpenGL ES 2.0 impose additional restrictions on the
>>>> -    * internalFormat.
>>>> -    */
>>>> -   if (_mesa_is_gles(ctx) && !_mesa_is_gles3(ctx)) {
>>>> -      switch (internalFormat) {
>>>> -      case GL_ALPHA:
>>>> -      case GL_RGB:
>>>> -      case GL_RGBA:
>>>> -      case GL_LUMINANCE:
>>>> -      case GL_LUMINANCE_ALPHA:
>>>> -         break;
>>>> -      default:
>>>> -         _mesa_error(ctx, GL_INVALID_VALUE,
>>>> -                     "glCopyTexImage%dD(internalFormat)", dimensions);
>>>> -         return GL_TRUE;
>>>> -      }
>>>> +   rb = _mesa_get_read_renderbuffer(ctx, internalFormat);
>>>> +   if (rb == NULL) {
>>>> +      _mesa_error(ctx, GL_INVALID_OPERATION,
>>>> +                  "glCopyTexImage%dD(read buffer)", dimensions);
>>>> +      return GL_TRUE;
>>>>      }
>>>>
>>>> +   rb_internal_format = rb->InternalFormat;
>>>>      baseFormat = _mesa_base_tex_format(ctx, internalFormat);
>>>> +   rb_base_format = _mesa_base_tex_format(ctx, rb->InternalFormat);
>>>>      if (baseFormat < 0) {
>>>> -      _mesa_error(ctx, GL_INVALID_VALUE,
>>>> +      _mesa_error(ctx, GL_INVALID_OPERATION,
>>>>                     "glCopyTexImage%dD(internalFormat)", dimensions);
>>>>         return GL_TRUE;
>>>>      }
>>>>
>>>> +   if (_mesa_is_color_format(internalFormat)) {
>>>> +      if (rb_base_format < 0) {
>>>> +         _mesa_error(ctx, GL_INVALID_VALUE,
>>>> +                     "glCopyTexImage%dD(internalFormat)", dimensions);
>>>> +         return GL_TRUE;
>>>> +      }
>>>> +   }
>>>> +
>>>> +   if (_mesa_is_gles(ctx)) {
>>>> +      bool valid = true;
>>>> +      if (_mesa_base_format_component_count(baseFormat) >
>>>> +          _mesa_base_format_component_count(rb_base_format)) {
>>>> +         valid = false;
>>>> +      }
>>>> +      if (baseFormat == GL_DEPTH_COMPONENT ||
>>>> +          baseFormat == GL_DEPTH_STENCIL ||
>>>> +          rb_base_format == GL_DEPTH_COMPONENT ||
>>>> +          rb_base_format == GL_DEPTH_STENCIL ||
>>>> +          ((baseFormat == GL_LUMINANCE_ALPHA ||
>>>> +            baseFormat == GL_ALPHA) &&
>>>> +           rb_base_format != GL_RGBA) ||
>>>> +          internalFormat == GL_RGB9_E5) {
>>>> +         valid = false;
>>>> +      }
>>>> +      if (internalFormat == GL_RGB9_E5) {
>>>> +         valid = false;
>>>> +      }
>>>> +      if (!valid) {
>>>> +         _mesa_error(ctx, GL_INVALID_OPERATION,
>>>> +                     "glCopyTexImage%dD(internalFormat)", dimensions);
>>>> +         return GL_TRUE;
>>>> +      }
>>>> +   }
>>>> +
>>> This looks incorrect as baseFormat=GL_DEPTH_COMPONENT is allowed in
>>> gles3. It was not allowed in gles 1.1 and 2.0 You need to put an additional
>>> check to test  ctx->Version < 30.
>>
>> I think depth/stencil is allowed for GLES3 with TexImage, but not for
>> CopyTexImage. This code will cause CopyTexImage to be rejected on any
>> GLES when the source or dest is depth/stencil.
>>
>> Does that seem correct?
> Yes, Table 3.15 in gles 3.0 spec doesn't allow depth/stencil for CopyTexImage.
> I was referring to gles 3.0 reference pages which now looks incorrect:
> http://www.khronos.org/opengles/sdk/docs/man3/

I've submitted a bug for this.  Good catch.

> I'll modify meta implementation of BlitFramebuffer() to account for this change.
>
>> -Jordan