Bug 101346 - No condition present to check if Compressed Texture S3TC extension support exists
Summary: No condition present to check if Compressed Texture S3TC extension support ex...
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: akaushik
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-06 07:26 PST by akaushik
Modified: 2017-07-18 08:29 PDT (History)
13 users (show)

See Also:


Attachments
proposed patch (904 bytes, patch)
2012-11-06 07:28 PST, akaushik
no flags Details | Formatted Diff | Diff
Patch (1.84 KB, patch)
2012-11-12 09:39 PST, akaushik
no flags Details | Formatted Diff | Diff
Patch (1.84 KB, patch)
2012-11-12 11:14 PST, akaushik
no flags Details | Formatted Diff | Diff
Patch (1.83 KB, patch)
2012-11-19 11:56 PST, akaushik
no flags Details | Formatted Diff | Diff
Patch (7.43 KB, patch)
2012-11-20 10:57 PST, akaushik
no flags Details | Formatted Diff | Diff
Patch (7.40 KB, patch)
2012-11-21 13:12 PST, akaushik
no flags Details | Formatted Diff | Diff
Patch (7.37 KB, patch)
2012-11-29 13:36 PST, akaushik
darin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description akaushik 2012-11-06 07:26:00 PST
WebGLRenderingCOntext::getExtension does not check if compressed_texture_s3tc is supported before returning a WebGLExtension object.
Comment 1 akaushik 2012-11-06 07:28:15 PST
Created attachment 172581 [details]
proposed patch

proposed patch
Comment 2 George Staikos 2012-11-11 10:21:10 PST
Is this patch up for review?
Comment 3 akaushik 2012-11-12 05:10:51 PST
Yes, this patch is up for review.

(In reply to comment #2)
> Is this patch up for review?
Comment 4 Antonio Gomes 2012-11-12 05:44:52 PST
Comment on attachment 172581 [details]
proposed patch

Please read http://www.webkit.org/coding/contributing.html
Comment 5 akaushik 2012-11-12 09:39:39 PST
Created attachment 173662 [details]
Patch
Comment 6 WebKit Review Bot 2012-11-12 10:25:40 PST
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.
Comment 7 akaushik 2012-11-12 11:14:13 PST
Created attachment 173674 [details]
Patch
Comment 8 Rob Buis 2012-11-15 12:48:22 PST
(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.
Comment 9 akaushik 2012-11-19 11:56:34 PST
Created attachment 175016 [details]
Patch
Comment 10 akaushik 2012-11-19 11:58:40 PST
(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.
Comment 11 Yong Li 2012-11-19 12:36:28 PST
(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?
Comment 12 akaushik 2012-11-19 12:43:59 PST
(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?
Comment 13 Yong Li 2012-11-19 12:50:00 PST
(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 14 akaushik 2012-11-20 06:02:13 PST
Comment on attachment 173674 [details]
Patch

No longer obsolete, since it's being considered for all platforms.
Comment 15 akaushik 2012-11-20 06:02:36 PST
Comment on attachment 175016 [details]
Patch

Obsolete.
Comment 16 WebKit Review Bot 2012-11-20 06:02:44 PST
Comment on attachment 173674 [details]
Patch

Rejecting attachment 173674 [details] from review queue.

akaushik@rim.com 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.
Comment 17 akaushik 2012-11-20 10:57:26 PST
Created attachment 175248 [details]
Patch
Comment 18 Kenneth Russell 2012-11-20 12:11:48 PST
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.
Comment 19 akaushik 2012-11-21 13:12:26 PST
Created attachment 175505 [details]
Patch
Comment 20 Kenneth Russell 2012-11-27 10:56:17 PST
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.
Comment 21 akaushik 2012-11-29 13:36:30 PST
Created attachment 176808 [details]
Patch
Comment 22 WebKit Review Bot 2013-02-15 10:58:19 PST
Comment on attachment 176808 [details]
Patch

Rejecting attachment 176808 [details] from commit-queue.

rwlbuis@gmail.com 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.
Comment 23 Kenneth Russell 2013-02-15 11:26:50 PST
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.
Comment 24 Darin Adler 2013-03-06 09:06:42 PST
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.