On 08/05/18 14:33, Antia Puentes wrote: > On 06/05/18 11:42, Alejandro Piñeiro wrote: > >> Until right now we were using glTexImage*D for any compressed >> internalformat. That would only work always if the driver support >> online compression for specific compressed internalformats. > s/support/supports >> As it is not really easy to check which internalformats the driver >> support online compression, it is safer to use glCompressedTexImage*D > Again s/support/supports. It sounds better to me rephrasing it as > "easy to check which internalformats the driver supports online > compression for" > >> for any specific internalformat, keeping glTexImage*D for the generic >> ones. >> --- >> tests/spec/arb_internalformat_query2/common.c | 126 >> ++++++++++++++++++++++++-- >> 1 file changed, 117 insertions(+), 9 deletions(-) >> >> diff --git a/tests/spec/arb_internalformat_query2/common.c >> b/tests/spec/arb_internalformat_query2/common.c >> index 68f33a4a0..204149936 100644 >> --- a/tests/spec/arb_internalformat_query2/common.c >> +++ b/tests/spec/arb_internalformat_query2/common.c >> @@ -186,6 +186,47 @@ test_data_check_supported(const test_data *data, >> return result; >> } >> + >> +/* >> + * Spec 4.6, Table 8.14 >> + */ >> +static const GLenum specific_compressed_internalformats[] = { >> + GL_COMPRESSED_SIGNED_RED_RGTC1, >> + GL_COMPRESSED_RG_RGTC2, >> + GL_COMPRESSED_SIGNED_RG_RGTC2, >> + GL_COMPRESSED_RGBA_BPTC_UNORM, >> + GL_COMPRESSED_SRGB_ALPHA_BPTC_UNORM, >> + GL_COMPRESSED_RGB_BPTC_SIGNED_FLOAT, >> + GL_COMPRESSED_RGB_BPTC_UNSIGNED_FLOAT, >> + GL_COMPRESSED_RGB8_ETC2, >> + GL_COMPRESSED_SRGB8_ETC2, >> + GL_COMPRESSED_RGB8_PUNCHTHROUGH_ALPHA1_ETC2, >> + GL_COMPRESSED_SRGB8_PUNCHTHROUGH_ALPHA1_ETC2, >> + GL_COMPRESSED_RGBA8_ETC2_EAC, >> + GL_COMPRESSED_SRGB8_ALPHA8_ETC2_EAC, >> + GL_COMPRESSED_R11_EAC, >> + GL_COMPRESSED_SIGNED_R11_EAC, >> + GL_COMPRESSED_RG11_EAC, >> + GL_COMPRESSED_SIGNED_RG11_EAC, >> +}; > GL_COMPRESSED_RED_RGTC1 is missing from the list. > >> + >> +/* >> + * Returns if @internalformat is a specific compressed internalformat. >> + * >> + * This is needed because specific compressed internalformats that >> + * doesn't support online compression can't be used with glTexImage*D, >> + * and glCompressedTexImage*D should be used instead. In general, for >> + * specific compressed internalformats, it is better to use >> + * glCompressedTexImage*D >> + */ >> +static bool >> +internalformat_is_specific_compressed(const GLenum internalformat) >> +{ >> + return value_on_set((const GLint*) >> specific_compressed_internalformats, >> + >> ARRAY_SIZE(specific_compressed_internalformats), >> + (GLint) internalformat); >> +} >> + >> /* Returns if @value is one of the values among @set */ >> bool >> value_on_set(const GLint *set, >> @@ -336,8 +377,8 @@ try_basic(const GLenum *targets, unsigned >> num_targets, >> } >> /* Returns a valid format for @internalformat, so it would be >> possible >> - * to create a texture using glTexImageXD with that >> - * format/internalformat combination */ >> + * to create a texture using glTexImageXD/glCompressedTexImageXD with >> + * that format/internalformat combination */ >> static GLenum >> format_for_internalformat(const GLenum internalformat) >> { >> @@ -432,6 +473,51 @@ create_texture(const GLenum target, >> int width = 16; >> int depth = 16; >> unsigned i; >> + bool is_specific_compressed = >> internalformat_is_specific_compressed(internalformat); >> + int image_size = 0; >> + >> + /* >> + * From OpenGL 4.6 spec, Section 8.5, Texture Image >> + * Specification: >> + * >> + * "An INVALID_ENUM error is generated by >> + * CompressedTexImage1D if in- ternalformat is one of the >> + * specific compressed formats. OpenGL defines no specific >> + * one-dimensional compressed formats, but such formats >> + * may be pro- vided by extensions." >> + * > s/in- ternalformat/internalformat > s/pro- vided/provided > >> + * From OpenGL 4.6 spec, Section 8.7, Compressed Texture >> + * Images: >> + * >> + * "An INVALID_ENUM error is generated if the target >> + * parameter to any of the CompressedTexImagenD commands >> + * is TEXTURE_RECTANGLE or PROXY_TEXTURE_RECTANGLE ." >> + */ >> + if (is_specific_compressed && >> + (target == GL_TEXTURE_1D || target == >> GL_TEXTURE_RECTANGLE)) >> + return false; >> + >> + /* >> + * From ARB_texture_multisample: >> + * >> + * "(2) What commands may be used on multisample textures? >> + * >> + * RESOLVED: Multisample textures can be bound for >> + * rendering and texturing, but they cannot be loaded/read >> + * with SubImage commands (TexSubImage, CopyTexSubImage, >> + * GetTexImage), they don't support compressed formats, and >> + * they don't need TexParameters since they can only be >> + * fetched with texelFetchMultisample." >> + */ >> + if (is_specific_compressed && >> + (target == GL_TEXTURE_2D_MULTISAMPLE || >> + target == GL_TEXTURE_2D_MULTISAMPLE_ARRAY)) { >> + return false; >> + } >> + >> + if (is_specific_compressed) { >> + image_size = >> piglit_compressed_image_size(internalformat, width, height); >> + } > > You may want to extract the common condition (is_specific_compressed) > and put the remaining code inside an "if (is_specific_compressed)" block. > Anyway, if you prefer to leave it as it is and avoid another level of > nesting, that is also fine by me:
I found the current status easier to read. Avoiding another level of nesting is another advantage. So if you don't mind I will let it as it is. > > Reviewed-by: Antia Puentes <apuen...@igalia.com> Thanks! > >> glGenTextures(1, &tex); >> glBindTexture(target, tex); >> @@ -444,14 +530,29 @@ create_texture(const GLenum target, >> case GL_TEXTURE_1D_ARRAY: >> case GL_TEXTURE_2D: >> case GL_TEXTURE_RECTANGLE: >> - glTexImage2D(target, 0, internalformat, width, >> height, 0, >> - format, type, NULL); >> + if (is_specific_compressed) { >> + glCompressedTexImage2D(target, 0, >> internalformat, >> + width, height, 0, >> + image_size, NULL); >> + } else { >> + glTexImage2D(target, 0, internalformat, >> + width, height, 0, >> + format, type, NULL); >> + } >> break; >> case GL_TEXTURE_CUBE_MAP: >> for (i = 0; i < 6; i++) { >> - glTexImage2D(GL_TEXTURE_CUBE_MAP_POSITIVE_X >> + i, 0, >> - internalformat, width, height, >> 0, format, type, >> - NULL); >> + if (is_specific_compressed) { >> + >> glCompressedTexImage2D(GL_TEXTURE_CUBE_MAP_POSITIVE_X + i, 0, >> + internalformat, >> + width, >> height, 0, >> + image_size, >> NULL); >> + } else { >> + >> glTexImage2D(GL_TEXTURE_CUBE_MAP_POSITIVE_X + i, 0, >> + internalformat, >> + width, height, 0, >> + format, type, NULL); >> + } >> } >> break; >> @@ -462,8 +563,15 @@ create_texture(const GLenum target, >> /* fall through */ >> case GL_TEXTURE_2D_ARRAY: >> case GL_TEXTURE_3D: >> - glTexImage3D(target, 0, internalformat, width, >> height, depth, 0, >> - format, type, NULL); >> + image_size = image_size * depth; >> + if (is_specific_compressed) { >> + glCompressedTexImage3D(target, 0, >> internalformat, >> + width, height, depth, 0, >> + image_size, NULL); >> + } else { >> + glTexImage3D(target, 0, internalformat, >> width, height, depth, 0, >> + format, type, NULL); >> + } >> break; >> case GL_TEXTURE_2D_MULTISAMPLE: >> glTexImage2DMultisample(target, 1, internalformat, width, >> height, > > _______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit