Bug 72086 – Implement WEBGL_compressed_textures WebGL extension

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Gregg Tavares]

Implement WEBGL_EXPERIMENTAL_compressed_textures WebGL extension

[Gregg Tavares]

Created attachment 114619 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Early Warning System Bot]

Comment on attachment 114619 [details]
Patch

Attachment 114619 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10429290

[Kenneth Russell]

Comment on attachment 114619 [details]
Patch

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

Looks great overall. A few minor comments, plus the GraphicsContext3DOpenGL build fix we discussed offline.

> Source/WebCore/html/canvas/WebGLExperimentalCompressedTextures.cpp:48
> +    if (gc3DContext->getExtensions()->supports("GL_EXT_texture_compression_dxt1")) {

Consider caching the result of getExtensions() in a local variable.

> Source/WebCore/html/canvas/WebGLExperimentalCompressedTextures.cpp:104
> +    return gc3DContext->getExtensions()->supports("GL_EXT_texture_compression_dxt1")

Here too.

> Source/WebCore/html/canvas/WebGLExperimentalCompressedTextures.cpp:116
> +        if (m_supportsDxt1)

Could just return m_supportsDxt1. Here and below.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:200
> +    // m_internal->compressedTexImage2D(target, level, internalformat, width, height, border, imageSize, data);

Add a FIXME here and below.

> Source/WebKit/chromium/public/WebGraphicsContext3D.h:226
> +    virtual void compressedTexSubImage2D(WGC3Denum target, WGC3Dint level, WGC3Dint xoffset, WGC3Dint yoffset, WGC3Dsizei width, WGC3Dsizei height, WGC3Denum format, WGC3Dsizei imageSize, const void* data) = 0;

Since these are pure virtual, you'll need to roll forward chromium_rev in Source/WebKit/chromium/DEPS to the revision where you added these entry points, if it hasn't been already.

[Gregg Tavares]

Created attachment 114775 [details]
Patch

[Gregg Tavares]

Created attachment 114806 [details]
Patch

[Kenneth Russell]

Comment on attachment 114806 [details]
Patch

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

Looks great. Did you want this submitted via the commit queue? Please mark the patch cq? if so.

> Source/WebKit/chromium/DEPS:35
> +  'chromium_rev': '109568'

I heard from someone else today that there was a problem rolling forward the Chromium revision, and that the most recent roll was reverted. Could you check on IRC with the Chromium WebKit gardener to see if that problem was resolved?

[Tony Chang]

(In reply to comment #7)
> (From update of attachment 114806 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114806&action=review
> 
> > Source/WebKit/chromium/DEPS:35
> > +  'chromium_rev': '109568'
> 
> I heard from someone else today that there was a problem rolling forward the Chromium revision, and that the most recent roll was reverted. Could you check on IRC with the Chromium WebKit gardener to see if that problem was resolved?

Ojan is about to roll to 109696 in https://bugs.webkit.org/attachment.cgi?id=114762&action=prettypatch .

[Kenneth Russell]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 114806 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=114806&action=review
> > 
> > > Source/WebKit/chromium/DEPS:35
> > > +  'chromium_rev': '109568'
> > 
> > I heard from someone else today that there was a problem rolling forward the Chromium revision, and that the most recent roll was reverted. Could you check on IRC with the Chromium WebKit gardener to see if that problem was resolved?
> 
> Ojan is about to roll to 109696 in https://bugs.webkit.org/attachment.cgi?id=114762&action=prettypatch .

Great, thanks. In that case, Gregg, could you re-upload the patch with that section removed? Any committer should be able to cq+ that without needing a re-review.

[Gregg Tavares]

Created attachment 114818 [details]
Patch

[Kenneth Russell]

Comment on attachment 114818 [details]
Patch

Looks great.

[WebKit Review Bot]

Comment on attachment 114818 [details]
Patch

Clearing flags on attachment: 114818

Committed r100176: <http://trac.webkit.org/changeset/100176>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Roben (:aroben)]

This broke the Lion build:

http://build.webkit.org/builders/Lion%20Intel%20Debug%20%28Build%29/builds/3595/steps/compile-webkit/logs/stdio

Undefined symbols for architecture x86_64:
  "__ZN7WebCore35WebGLExperimentalCompressedTextures6createEPNS_21WebGLRenderingContextE", referenced from:
      __ZN7WebCore21WebGLRenderingContext12getExtensionERKN3WTF6StringE in WebGLRenderingContext.o
  "__ZN7WebCore35WebGLExperimentalCompressedTextures27getCompressedTextureFormatsEv", referenced from:
      __ZN7WebCore21WebGLRenderingContext12getParameterEjRi in WebGLRenderingContext.o
  "__ZN7WebCore35WebGLExperimentalCompressedTextures9supportedEPNS_21WebGLRenderingContextE", referenced from:
      __ZN7WebCore21WebGLRenderingContext22getSupportedExtensionsEv in WebGLRenderingContext.o

[David Kilzer (:ddkilzer)]

(In reply to comment #14)
> This broke the Lion build:
> 
> http://build.webkit.org/builders/Lion%20Intel%20Debug%20%28Build%29/builds/3595/steps/compile-webkit/logs/stdio
> 
> Undefined symbols for architecture x86_64:
>   "__ZN7WebCore35WebGLExperimentalCompressedTextures6createEPNS_21WebGLRenderingContextE", referenced from:
>       __ZN7WebCore21WebGLRenderingContext12getExtensionERKN3WTF6StringE in WebGLRenderingContext.o
>   "__ZN7WebCore35WebGLExperimentalCompressedTextures27getCompressedTextureFormatsEv", referenced from:
>       __ZN7WebCore21WebGLRenderingContext12getParameterEjRi in WebGLRenderingContext.o
>   "__ZN7WebCore35WebGLExperimentalCompressedTextures9supportedEPNS_21WebGLRenderingContextE", referenced from:
>       __ZN7WebCore21WebGLRenderingContext22getSupportedExtensionsEv in WebGLRenderingContext.o

How did this patch make it past the commit queue?

[David Kilzer (:ddkilzer)]

(In reply to comment #15)
> (In reply to comment #14)
> > This broke the Lion build:
> > 
> > http://build.webkit.org/builders/Lion%20Intel%20Debug%20%28Build%29/builds/3595/steps/compile-webkit/logs/stdio
> > 
> > Undefined symbols for architecture x86_64:
> >   "__ZN7WebCore35WebGLExperimentalCompressedTextures6createEPNS_21WebGLRenderingContextE", referenced from:
> >       __ZN7WebCore21WebGLRenderingContext12getExtensionERKN3WTF6StringE in WebGLRenderingContext.o
> >   "__ZN7WebCore35WebGLExperimentalCompressedTextures27getCompressedTextureFormatsEv", referenced from:
> >       __ZN7WebCore21WebGLRenderingContext12getParameterEjRi in WebGLRenderingContext.o
> >   "__ZN7WebCore35WebGLExperimentalCompressedTextures9supportedEPNS_21WebGLRenderingContextE", referenced from:
> >       __ZN7WebCore21WebGLRenderingContext22getSupportedExtensionsEv in WebGLRenderingContext.o
> 
> How did this patch make it past the commit queue?

Oh it's the bots-can't-check-patches-by-non-committers rule.  If we land the patch, it seems like the buildbots will be building the patch anyway, so why not test it before landing via the commit bot?  At least it's had a review by a trusted person.

[Julien Chaffraix]

Rolled the patch in http://trac.webkit.org/changeset/100191 because of the previous missing symbols.

[Kenneth Russell]

I'll figure out the missing symbol problem and resubmit the patch. It really shouldn't have made it through the commit queue.

[Kenneth Russell]

Created attachment 115313 [details]
Patch

[Kenneth Russell]

Updated DerivedSources.make and WebCore.xcodeproj to fix Mac build.

[WebKit Review Bot]

Comment on attachment 115313 [details]
Patch

Attachment 115313 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10484643

[Kenneth Russell]

Created attachment 115476 [details]
Patch

[Kenneth Russell]

Merged with move of ArrayBufferView from WebCore to WTF.

[Chris Marrin]

Comment on attachment 115476 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2117
> +    if (equalIgnoringCase(name, "WEBGL_EXPERIMENTAL_compressed_textures")) {

Based on the discussions on the WebGL mailing list, we should hold off adding an extension with this name. My feeling is that this should be called WEBKIT_WEBGL_compressed_textures, but I know we haven't made a decision. So until we do, we shouldn't go forward with this name.

[Kenneth Russell]

Created attachment 117286 [details]
Patch after extension rename

[Kenneth Russell]

Per feedback above and after discussion on public_webgl mailing list, exposed the extension under the name WEBKIT_WEBGL_compressed_textures. Submitting to EWS bots before committing. Further feedback welcome, but review not necessary; it's Gregg's patch and I've already reviewed it.

[Early Warning System Bot]

Comment on attachment 117286 [details]
Patch after extension rename

Attachment 117286 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10687354

[Kenneth Russell]

Created attachment 117293 [details]
Fixed more build issues

[Kenneth Russell]

Committed r101578: <http://trac.webkit.org/changeset/101578>

[Ben Adams]

Probably not the right place - but...

Thank you, thank you, thank you!

Ben