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.
> + }
> + 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.
> + }
> + 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?
Sorry for the previous e-mail. I thought I had read it but aparently
didn't read carefully enough.
--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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
