Bug 101346 – No condition present to check if Compressed Texture S3TC extension support exists

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: WebGL

WebGLRenderingCOntext::getExtension does not check if compressed_texture_s3tc is supported before returning a WebGLExtension object.

Created attachment 172581 [details]
proposed patch

proposed patch

[George Staikos]

Is this patch up for review?

Yes, this patch is up for review.

(In reply to comment #2)
> Is this patch up for review?

[Antonio Gomes]

Comment on attachment 172581 [details]
proposed patch

Please read http://www.webkit.org/coding/contributing.html

Created attachment 173662 [details]
Patch

[WebKit Review Bot]

Attachment 173662 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2369:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 173674 [details]
Patch

[Rob Buis]

(In reply to comment #7)
> Created an attachment (id=173674) [details]
> Patch

Do you mean this to be BlackBerry specific? If so you need to ifdef (PLATFORM(BB)) the change. If this is for all ports, the [BlackBerry] in the title is misleading, and we want non BB reviewers to look at it.

Created attachment 175016 [details]
Patch

(In reply to comment #8)
Yes, the patch is meant to be BlackBerry specific, and the change you suggested has been made.
> (In reply to comment #7)
> > Created an attachment (id=173674) [details] [details]
> > Patch
> 
> Do you mean this to be BlackBerry specific? If so you need to ifdef (PLATFORM(BB)) the change. If this is for all ports, the [BlackBerry] in the title is misleading, and we want non BB reviewers to look at it.

[Yong Li]

(In reply to comment #10)
> (In reply to comment #8)
> Yes, the patch is meant to be BlackBerry specific, and the change you suggested has been made.
> > (In reply to comment #7)
> > > Created an attachment (id=173674) [details] [details] [details]
> > > Patch
> > 
> > Do you mean this to be BlackBerry specific? If so you need to ifdef (PLATFORM(BB)) the change. If this is for all ports, the [BlackBerry] in the title is misleading, and we want non BB reviewers to look at it.

Might be also good for other platforms?

(In reply to comment #11)
We're not sure about the string that's being used to check whether s3tc is supported or not. We are using this website (http://www.opengl.org/registry/specs/EXT/texture_compression_s3tc.txt) as our basis for the string being passed in, but we're not a 100% sure and don't want other platforms to suffer because of this. 

> (In reply to comment #10)
> > (In reply to comment #8)
> > Yes, the patch is meant to be BlackBerry specific, and the change you suggested has been made.
> > > (In reply to comment #7)
> > > > Created an attachment (id=173674) [details] [details] [details] [details]
> > > > Patch
> > > 
> > > Do you mean this to be BlackBerry specific? If so you need to ifdef (PLATFORM(BB)) the change. If this is for all ports, the [BlackBerry] in the title is misleading, and we want non BB reviewers to look at it.
> 
> Might be also good for other platforms?

[Yong Li]

(In reply to comment #12)
> (In reply to comment #11)
> We're not sure about the string that's being used to check whether s3tc is supported or not. We are using this website (http://www.opengl.org/registry/specs/EXT/texture_compression_s3tc.txt) as our basis for the string being passed in, but we're not a 100% sure and don't want other platforms to suffer because of this. 
> 

If a platform does support GL_EXT_texture_compression_s3tc but Extensions3D::supports(GL_EXT_texture_compression_s3tc) returns false, I would think it is a bug in that port, and should be fixed by people who maintain that port.

I think this is actually a cross-platform fix. Can you create a layout-test and also get WebGL reviewers involved?

class Extensions3D {
public:
    virtual ~Extensions3D() {}

    // Supported extensions:
    //   GL_EXT_texture_format_BGRA8888
    //   GL_EXT_read_format_bgra
    //   GL_ARB_robustness
    //   GL_ARB_texture_non_power_of_two / GL_OES_texture_npot
    //   GL_EXT_packed_depth_stencil / GL_OES_packed_depth_stencil
    //   GL_ANGLE_framebuffer_blit / GL_ANGLE_framebuffer_multisample
    //   GL_OES_texture_float
    //   GL_OES_standard_derivatives
    //   GL_OES_rgb8_rgba8
    //   GL_OES_vertex_array_object
    //   GL_ANGLE_translated_shader_source
    //   GL_ARB_texture_rectangle (only the subset required to
    //     implement IOSurface binding; it's recommended to support
    //     this only on Mac OS X to limit the amount of code dependent
    //     on this extension)
    //   GL_EXT_texture_compression_dxt1
    //   GL_EXT_texture_compression_s3tc
    //   GL_OES_compressed_ETC1_RGB8_texture
    //   GL_IMG_texture_compression_pvrtc
    //   EXT_texture_filter_anisotropic
    //   GL_EXT_debug_marker
    //   GL_CHROMIUM_copy_texture
    //   GL_CHROMIUM_flipy

    // Takes full name of extension; for example,
    // "GL_EXT_texture_format_BGRA8888".
    virtual bool supports(const String&) = 0;

Comment on attachment 173674 [details]
Patch

No longer obsolete, since it's being considered for all platforms.

Comment on attachment 175016 [details]
Patch

Obsolete.

[WebKit Review Bot]

Comment on attachment 173674 [details]
Patch

Rejecting attachment 173674 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

Created attachment 175248 [details]
Patch

[Kenneth Russell]

Comment on attachment 175248 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175248&action=review

Looks fair overall but needs a little more work.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2382
> +        && m_context->getExtensions()->supports("GL_EXT_texture_compression_s3tc")) {

Use WebGLCompressedTextureS3TC::supported(WebGLRenderingContext*) instead to keep the checks here and WebGLRenderingContext::getSupportedExtensions symmetric.

Created attachment 175505 [details]
Patch

[Kenneth Russell]

Comment on attachment 175505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175505&action=review

Sorry for the delay getting back to this. Looks good; r=me. Please make the requested cleanups. You can then add my name to the "Reviewed by" line by hand, upload a revised patch, and any committer can cq+ it.

> LayoutTests/ChangeLog:4
> +        LayoutTests:

Remove this and surrounding blank lines.

> LayoutTests/fast/canvas/webgl/webgl-compressed-texture-s3tc.html:1
> +<!DOCTYPE html>

Please clean up the weird characters at the beginning of this line.

Created attachment 176808 [details]
Patch

[WebKit Review Bot]

Comment on attachment 176808 [details]
Patch

Rejecting attachment 176808 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Kenneth Russell]

Sorry, somehow this got lost. This issue was recently rediscovered in https://bugs.webkit.org/show_bug.cgi?id=109508 and the code change was made, so the attached patch will now have conflicts, but if you want to land the test, that would be useful.

[Darin Adler]

Comment on attachment 176808 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176808&action=review

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2382
> +    if (equalIgnoringCase(name, "WEBKIT_WEBGL_compressed_texture_s3tc")
> +        && WebGLCompressedTextureS3TC::supported(this)) {

Normally we’d just put this on one line.