<div dir="ltr">On Thu, Nov 21, 2013 at 5:56 PM, Brian Paul <<a href="mailto:brianp@vmware.com" target="_blank">brianp@vmware.com</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">I'd rename this summary as something like "mesa: implement the _mesa_TextureView() function"<div> <div class="h5"> On 11/19/2013 04:16 PM, Courtney Goeltzenleuchter wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> Add Mesa TextureView logic. Incorporate feedback on ARB_texture_view Signed-off-by: Courtney Goeltzenleuchter <courtney@LunarG.com> --- src/mesa/main/textureview.c | 562 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 561 insertions(+), 1 deletion(-) diff --git a/src/mesa/main/textureview.c b/src/mesa/main/textureview.c index 4a6bd62..1858465 100644 --- a/src/mesa/main/textureview.c +++ b/src/mesa/main/textureview.c @@ -40,8 +40,346 @@ #include "texobj.h" #include "texstorage.h" #include "textureview.h" +#include "stdbool.h" #include "mtypes.h" +/* Table 3.X.2 (Compatible internal formats for TextureView) + --------------------------------------------------------------------------- + | Class | Internal formats | + --------------------------------------------------------------------------- + | VIEW_CLASS_128_BITS | RGBA32F, RGBA32UI, RGBA32I | + --------------------------------------------------------------------------- + | VIEW_CLASS_96_BITS | RGB32F, RGB32UI, RGB32I | + --------------------------------------------------------------------------- + | VIEW_CLASS_64_BITS | RGBA16F, RG32F, RGBA16UI, RG32UI, RGBA16I, | + | | RG32I, RGBA16, RGBA16_SNORM | + --------------------------------------------------------------------------- + | VIEW_CLASS_48_BITS | RGB16, RGB16_SNORM, RGB16F, RGB16UI, RGB16I | + --------------------------------------------------------------------------- + | VIEW_CLASS_32_BITS | RG16F, R11F_G11F_B10F, R32F, | + | | RGB10_A2UI, RGBA8UI, RG16UI, R32UI, | + | | RGBA8I, RG16I, R32I, RGB10_A2, RGBA8, RG16, | + | | RGBA8_SNORM, RG16_SNORM, SRGB8_ALPHA8, RGB9_E5 | + --------------------------------------------------------------------------- + | VIEW_CLASS_24_BITS | RGB8, RGB8_SNORM, SRGB8, RGB8UI, RGB8I | + --------------------------------------------------------------------------- + | VIEW_CLASS_16_BITS | R16F, RG8UI, R16UI, RG8I, R16I, RG8, R16, | + | | RG8_SNORM, R16_SNORM | + --------------------------------------------------------------------------- + | VIEW_CLASS_8_BITS | R8UI, R8I, R8, R8_SNORM | + --------------------------------------------------------------------------- + | VIEW_CLASS_RGTC1_RED | COMPRESSED_RED_RGTC1, | + | | COMPRESSED_SIGNED_RED_RGTC1 | + --------------------------------------------------------------------------- + | VIEW_CLASS_RGTC2_RG | COMPRESSED_RG_RGTC2, | + | | COMPRESSED_SIGNED_RG_RGTC2 | + --------------------------------------------------------------------------- + | VIEW_CLASS_BPTC_UNORM | COMPRESSED_RGBA_BPTC_UNORM, | + | | COMPRESSED_SRGB_ALPHA_BPTC_UNORM | + --------------------------------------------------------------------------- + | VIEW_CLASS_BPTC_FLOAT | COMPRESSED_RGB_BPTC_SIGNED_FLOAT, | + | | COMPRESSED_RGB_BPTC_UNSIGNED_FLOAT | + --------------------------------------------------------------------------- + */ +struct internal_format_class_info { + GLenum view_class; + GLenum internal_format; +}; +#define INFO(c,f) {GL_##c, GL_##f} +static const struct internal_format_class_info _compatible_internal_formats[] = { + INFO(VIEW_CLASS_128_BITS, RGBA32F), </blockquote> </div></div> If the point of the macro is just to save typing GL_ prefixes, I don't think it's worth it. </blockquote><div>I mostly did it so that I could copy & paste the values from the spec to here.</div><div> </div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> The leading underscore on _compatible_internal_formats isn't really necessary.</blockquote><div>underscore removed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div><div class="h5"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> + INFO(VIEW_CLASS_128_BITS, RGBA32UI), + INFO(VIEW_CLASS_128_BITS, RGBA32I), + INFO(VIEW_CLASS_96_BITS, RGB32F), + INFO(VIEW_CLASS_96_BITS, RGB32UI), + INFO(VIEW_CLASS_96_BITS, RGB32I), + INFO(VIEW_CLASS_64_BITS, RGBA16F), + INFO(VIEW_CLASS_64_BITS, RG32F), + INFO(VIEW_CLASS_64_BITS, RGBA16UI), + INFO(VIEW_CLASS_64_BITS, RG32UI), + INFO(VIEW_CLASS_64_BITS, RGBA16I), + INFO(VIEW_CLASS_64_BITS, RG32I), + INFO(VIEW_CLASS_64_BITS, RGBA16), + INFO(VIEW_CLASS_64_BITS, RGBA16_SNORM), + INFO(VIEW_CLASS_48_BITS, RGB16), + INFO(VIEW_CLASS_48_BITS, RGB16_SNORM), + INFO(VIEW_CLASS_48_BITS, RGB16F), + INFO(VIEW_CLASS_48_BITS, RGB16UI), + INFO(VIEW_CLASS_48_BITS, RGB16I), + INFO(VIEW_CLASS_32_BITS, RG16F), + INFO(VIEW_CLASS_32_BITS, R11F_G11F_B10F), + INFO(VIEW_CLASS_32_BITS, R32F), + INFO(VIEW_CLASS_32_BITS, RGB10_A2UI), + INFO(VIEW_CLASS_32_BITS, RGBA8UI), + INFO(VIEW_CLASS_32_BITS, RG16UI), + INFO(VIEW_CLASS_32_BITS, R32UI), + INFO(VIEW_CLASS_32_BITS, RGBA8I), + INFO(VIEW_CLASS_32_BITS, RG16I), + INFO(VIEW_CLASS_32_BITS, R32I), + INFO(VIEW_CLASS_32_BITS, RGB10_A2), + INFO(VIEW_CLASS_32_BITS, RGBA8), + INFO(VIEW_CLASS_32_BITS, RG16), + INFO(VIEW_CLASS_32_BITS, RGBA8_SNORM), + INFO(VIEW_CLASS_32_BITS, RG16_SNORM), + INFO(VIEW_CLASS_32_BITS, SRGB8_ALPHA8), + INFO(VIEW_CLASS_32_BITS, RGB9_E5), + INFO(VIEW_CLASS_24_BITS, RGB8), + INFO(VIEW_CLASS_24_BITS, RGB8_SNORM), + INFO(VIEW_CLASS_24_BITS, SRGB8), + INFO(VIEW_CLASS_24_BITS, RGB8UI), + INFO(VIEW_CLASS_24_BITS, RGB8I), + INFO(VIEW_CLASS_16_BITS, R16F), + INFO(VIEW_CLASS_16_BITS, RG8UI), + INFO(VIEW_CLASS_16_BITS, R16UI), + INFO(VIEW_CLASS_16_BITS, RG8I), + INFO(VIEW_CLASS_16_BITS, R16I), + INFO(VIEW_CLASS_16_BITS, RG8), + INFO(VIEW_CLASS_16_BITS, R16), + INFO(VIEW_CLASS_16_BITS, RG8_SNORM), + INFO(VIEW_CLASS_16_BITS, R16_SNORM), + INFO(VIEW_CLASS_8_BITS, R8UI), + INFO(VIEW_CLASS_8_BITS, R8I), + INFO(VIEW_CLASS_8_BITS, R8), + INFO(VIEW_CLASS_8_BITS, R8_SNORM), + INFO(VIEW_CLASS_RGTC1_RED, COMPRESSED_RED_RGTC1), + INFO(VIEW_CLASS_RGTC1_RED, COMPRESSED_SIGNED_RED_RGTC1), + INFO(VIEW_CLASS_RGTC2_RG, COMPRESSED_RG_RGTC2), + INFO(VIEW_CLASS_RGTC2_RG, COMPRESSED_SIGNED_RG_RGTC2), + INFO(VIEW_CLASS_BPTC_UNORM, COMPRESSED_RGBA_BPTC_UNORM_ARB), + INFO(VIEW_CLASS_BPTC_UNORM, COMPRESSED_SRGB_ALPHA_BPTC_UNORM_ARB), + INFO(VIEW_CLASS_BPTC_FLOAT, COMPRESSED_RGB_BPTC_SIGNED_FLOAT_ARB), + INFO(VIEW_CLASS_BPTC_FLOAT, COMPRESSED_RGB_BPTC_UNSIGNED_FLOAT_ARB), +}; +#undef INFO + +/** + * Lookup format view class based on internalformat + * \return VIEW_CLASS if internalformat found in table, GL_FALSE otherwise. + */ +static unsigned int </blockquote> </div></div> Return GLenum?</blockquote><div>I had feedback elsewhere that it was better to stay away from GLenums for internal functions.</div><div>So I could either change the comment to match the return of false. Hmm, as I look at this again the function is going to either return a GL_VIEW_CLASS... enum from the lookup table or false. Yeah, probably make more sense to just be GLenum for consistency.</div> <div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> +lookup_view_class(GLenum internalformat) +{ + uint i; + + for (i = 0; i < ARRAY_SIZE(_compatible_internal_formats); i++) { + if (_compatible_internal_formats[i].internal_format == internalformat) + return _compatible_internal_formats[i].view_class; + } + return false; </blockquote> </div> return GL_FALSE, per the comment.</blockquote><div>got it. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div class="im"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> +} + +/** + * Compute the size of the next mipmap level. + */ +static void +next_mipmap_level_size(GLenum target, + GLint *width, GLint *height, GLint *depth) +{ + if (*width > 1) { + *width /= 2; + } + + if ((*height > 1) && (target != GL_TEXTURE_1D_ARRAY)) { + *height /= 2; + } + + if ((*depth > 1) && (target != GL_TEXTURE_2D_ARRAY)) { + *depth /= 2; + } +} </blockquote> </div> Can the next_mipmap_level_size() helper in mipmap.c be re-used? And yes, we should probably re-use that function in texstorage.c</blockquote><div>Sure, that will work. So will one in texstorage.c.</div><div>Okay, I'll make both texstorage.c and textureview.c use the mipmap function.</div> <div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="h5"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> + +/** Helper to get a particular texture image in a texture object */ +static struct gl_texture_image * +get_tex_image(struct gl_context *ctx, + struct gl_texture_object *texObj, + GLuint face, GLuint level) +{ + struct gl_texture_image *texImage; + + if (!texObj) + return NULL; + + texImage = texObj->Image[face][level]; + if (!texImage) { + texImage = ctx->Driver.NewTextureImage(ctx); + if (!texImage) { + _mesa_error(ctx, GL_OUT_OF_MEMORY, "texture image allocation"); + return NULL; + } + + texObj->Image[face][level] = texImage; + + /* Set the 'back' pointer */ + texImage->TexObject = texObj; + texImage->Level = level; + texImage->Face = face; + } + + return texImage; +} </blockquote> </div></div> That function looks a lot like _mesa_get_tex_image(), or get_tex_image() in texstorage.c.</blockquote><div>Hadn't seen that one. _mesa_get_tex_image() does the same thing.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div><div class="h5"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> + +/** + * Initialize new texture's gl_texture_image structures. Will not call driver + * to allocate new space, simply record relevant layer, face, format, etc. + * \return GL_FALSE if any error, GL_TRUE otherwise. + */ +static GLboolean +initialize_texture_fields(struct gl_context *ctx, + GLenum target, + struct gl_texture_object *texObj, + GLint levels, + GLsizei width, GLsizei height, GLsizei depth, + GLenum internalFormat, gl_format texFormat) +{ + const GLuint numFaces = _mesa_num_tex_faces(target); + GLint level, levelWidth = width, levelHeight = height, levelDepth = depth; + GLuint face; + + /* Pretend we are bound to initialize the gl_texture_image structs */ + texObj->Target = target; + + /* Set up all the texture object's gl_texture_images */ + for (level = 0; level < levels; level++) { + for (face = 0; face < numFaces; face++) { + struct gl_texture_image *texImage = + get_tex_image(ctx, texObj, face, level); + + if (!texImage) { + _mesa_error(ctx, GL_OUT_OF_MEMORY, "glTexStorage"); + return GL_FALSE; + } + + _mesa_init_teximage_fields(ctx, texImage, + levelWidth, levelHeight, levelDepth, + 0, internalFormat, texFormat); + } + + next_mipmap_level_size(target, &levelWidth, &levelHeight, &levelDepth); + } + + /* "unbind" */ + texObj->Target = 0; + + return GL_TRUE; +} + +#define RETURN_IF_SUPPORTED(t) do { \ + if (newTarget == GL_ ## t) \ + return GL_TRUE; \ +} while (0) + +/** + * Check for compatible target + * If an error is found, record it with _mesa_error() + * \return GL_FALSE if any error, GL_TRUE otherwise. + */ +static GLboolean +target_valid(struct gl_context *ctx, GLenum origTarget, GLenum newTarget) +{ + /* + * From ARB_texture_view spec: + --------------------------------------------------------------------------------------------------------- + | Original target | Valid new targets | + --------------------------------------------------------------------------------------------------------- + | TEXTURE_1D | TEXTURE_1D, TEXTURE_1D_ARRAY | + | ------------------------------------------------------------------------------------------------------- | + | TEXTURE_2D | TEXTURE_2D, TEXTURE_2D_ARRAY | + | ------------------------------------------------------------------------------------------------------- | + | TEXTURE_3D | TEXTURE_3D | + | ------------------------------------------------------------------------------------------------------- | + | TEXTURE_CUBE_MAP | TEXTURE_CUBE_MAP, TEXTURE_2D, TEXTURE_2D_ARRAY, TEXTURE_CUBE_MAP_ARRAY | + | ------------------------------------------------------------------------------------------------------- | + | TEXTURE_RECTANGLE | TEXTURE_RECTANGLE | + | ------------------------------------------------------------------------------------------------------- | + | TEXTURE_BUFFER | <none> | + | ------------------------------------------------------------------------------------------------------- | + | TEXTURE_1D_ARRAY | TEXTURE_1D_ARRAY, TEXTURE_1D | + | ------------------------------------------------------------------------------------------------------- | + | TEXTURE_2D_ARRAY | TEXTURE_2D_ARRAY, TEXTURE_2D, TEXTURE_CUBE_MAP, TEXTURE_CUBE_MAP_ARRAY | + | ------------------------------------------------------------------------------------------------------- | + | TEXTURE_CUBE_MAP_ARRAY | TEXTURE_CUBE_MAP_ARRAY, TEXTURE_2D_ARRAY, TEXTURE_2D, TEXTURE_CUBE_MAP | + | ------------------------------------------------------------------------------------------------------- | + | TEXTURE_2D_MULTISAMPLE | TEXTURE_2D_MULTISAMPLE, TEXTURE_2D_MULTISAMPLE_ARRAY | + | ------------------------------------------------------------------------------------------------------- | + | TEXTURE_2D_MULTISAMPLE_ARRAY | TEXTURE_2D_MULTISAMPLE, TEXTURE_2D_MULTISAMPLE_ARRAY | + --------------------------------------------------------------------------------------------------------- + */ + + switch (origTarget) { + case GL_TEXTURE_1D: + case GL_TEXTURE_1D_ARRAY: + RETURN_IF_SUPPORTED(TEXTURE_1D); + RETURN_IF_SUPPORTED(TEXTURE_1D_ARRAY); + break; + case GL_TEXTURE_2D: + RETURN_IF_SUPPORTED(TEXTURE_2D); + RETURN_IF_SUPPORTED(TEXTURE_2D_ARRAY); + break; + case GL_TEXTURE_3D: + RETURN_IF_SUPPORTED(TEXTURE_3D); + break; + case GL_TEXTURE_RECTANGLE: + RETURN_IF_SUPPORTED(TEXTURE_RECTANGLE); + break; + case GL_TEXTURE_CUBE_MAP: + case GL_TEXTURE_2D_ARRAY: + case GL_TEXTURE_CUBE_MAP_ARRAY: + RETURN_IF_SUPPORTED(TEXTURE_2D); + RETURN_IF_SUPPORTED(TEXTURE_2D_ARRAY); + RETURN_IF_SUPPORTED(TEXTURE_CUBE_MAP); + RETURN_IF_SUPPORTED(TEXTURE_CUBE_MAP_ARRAY); + break; + case GL_TEXTURE_2D_MULTISAMPLE: + case GL_TEXTURE_2D_MULTISAMPLE_ARRAY: + RETURN_IF_SUPPORTED(TEXTURE_2D_MULTISAMPLE); + RETURN_IF_SUPPORTED(TEXTURE_2D_MULTISAMPLE_ARRAY); + break; + } + _mesa_error(ctx, GL_INVALID_OPERATION, + "glTextureView(illegal target=%s)", + _mesa_lookup_enum_by_nr(newTarget)); + return GL_FALSE; </blockquote> </div></div> Typically, we put a _mesa_error() call like that after a default switch case.</blockquote><div> </div><div>I modeled this function after _mesa_choose_tex_format which uses the same pattern. Default case will not catch all the error conditions.</div> <div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> +} +#undef RETURN_IF_SUPPORTED + +/** + * Check for compatible format + * If an error is found, record it with _mesa_error() + * \return GL_FALSE if any error, GL_TRUE otherwise. + */ +static GLboolean </blockquote> </div> Since this is an internal function, it could return bool.</blockquote><div>OK </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div class="im"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> +compatible_format(struct gl_context *ctx, struct gl_texture_object *origTexObj, </blockquote> </div> origTexObj could be const qualified.</blockquote><div>OK </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div class="im"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> + GLenum internalformat) +{ + /* Level 0 of a texture created by glTextureStorage or glTextureView + * is always defined. + */ + struct gl_texture_image *texImage = origTexObj->Image[0][0]; + GLint origInternalFormat = texImage->InternalFormat; + unsigned int origViewClass, newViewClass; + + + /* The two textures' internal formats must be compatible according to + * Table 3.X.2 (Compatible internal formats for TextureView) + * if the internal format exists in that table the view class must match. + * The internal formats must be identical if not in that table, + * or an INVALID_OPERATION error is generated. + */ + if (origInternalFormat == internalformat) + return GL_TRUE; + + origViewClass = lookup_view_class(origInternalFormat); + newViewClass = lookup_view_class(internalformat); + if ((origViewClass == newViewClass) && origViewClass != false) </blockquote> </div> != GL_FALSE</blockquote><div>Another internal function, I'll change it to bool.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div class="im"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> + return GL_TRUE; + + _mesa_error(ctx, GL_INVALID_OPERATION, + "glTextureView(internalformat %s not compatible with origtexture %s)", </blockquote> </div> Extra space before first %s<div><div class="h5"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> + _mesa_lookup_enum_by_nr(internalformat), + _mesa_lookup_enum_by_nr(origInternalFormat)); + return GL_FALSE; +} + /** * glTextureView (ARB_texture_view) * If an error is found, record it with _mesa_error() @@ -53,13 +391,235 @@ _mesa_TextureView(GLuint texture, GLenum target, GLuint origtexture, GLuint minlevel, GLuint numlevels, GLuint minlayer, GLuint numlayers) { + struct gl_texture_object *texObj; + struct gl_texture_object *origTexObj; + struct gl_texture_image *origTexImage; + GLuint newViewMinLevel, newViewMinLayer; + GLuint newViewNumLevels, newViewNumLayers; + GLsizei width, height, depth; + gl_format texFormat; + GLboolean sizeOK, dimensionsOK; + GLenum faceTarget; + GET_CURRENT_CONTEXT(ctx); if (MESA_VERBOSE & (VERBOSE_API | VERBOSE_TEXTURE)) - _mesa_debug(ctx, "glTextureView (unfinished) %d %s %d %s %d %d %d %d\n", + _mesa_debug(ctx, "glTextureView %d %s %d %s %d %d %d %d\n", texture, _mesa_lookup_enum_by_nr(target), origtexture, _mesa_lookup_enum_by_nr(internalformat), minlevel, numlevels, minlayer, numlayers); + if (origtexture == 0) { + _mesa_error(ctx, GL_INVALID_VALUE, "glTextureView(origtexture = 0x%x)", origtexture); + return; + } + + /* Need original texture information to validate arguments */ + origTexObj = _mesa_lookup_texture(ctx, origtexture); + + /* If <origtexture> is not the name of a texture, INVALID_VALUE is generated. */ + if (!origTexObj) { + _mesa_error(ctx, GL_INVALID_VALUE, "glTextureView(origtexture = 0x%x)", origtexture); </blockquote> </div></div> I think we usually print/record object IDs as decimal.</blockquote><div>OK </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div><div class="h5"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> + return; + } + + /* If <origtexture>'s TEXTURE_IMMUTABLE_FORMAT value is not TRUE, + * INVALID_OPERATION is generated. + */ + if (!origTexObj->Immutable) { + _mesa_error(ctx, GL_INVALID_OPERATION, "glTextureView(origtexture not immutable)"); + return; + } + + /* If <texture> is 0, INVALID_VALUE is generated. */ + if (texture == 0) { + _mesa_error(ctx, GL_INVALID_VALUE, "glTextureView(texture = 0)"); + return; + } + + /* If <texture> is not a valid name returned by GenTextures, + * the error INVALID_OPERATION is generated. + */ + texObj = _mesa_lookup_texture(ctx, texture); + if (texObj == NULL) { + _mesa_error(ctx, GL_INVALID_OPERATION, "glTextureView(texture 0x%x non-gen name)", texture); + return; + } + + /* If <texture> has already been bound and given a target, then + * the error INVALID_OPERATION is generated. + */ + if (texObj->Target) { + _mesa_error(ctx, GL_INVALID_OPERATION, "glTextureView(texture 0x%x already bound)", texture); + return; + } + + /* Check for compatible target */ + if (!target_valid(ctx, origTexObj->Target, target)) { + return; /* error was recorded */ + } + + /* minlevel and minlayer are relative to the view of origtexture */ + /* If minlevel or minlayer is greater than level or layer, respectively, + * of origtexture return INVALID_VALUE. + */ + newViewMinLevel = origTexObj->MinLevel + minlevel; + newViewMinLayer = origTexObj->MinLayer + minlayer; + if (newViewMinLevel >= (origTexObj->MinLevel + origTexObj->NumLevels) || + newViewMinLayer >= (origTexObj->MinLayer + origTexObj->NumLayers)) { + _mesa_error(ctx, GL_INVALID_VALUE, "glTextureView(origtexture = 0x%x)", origtexture); + return; + } </blockquote> </div></div> I'd probably split the level and layer error checks into two separate if statements. Then, the error message could have more detailed info about the invalid layer or level.<div class="im"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> + if (!compatible_format(ctx, origTexObj, internalformat)) { + return; // Error logged </blockquote> </div> /* error logged */ just to be consistent with other comments.</blockquote><div>OK</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div class="im"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> + } + + texFormat = _mesa_choose_texture_format(ctx, texObj, target, 0, + internalformat, GL_NONE, GL_NONE); + assert(texFormat != MESA_FORMAT_NONE); </blockquote> </div> Just to be safe, let's also put in a "if (texFormat == MESA_FORMAT_NONE) return" here for release builds.</blockquote><div>OK </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div class="im"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> + + newViewNumLevels = MIN2(numlevels, origTexObj->NumLevels - minlevel); + newViewNumLayers = MIN2(numlayers, origTexObj->NumLayers - minlayer); + + faceTarget = origTexObj->Target; + if (faceTarget == GL_TEXTURE_CUBE_MAP) </blockquote> </div> or GL_TEXTURE_CUBE_MAP_ARRAY?</blockquote><div>That's a great question! I believe the answer is no. As I understand it, for a CUBE_MAP the numlayers parameter corresponds to the faces (a CUBE_MAP will always have numlayers = 6). For a CUBE_MAP_ARRAY the numlayers refers to the depth not the face.</div> <div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="h5"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> + faceTarget = GL_TEXTURE_CUBE_MAP_POSITIVE_X + minlayer; + + /* Get a reference to what will become this View's base level */ + origTexImage = _mesa_select_tex_image(ctx, origTexObj, + faceTarget, minlevel); + width = origTexImage->Width; + height = origTexImage->Height; + depth = origTexImage->Depth; + + /* Adjust width, height, depth to be appropriate for new target */ + switch (target) { + case GL_TEXTURE_1D: + case GL_TEXTURE_3D: + break; + + case GL_TEXTURE_1D_ARRAY: + height = (GLsizei) newViewNumLayers; + break; + + case GL_TEXTURE_2D: + case GL_TEXTURE_2D_MULTISAMPLE: + case GL_TEXTURE_RECTANGLE: + case GL_TEXTURE_CUBE_MAP: + depth = 1; + break; + + case GL_TEXTURE_2D_ARRAY: + case GL_TEXTURE_CUBE_MAP_ARRAY: + case GL_TEXTURE_2D_MULTISAMPLE_ARRAY: + depth = newViewNumLayers; + break; + } + + /* If the dimensions of the original texture are larger than the maximum + * supported dimensions of the new target, the error INVALID_OPERATION is + * generated. For example, if the original texture has a TEXTURE_2D_ARRAY + * target and its width is greater than MAX_CUBE_MAP_TEXTURE_SIZE, an error + * will be generated if TextureView is called to create a TEXTURE_CUBE_MAP + * view. + */ + dimensionsOK = _mesa_legal_texture_dimensions(ctx, target, 0, + width, height, depth, 0); + if (!dimensionsOK) { + _mesa_error(ctx, GL_INVALID_OPERATION, "glTextureView(invalid width or height or depth)"); + return; + } + + sizeOK = ctx->Driver.TestProxyTexImage(ctx, target, 0, texFormat, + width, height, depth, 0); + if (!sizeOK) { + _mesa_error(ctx, GL_INVALID_OPERATION, "glTextureView(invalid texture size)"); + return; + } + + /* If <target> is TEXTURE_1D, TEXTURE_2D, TEXTURE_3D, TEXTURE_RECTANGLE, + * or TEXTURE_2D_MULTISAMPLE and <numlayers> does not equal 1, the error + * INVALID_VALUE is generated. + */ + switch (target) { + case GL_TEXTURE_1D: + case GL_TEXTURE_2D: + case GL_TEXTURE_3D: + case GL_TEXTURE_RECTANGLE: + case GL_TEXTURE_2D_MULTISAMPLE: + if (numlayers != 1) { + _mesa_error(ctx, GL_INVALID_VALUE, "glTextureView(numlayers %d != 1)", numlayers); + return; + } + break; + + case GL_TEXTURE_CUBE_MAP: + /* If the new texture's target is TEXTURE_CUBE_MAP, the clamped <numlayers> + * must be equal to 6. + */ + if (newViewNumLayers != 6) { + _mesa_error(ctx, GL_INVALID_VALUE, "glTextureView(clamped numlayers %d != 6)", + newViewNumLayers); + return; + } + break; + + case GL_TEXTURE_CUBE_MAP_ARRAY: + /* If the new texture's target is TEXTURE_CUBE_MAP_ARRAY, + * then <numlayers> counts layer-faces rather than layers, + * and the clamped <numlayers> must be a multiple of 6. + * Otherwise, the error INVALID_VALUE is generated. + */ + if ((newViewNumLayers % 6) != 0) { + _mesa_error(ctx, GL_INVALID_VALUE, "glTextureView(clamped numlayers %d is not a multiple of 6)", </blockquote> </div></div> If that line's longer than 78 chars, can you wrap it?</blockquote><div>OK</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div><div class="h5"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> + newViewNumLayers); + return; + } + break; + } + + /* If the new texture's target is TEXTURE_CUBE_MAP or + * TEXTURE_CUBE_MAP_ARRAY, the width and height of the original texture's + * levels must be equal otherwise the error INVALID_OPERATION is generated. + */ + if ((target == GL_TEXTURE_CUBE_MAP || target == GL_TEXTURE_CUBE_MAP_ARRAY) && + (origTexImage->Width != origTexImage->Height)) { + _mesa_error(ctx, GL_INVALID_OPERATION, "glTextureView(origtexture width (%d) != height (%d))", + origTexImage->Width, origTexImage->Height); + return; + } + + /* When the original texture's target is TEXTURE_CUBE_MAP, the layer + * parameters are interpreted in the same order as if it were a + * TEXTURE_CUBE_MAP_ARRAY with 6 layer-faces. + */ + + /* If the internal format does not exactly match the internal format of the + * original texture, the contents of the memory are reinterpreted in the + * same manner as for image bindings described in + * section 3.9.20 (Texture Image Loads and Stores). + */ + + /* TEXTURE_BASE_LEVEL and TEXTURE_MAX_LEVEL are interpreted + * relative to the view and not relative to the original data store. + */ + + if (!initialize_texture_fields(ctx, target, texObj, newViewNumLevels, + width, height, depth, + internalformat, texFormat)) { + return; /* Already recorded error */ + } + + texObj->MinLevel = newViewMinLevel; + texObj->MinLayer = newViewMinLayer; + texObj->NumLevels = newViewNumLevels; + texObj->NumLayers = newViewNumLayers; + texObj->Immutable = GL_TRUE; + texObj->ImmutableLevels = origTexObj->ImmutableLevels; + texObj->Target = target; </blockquote> </div></div> Maybe these assignments should be rolled into initialize_texture_fields()?</blockquote><div>Why?</div><div> </div><div>initialize_texture_fields is initializing the teximage properties of the texture whereas the assignments are setting the texture object's properties.</div> <div> </div><div>Thanks for the feedback!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div class="im"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> + if (!ctx->Driver.TextureView(ctx, texObj, origTexObj)) { + return; /* driver recorded error */ + } } </blockquote> </div> _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> <div> </div>-- <div dir="ltr">Courtney Goeltzenleuchter <div>LunarG</div><div><img src="http://media.lunarg.com/wp-content/themes/LunarG/images/logo.png" width="96" height="65"> </div></div> </div></div>