On 09/14/2015 08:47 PM, Jason Ekstrand wrote:
> Actually, a couple of comments...
>
> On Fri, Sep 4, 2015 at 7:21 AM, Eduardo Lima Mitev <el...@igalia.com> wrote:
>> When validating format+type+internalFormat for texture pixel operations
>> on GLES3, the effective internal format should be used if the one
>> specified is an unsized internal format. Page 127, section "3.8 Texturing"
>> of the GLES 3.0.4 spec says:
>>
>> "if internalformat is a base internal format, the effective internal
>> format is a sized internal format that is derived from the format and
>> type for internal use by the GL. Table 3.12 specifies the mapping of
>> format and type to effective internal formats. The effective internal
>> format is used by the GL for purposes such as texture completeness or
>> type checks for CopyTex* commands. In these cases, the GL is required
>> to operate as if the effective internal format was used as the
>> internalformat when specifying the texture data."
>>
>> v2: Per the spec, Luminance8Alpha8, Luminance8 and Alpha8 should not be
>> considered sized internal formats. Return the corresponding unsize format
>> instead.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91582
>> ---
>> src/mesa/main/glformats.c | 127
>> ++++++++++++++++++++++++++++++++++++++++++++++
>> src/mesa/main/teximage.c | 12 ++---
>> 2 files changed, 133 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/mesa/main/glformats.c b/src/mesa/main/glformats.c
>> index e4e0fdc..ccd468d 100644
>> --- a/src/mesa/main/glformats.c
>> +++ b/src/mesa/main/glformats.c
>> @@ -2680,6 +2680,103 @@ _mesa_base_tex_format(const struct gl_context *ctx,
>> GLint internalFormat)
>> }
>>
>> /**
>> + * Returns the effective internal format from a texture format and type.
>> + * This is used by texture image operations internally for validation, when
>> + * the specified internal format is a base (unsized) format.
>> + *
>> + * This method will only return a valid effective internal format if the
>> + * combination of format, type and internal format in base form, is
>> acceptable.
>> + *
>> + * \param format the texture format
>> + * \param type the texture type
>> + */
>> +static GLenum
>> +_mesa_es3_effective_internal_format_for_format_and_type(GLenum format,
>> + GLenum type)
>> +{
>> + switch (type) {
>> + case GL_UNSIGNED_BYTE:
>> + switch (format) {
>> + case GL_RGBA:
>> + return GL_RGBA8;
>> + case GL_RGB:
>> + return GL_RGB8;
>> + case GL_LUMINANCE_ALPHA:
>> + case GL_LUMINANCE:
>> + case GL_ALPHA:
>> + return format;
>
> According to the table, these have explicit LUMINANCE8_ALPHA8 forms.
> Why are we not using those? At the very least, it deserves a comment.
>
The table 3.12 (page 128 of the OpenGL-ES 3.0.4 PDF) says:
"Table 3.12: Effective internal format corresponding to external for-
mat and type. *Formats in italics do not correspond to GL constants.*"
In this case, Luminance8Alpha8, Luminance8 and Alpha8 are in italics, so
I left them out in this version of the patch (I initially included
them.. see the v1 text in the commit log).
I agree that it deserves a comment. Will add that.
>> + }
>> + break;
>> +
>> + case GL_UNSIGNED_SHORT_4_4_4_4:
>> + if (format == GL_RGBA)
>> + return GL_RGBA4;
>> + break;
>> +
>> + case GL_UNSIGNED_SHORT_5_5_5_1:
>> + if (format == GL_RGBA)
>> + return GL_RGB5_A1;
>> + break;
>> +
>> + case GL_UNSIGNED_SHORT_5_6_5:
>> + if (format == GL_RGB)
>> + return GL_RGB565;
>> + break;
>> +
>> + case GL_UNSIGNED_INT:
>> + case GL_UNSIGNED_SHORT:
>> + switch (format) {
>> + case GL_DEPTH_COMPONENT:
>> + return format;
>
> This case and the cases below don't actually show up in the table.
> I'm not sure what I think about having them in this function. They
> seem like some sort of a "default" case. Unless, of course, the
> intention is to simply validate that you're providing a valid
> format/type combination. If that's the case, then it might be worth
> having a comment to that effect.
>
Yes, the cases that are not in that table are either defined in other
table (like depth/stencil, which are defined in Table 3.14 of same PDF);
or in an extension (like half float).
The combinations that return the same format are indeed provided for
validation, as you comment. Also, in cases when there is not a single
sized internal format for a particular base format, my interpretation is
that the base format is used to work around the ambiguity.
I will add comments to clarify this too.
>> + }
>> + break;
>> +
>> + /* OES_texture_float and OES_texture_half_float */
>> + case GL_FLOAT:
>> + case GL_HALF_FLOAT_OES:
>> + switch (format) {
>> + case GL_RGBA:
>> + case GL_RGB:
>> + case GL_LUMINANCE_ALPHA:
>> + case GL_LUMINANCE:
>> + case GL_ALPHA:
>> + case GL_RED:
>> + case GL_RG:
>> + return format;
>> + }
>> + break;
>> + case GL_HALF_FLOAT:
>> + switch (format) {
>> + case GL_RG:
>> + case GL_RED:
>> + return format;
>> + }
>> + break;
>> +
>> + /* OES_packed_depth_stencil */
>> + case GL_UNSIGNED_INT_24_8:
>> + if (format == GL_DEPTH_STENCIL)
>> + return GL_DEPTH24_STENCIL8;
>> + break;
>> +
>> + /* GL_EXT_texture_type_2_10_10_10_REV */
>> + case GL_UNSIGNED_INT_2_10_10_10_REV:
>> + switch (format) {
>> + case GL_RGBA:
>> + case GL_RGB:
>> + return format;
>> + }
>> + break;
>> +
>> + default:
>> + /* fall through and return NONE */
>> + break;
>> + }
>> +
>> + return GL_NONE;
>> +}
>> +
>> +/**
>> * Do error checking of format/type combinations for OpenGL ES 3
>> * glTex[Sub]Image.
>> * \return error code, or GL_NO_ERROR.
>> @@ -2689,6 +2786,36 @@ _mesa_es3_error_check_format_and_type(const struct
>> gl_context *ctx,
>> GLenum format, GLenum type,
>> GLenum internalFormat)
>> {
>> + /* If internalFormat is an unsized format, then the effective internal
>> + * format derived from format and type should be used instead. Page 127,
>> + * section "3.8 Texturing" of the GLES 3.0.4 spec states:
>> + *
>> + * "if internalformat is a base internal format, the effective
>> + * internal format is a sized internal format that is derived
>> + * from the format and type for internal use by the GL.
>> + * Table 3.12 specifies the mapping of format and type to effective
>> + * internal formats. The effective internal format is used by the GL
>> + * for purposes such as texture completeness or type checks for
>> + * CopyTex* commands. In these cases, the GL is required to operate
>> + * as if the effective internal format was used as the internalformat
>> + * when specifying the texture data."
>> + */
>> + if (_mesa_is_enum_format_unsized(internalFormat)) {
>> + GLenum effectiveInternalFormat =
>> + _mesa_es3_effective_internal_format_for_format_and_type(format,
>> type);
>> +
>> + if (effectiveInternalFormat == GL_NONE)
>> + return GL_INVALID_OPERATION;
>> +
>> + GLenum baseInternalFormat =
>> + _mesa_base_tex_format(ctx, effectiveInternalFormat);
>> +
>> + if (internalFormat != baseInternalFormat)
>> + return GL_INVALID_OPERATION;
>> +
>> + internalFormat = effectiveInternalFormat;
>> + }
>> +
>> switch (format) {
>> case GL_RGBA:
>> switch (type) {
>> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
>> index d9453e3..09040d5 100644
>> --- a/src/mesa/main/teximage.c
>> +++ b/src/mesa/main/teximage.c
>> @@ -2117,6 +2117,12 @@ texsubimage_error_check(struct gl_context *ctx,
>> GLuint dimensions,
>> return GL_TRUE;
>> }
>>
>> + if (error_check_subtexture_dimensions(ctx, dimensions,
>> + texImage, xoffset, yoffset,
>> zoffset,
>> + width, height, depth, callerName))
>> {
>> + return GL_TRUE;
>> + }
>> +
>> err = _mesa_error_check_format_and_type(ctx, format, type);
>> if (err != GL_NO_ERROR) {
>> _mesa_error(ctx, err,
>> @@ -2145,12 +2151,6 @@ texsubimage_error_check(struct gl_context *ctx,
>> GLuint dimensions,
>> return GL_TRUE;
>> }
>>
>> - if (error_check_subtexture_dimensions(ctx, dimensions,
>> - texImage, xoffset, yoffset,
>> zoffset,
>> - width, height, depth, callerName))
>> {
>> - return GL_TRUE;
>> - }
>
> Why did this get moved in this patch?
>
Yes, this is a bit controversial and it should probably go in a
different patch.
The issue is that this change uncovers a miss-order of validation that
bring regressions. It was working before by chance, because the type of
errors matched.
I will split the patch.
> Sorry for the previous e-mail. I thought I had read it but aparently
> didn't read carefully enough.
No prob. Thanks for the review!
Eduardo
> --Jason
>
>> -
>> if (_mesa_is_format_compressed(texImage->TexFormat)) {
>> if (_mesa_format_no_online_compression(ctx,
>> texImage->InternalFormat)) {
>> _mesa_error(ctx, GL_INVALID_OPERATION,
>> --
>> 2.4.6
>>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
