A good starting point for adding a new extension is OES_standard_derivatives: bug 684853. Except that we're no longer adding private IIDs to concrete classes. You'll have to check that the client supports these formats. glGetIntegerv with NUM_COMPRESSED_TEXTURE_FORMATS and COMPRESSED_TEXTURE_FORMATS should give you that. Then in compressedTexImage2D you'll have to check that the typed array's length is large enough for the given width, height, format. A formula is given in the spec, for each format. For that, you need to use CheckedInt, see usage of it in the same file. The unit test might complain that properties added to the JS object returned by getExtension, are not correctly preserved if you get again this object by calling again getExtension (the "unique object test"). In that case you'll have to temporarily comment out this part of the conformance test, adding a .patch file, until we fix this issue (soon, anyway).

It was raise on an email thread with Benoit that we have some games and samples that are currently using this in Chrome (its WEBKIT_WEBGL_compressed_texture_s3tc) so if you want some tests cases we can update them when this is exposed (MOZ_WEBGL_compressed_texture_s3tc?). I saw the general note on https://wiki.mozilla.org/Platform/GFX/WebGL/Contribute/Extensions about testing WebGL extensions we can update our samples to use other extension when available.

This feels nearly ready to go. It passes locally, and here's a try server build for other platforms: https://tbpl.mozilla.org/?tree=Try&rev=460091a5ecb1

Comment on attachment 618559 [details] [diff] [review] s3tc extension v1 Review of attachment 618559 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLContextGL.cpp @@ +2259,5 @@ > wrval->SetAsInt32(0); > break; > case LOCAL_GL_COMPRESSED_TEXTURE_FORMATS: > + { > + int i = mCompressedTextureFormats.Length(); PRUint32 length @@ +2272,1 @@ > break; break inside the {} @@ +4831,5 @@ > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT1_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT3_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT5_EXT: > + { > + if (xoffset % 4 != 0 || yoffset % 4 != 0 || !(width % 4 == 0 || width == imageInfo.Width()) || !(height % 4 == 0 || height == imageInfo.Height())) { This condition is pretty unreadable, IMO @@ +5281,5 @@ > if (!IsContextStable()) > return NS_OK; > > if (pixels && !JS_IsTypedArrayObject(pixels, cx)) > + return ErrorInvalidValue("texImage2D: pixels are wrong type!"); {} while you're here ::: content/canvas/src/WebGLContextValidate.cpp @@ +370,5 @@ > > return true; > } > > +bool WebGLContext::ValidateTexImage2DTarget(WebGLenum target, WebGLsizei width, WebGLsizei height, const char *info) * on the left, and wrap to 80 characters ::: content/canvas/src/WebGLExtensionCompressedTextureS3TC.cpp @@ +7,5 @@ > + > +using namespace mozilla; > + > +WebGLExtensionCompressedTextureS3TC::WebGLExtensionCompressedTextureS3TC(WebGLContext* context) : > + WebGLExtension(context) : on the left on the next line

Comment on attachment 618559 [details] [diff] [review] s3tc extension v1 Review of attachment 618559 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLContextGL.cpp @@ +4824,5 @@ > + return NS_OK; > + } > + > + size_t face = WebGLTexture::FaceForTarget(target); > + const WebGLTexture::ImageInfo &imageInfo = tex->ImageInfoAt(level, face); Two problems here. 1. the |face| parameter has not been validated yet 2. must check that there is image info for this level and face. You can call HasImageInfoAt(level, face) to check if there is image info for this level and face. This should be no different than what is being done in TexSubImage2D. @@ +4831,5 @@ > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT1_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT3_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT5_EXT: > + { > + if (xoffset % 4 != 0 || yoffset % 4 != 0 || !(width % 4 == 0 || width == imageInfo.Width()) || !(height % 4 == 0 || height == imageInfo.Height())) { Unreadable indeed! I suggest introducing temporary boolean variables with explicit names. @@ +5363,5 @@ > default: > return ErrorInvalidEnumInfo("texSubImage2D: target", target); > } > > + if (!ValidateLevelWidthHeightBorderForTarget(target, level, width, height, "texImage2D")) { "texSubImage2D" ::: content/canvas/src/WebGLContextValidate.cpp @@ +400,5 @@ > + switch (format) { > + case LOCAL_GL_COMPRESSED_RGB_S3TC_DXT1_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT1_EXT: > + { > + if (byteLength != ((width + 3) / 4) * ((height + 3) / 4) * 8) { Here you really must use CheckedInt, to check for integer overflow. @@ +435,5 @@ > + > + return true; > +} > + > +bool WebGLContext::ValidateLevelWidthHeightBorderForTarget(WebGLenum target, WebGLint level, WebGLsizei width, WebGLsizei height, const char *info) This function has 'border' in its name but doesn't take a border argument. @@ +481,5 @@ > + case LOCAL_GL_COMPRESSED_RGB_S3TC_DXT1_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT1_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT3_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT5_EXT: > + return 1; This is actually a pretty big FIXME as it means that at least about:memory reporting will be wrong. Can you fix it? Check that only about:memory is concerned; if yes, returning a float is indeed the right approach in general. Or return an integer number of bits: indeed, all the existing texture formats i know have an integer bitrate.

Returning an int number of bits is probably preferable to a float number of bytes.

This fixes all of the comments noted in the above reviews except for memory reporting. Hopefully this try server run actually builds: https://tbpl.mozilla.org/?tree=Try&rev=ec79c6bb5211

Comment on attachment 621500 [details] [diff] [review] s3tc extension v2 Review of attachment 621500 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, we need another round of reviewing. In particular, the validation of xoffset/yoffset/width/height doesn't seem quite right, and CheckedInt isn't used properly yet. Some more nits. ::: content/canvas/src/WebGLContext.cpp @@ +885,5 @@ > case WebGL_MOZ_WEBGL_lose_context: > // We always support this extension. > isSupported = true; > break; > + case WebGL_MOZ_WEBGL_compressed_texture_s3tc: MOZ_ is only needed in the extension string name, not in any c++ identifier. Nevermind the bad example set by MOZ_WEBGL_lose_context above. @@ +950,5 @@ > mEnabledExtensions[ei] = new WebGLExtensionLoseContext(this); > break; > + case WebGL_MOZ_WEBGL_compressed_texture_s3tc: > + mEnabledExtensions[ei] = new WebGLExtensionCompressedTextureS3TC(this); > + //XXX Move these to the s3tc constructor maybe? Yes, that would be a good idea, and friend this class in WebGLContext. ::: content/canvas/src/WebGLContext.h @@ +1158,5 @@ > WebGL_OES_texture_float, > WebGL_OES_standard_derivatives, > WebGL_EXT_texture_filter_anisotropic, > WebGL_MOZ_WEBGL_lose_context, > + WebGL_MOZ_WEBGL_compressed_texture_s3tc, MOZ_ is only needed in the extension string name, not in any c++ identifier. Nevermind the bad example set by MOZ_WEBGL_lose_context above. @@ +1196,5 @@ > + bool ValidateTexImage2DTarget(WebGLenum target, WebGLsizei width, WebGLsizei height, const char* info); > + bool ValidateCompressedTextureSize(WebGLint level, WebGLenum format, WebGLsizei width, WebGLsizei height, uint32_t byteLength, const char* info); > + bool ValidateLevelWidthHeightForTarget(WebGLenum target, WebGLint level, WebGLsizei width, WebGLsizei height, const char* info); > + > + static PRUint64 GetTexelSize(WebGLenum format, WebGLenum type); why would this need to return a PRUint64? ::: content/canvas/src/WebGLContextGL.cpp @@ +5362,5 @@ > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT1_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT3_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT5_EXT: > + { > + if (xoffset <= 0 || xoffset % 4 != 0) { xoffset == 0 should definitely be allowed! xoffset < 0 is the right condition here. @@ +5365,5 @@ > + { > + if (xoffset <= 0 || xoffset % 4 != 0) { > + ErrorInvalidOperation("compressedTexSubImage2D: xoffset is not a multiple of 4"); > + return; > + } else if (yoffset <= 0 || yoffset % 4 != 0) { 'else' is not needed after a 'return'; standard moz coding practice is to omit 'else' in this case, instead just do a separate if() statement. Also, yoffset == 0 is allowed! @@ +5368,5 @@ > + return; > + } else if (yoffset <= 0 || yoffset % 4 != 0) { > + ErrorInvalidOperation("compressedTexSubImage2D: yoffset is not a multiple of 4"); > + return; > + } else if (width % 4 != 0 || width != imageInfo.Width()) { This forbids updating less than the existing image width. I think that &&, not ||, is wanted here. @@ +5373,5 @@ > + ErrorInvalidOperation("compressedTexSubImage2D: width is not a multiple of 4 or equal to texture width"); > + return; > + } else if (height % 4 != 0 || height != imageInfo.Height()) { > + ErrorInvalidOperation("compressedTexSubImage2D: height is not a multiple of 4 or equal to texture height"); > + return; Another remark: according to http://www.khronos.org/opengles/sdk/docs/man/xhtml/glCompressedTexSubImage2D.xml We also should generate INVALID_VALUE if xoffset + width > imageInfo.Width() and same for height. ::: content/canvas/src/WebGLContextValidate.cpp @@ +409,5 @@ > + switch (format) { > + case LOCAL_GL_COMPRESSED_RGB_S3TC_DXT1_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT1_EXT: > + { > + calculated_byteLength = ((width + 3) / 4) * ((height + 3) / 4) * 8; While calculated_byteLength is a CheckedInt, the RHS expression here involves only regular non-checked integers, so it will be evaluated using regular non-checked integer arithmetic before CheckedInt comes into play. But this would work: calculated_byteLength = ((CheckedUint32(width) + 3) / 4) * ((height + 3) / 4) * 8; You may prefer also replacing height by CheckedUint32(height), but that is purely cosmetic as the operators here have left-to-right associativity, and operations mixing CheckedInt and regular ints are checked. Up to you. @@ +419,5 @@ > + } > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT3_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT5_EXT: > + { > + calculated_byteLength = ((width + 3) / 4) * ((height + 3) / 4) * 16; Same. @@ +436,5 @@ > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT5_EXT: > + { > + if (!((level == 0 && width % 4 == 0 && height % 4 == 0) || > + (level > 0 && (width == 0 || width == 1 || width == 2 || width % 4 == 0) && > + (height == 0 || height == 1 || height == 2 || height % 4 == 0)))) { This if() condition is still unreadable, please split. @@ +493,5 @@ > + case LOCAL_GL_COMPRESSED_RGB_S3TC_DXT1_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT1_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT3_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT5_EXT: > + return 1; Still returning 1? I don't understand why the return type of that function was changed to PRUint64. Let's keep it PRUint32, and return number of bits. So change that function's name to GetBitsPerTexel, multiply existing non-compressed format values by 8, return 8 for DXT3 and DXT5, and return 4 for DXT1. (Per the extension spec). Search for users of the existing GetTexelSize method, and adapt them, e.g. WebGLTexture::MemoryUsage will want to divide by 8 the result, to return a number of bytes.

(In reply to Benoit Jacob [:bjacob] from comment #7) > ::: content/canvas/src/WebGLContext.cpp > @@ +885,5 @@ > > case WebGL_MOZ_WEBGL_lose_context: > > // We always support this extension. > > isSupported = true; > > break; > > + case WebGL_MOZ_WEBGL_compressed_texture_s3tc: > > MOZ_ is only needed in the extension string name, not in any c++ identifier. > Nevermind the bad example set by MOZ_WEBGL_lose_context above. I've updated WEBGL_compressed_texture_s3tc and WEBGL_lose_context c++ identifiers to lose the MOZ_ prefix > @@ +5373,5 @@ > > + ErrorInvalidOperation("compressedTexSubImage2D: width is not a multiple of 4 or equal to texture width"); > > + return; > > + } else if (height % 4 != 0 || height != imageInfo.Height()) { > > + ErrorInvalidOperation("compressedTexSubImage2D: height is not a multiple of 4 or equal to texture height"); > > + return; > > Another remark: according to > > http://www.khronos.org/opengles/sdk/docs/man/xhtml/glCompressedTexSubImage2D. > xml > We also should generate INVALID_VALUE if xoffset + width > imageInfo.Width() > and same for height. Alright, I added CanvasUtils::CheckSaneSubrectSize to check this

This patch fixes nits from the previous review, except for moving mCompressedTextureFormats.AppendElement(...) from WebGLContext.cpp to the S3TC constructor because I can't figure out how to get class friends working. Once more unto the try server: https://tbpl.mozilla.org/?tree=Try&rev=a226989d5f8b

Comment on attachment 621868 [details] [diff] [review] s3tc extension v3 Review of attachment 621868 [details] [diff] [review]: ----------------------------------------------------------------- Looks good now! r+ with some nits, and just one important thing that really needs to be fixed before landing: as it currently stands MemoryUsage will report 0 bytes for DXT1 textures due to integer division rounding below. Some more nits, and a place where I gave you wrong advice with CheckedInt, see below, leading to a theoretical bug, please correct (see below) before landing. ::: content/canvas/src/WebGLContext.cpp @@ +951,5 @@ > mEnabledExtensions[ei] = new WebGLExtensionLoseContext(this); > break; > + case WebGL_WEBGL_compressed_texture_s3tc: > + mEnabledExtensions[ei] = new WebGLExtensionCompressedTextureS3TC(this); > + // XXX Move these to the s3tc constructor if I can ever figure out C++ friends Just add friend class WebGLExtensionCompressedTextureS3TC; to WebGLContext, next to other friend declarations there. But it doesn't matter much so you can also leave these AppendElement's here and remove this comment. Or we can talk about it today. ::: content/canvas/src/WebGLContext.h @@ +1684,5 @@ > } > PRInt64 MemoryUsage() const { > if (!mIsDefined) > return 0; > + PRInt64 texelSizeInBytes = WebGLContext::GetBitsPerTexel(mFormat, mType) / 8; This does not work! As texelSizeInBytes is an integer, the division by 8 is rounded below, so you lose the benefit of having GetBitsPerTexel returning a number of bits. @@ +1685,5 @@ > PRInt64 MemoryUsage() const { > if (!mIsDefined) > return 0; > + PRInt64 texelSizeInBytes = WebGLContext::GetBitsPerTexel(mFormat, mType) / 8; > + return PRInt64(mWidth) * PRInt64(mHeight) * texelSizeInBytes; Do it all in one line: return PRInt64(mWidth) * PRInt64(mHeight) * texelSizeInBits / 8; Thanks to left-to-right associativity and equal precedence, the multiplications will be evaluated first, giving the total number of bits in the texture, then finally divided by 8. ::: content/canvas/src/WebGLContextValidate.cpp @@ +409,5 @@ > + switch (format) { > + case LOCAL_GL_COMPRESSED_RGB_S3TC_DXT1_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT1_EXT: > + { > + calculated_byteLength = ((CheckedUint32(width) + 3) / 4) * ((height + 3) / 4) * 8; Actually I was wrong, sorry: due to the parentheses, you need to also replace height by CheckedUint32(height). This doesn't matter in practice though, because it would only allow for overflow with height > 4 billion which is higher than allowed texture sizes. @@ +419,5 @@ > + } > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT3_EXT: > + case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT5_EXT: > + { > + calculated_byteLength = ((CheckedUint32(width) + 3) / 4) * ((height + 3) / 4) * 16; Same. @@ +439,5 @@ > + return true; > + } > + if (level > 0 > + && (width == 0 || width == 1 || width == 2 || width % 4 == 0) > + && (height == 0 || height == 1 || height == 2 || height % 4 == 0)) { Put the { on a new line after a multi-line if() condition.

v4 with all nits fixed. Carrying forward r+ from v3. Try server set to run mochitest-1: https://tbpl.mozilla.org/?tree=Try&rev=bb299138afb8

Sorry, backed out for compilation failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ba7cf6fd10ae eg https://tbpl.mozilla.org/php/getParsedLog.php?id=11574551&tree=Mozilla-Inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/f2f8b1127a54

Sorry, a combination of mistyping 'hg qimport' as 'hg import' and then forgetting a 'hg add'. Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/91a83411164e

Original landing and backout: https://hg.mozilla.org/mozilla-central/rev/ba7cf6fd10ae https://hg.mozilla.org/mozilla-central/rev/f2f8b1127a54 Relanded: https://hg.mozilla.org/mozilla-central/rev/91a83411164e

This got documented by other folks; I've added it to Firefox 15 for developers. https://developer.mozilla.org/en/WebGL/Using_Extensions#WEBGL_compressed_texture_s3tc

I bugged that its not working on Angle https://bugzilla.mozilla.org/show_bug.cgi?id=763559