Bug 75925

Summary: Implement WebGLShaderPrecisionFormat
Product: WebKit Reporter: Zhenyao Mo <zmo>
Component: WebGLAssignee: Zhenyao Mo <zmo>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cmarrin, dglazkov, fishd, gustavo, jamesr, kbr, ojan, pnormand, rakuco, tkent, vestbo, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch kbr: review+, pnormand: commit-queue-

Description Zhenyao Mo 2012-01-09 18:40:18 PST
and its query
Comment 1 Zhenyao Mo 2012-04-02 19:02:03 PDT
Created attachment 135256 [details]
Patch
Comment 2 Zhenyao Mo 2012-04-02 19:03:10 PDT
Ken, please review.

Developing in Webkit becomes more and more painful when new files are added.
Comment 3 Zhenyao Mo 2012-04-03 10:21:35 PDT
Created attachment 135353 [details]
Patch
Comment 4 WebKit Review Bot 2012-04-03 10:24:50 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 5 Kenneth Russell 2012-04-03 10:59:00 PDT
Comment on attachment 135353 [details]
Patch

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

Looks good overall. A few minor issues which you can fix upon landing, or feel free to upload a new version. r=me

> Source/Platform/chromium/public/WebGraphicsContext3D.h:289
> +    virtual void getShaderPrecisionFormat(WGC3Denum shadertype, WGC3Denum precisiontype, WGC3Dint* range, WGC3Dint* precision) = 0;

Has the Chromium implementation of this already landed?

> Source/WebCore/html/canvas/WebGLShaderPrecisionFormat.h:38
> +    static PassRefPtr<WebGLShaderPrecisionFormat> create(GC3Dint rangeMin, GC3Dint rangeMax, GC3Dint precision)

Please consider putting these inlined methods into a .cpp file. Having them in the header increases code size.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1217
> +void GraphicsContext3D::getShaderPrecisionFormat(GC3Denum shaderType, GC3Denum precisionType, GC3Dint* range, GC3Dint* precision)

Architecturally speaking I think this should go in GraphicsContext3DOpenGL.cpp. That file targets desktop GL specifically, while the Common file is used on both ES and GL systems. This hand-coded implementation is only needed on desktop GL, since on ES 2.0 this call actually exists.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1229
> +        range[0] = -31;

Could you add a comment indicating that these constants came from the Chromium port, and that we believe they originally came from making the actual API call on a representative desktop system?

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:1375
> +        range[0] = -31;

Same comment needed here. Also I think a FIXME should be added indicating that this should be retargeted at the real getShaderPrecisionFormat call on true ES 2.0 hardware. (I believe the Qt port aims to run on ES 2.0.)
Comment 6 WebKit Review Bot 2012-04-03 11:27:09 PDT
Comment on attachment 135353 [details]
Patch

Attachment 135353 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12321246
Comment 7 Zhenyao Mo 2012-04-03 11:56:51 PDT
Created attachment 135382 [details]
Patch
Comment 8 Zhenyao Mo 2012-04-03 11:58:09 PDT
(In reply to comment #5)
> (From update of attachment 135353 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135353&action=review
> 
> Looks good overall. A few minor issues which you can fix upon landing, or feel free to upload a new version. r=me
> 
> > Source/Platform/chromium/public/WebGraphicsContext3D.h:289
> > +    virtual void getShaderPrecisionFormat(WGC3Denum shadertype, WGC3Denum precisiontype, WGC3Dint* range, WGC3Dint* precision) = 0;
> 
> Has the Chromium implementation of this already landed?

Yes.

> 
> > Source/WebCore/html/canvas/WebGLShaderPrecisionFormat.h:38
> > +    static PassRefPtr<WebGLShaderPrecisionFormat> create(GC3Dint rangeMin, GC3Dint rangeMax, GC3Dint precision)
> 
> Please consider putting these inlined methods into a .cpp file. Having them in the header increases code size.

Done.

> 
> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1217
> > +void GraphicsContext3D::getShaderPrecisionFormat(GC3Denum shaderType, GC3Denum precisionType, GC3Dint* range, GC3Dint* precision)
> 
> Architecturally speaking I think this should go in GraphicsContext3DOpenGL.cpp. That file targets desktop GL specifically, while the Common file is used on both ES and GL systems. This hand-coded implementation is only needed on desktop GL, since on ES 2.0 this call actually exists.

Done, and also add the function to GraphicsContext3DOpenGLES.cpp

> 
> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1229
> > +        range[0] = -31;
> 
> Could you add a comment indicating that these constants came from the Chromium port, and that we believe they originally came from making the actual API call on a representative desktop system?

Done.

> 
> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:1375
> > +        range[0] = -31;
> 
> Same comment needed here. Also I think a FIXME should be added indicating that this should be retargeted at the real getShaderPrecisionFormat call on true ES 2.0 hardware. (I believe the Qt port aims to run on ES 2.0.)

Done.
Comment 9 Philippe Normand 2012-04-03 12:08:36 PDT
Comment on attachment 135382 [details]
Patch

Attachment 135382 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12322293
Comment 10 Kenneth Russell 2012-04-03 12:11:40 PDT
Comment on attachment 135382 [details]
Patch

Thanks for the updates. Looks good. r=me again, though looks like there are still build issues.
Comment 11 Zhenyao Mo 2012-04-03 12:58:46 PDT
(In reply to comment #10)
> (From update of attachment 135382 [details])
> Thanks for the updates. Looks good. r=me again, though looks like there are still build issues.

The build issue in chromium was due to FakeWebGraphicsContext3D missing the implementation of getShaderPrecisionFormat, which I fixed in the patch.  Hopefully the bot will turn green.
Comment 12 Zhenyao Mo 2012-04-03 14:55:15 PDT
Committed r113092: <http://trac.webkit.org/changeset/113092>