Bug 75925 – Implement WebGLShaderPrecisionFormat

• Component: WebGL
• Status: RESOLVED FIXED

[Zhenyao Mo]

and its query

[Zhenyao Mo]

Created attachment 135256 [details]
Patch

[Zhenyao Mo]

Ken, please review.

Developing in Webkit becomes more and more painful when new files are added.

[Zhenyao Mo]

Created attachment 135353 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Kenneth Russell]

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.)

[WebKit Review Bot]

Comment on attachment 135353 [details]
Patch

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

[Zhenyao Mo]

Created attachment 135382 [details]
Patch

[Zhenyao Mo]

(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.

[Philippe Normand]

Comment on attachment 135382 [details]
Patch

Attachment 135382 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12322293

[Kenneth Russell]

Comment on attachment 135382 [details]
Patch

Thanks for the updates. Looks good. r=me again, though looks like there are still build issues.

[Zhenyao Mo]

(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.

[Zhenyao Mo]

Committed r113092: <http://trac.webkit.org/changeset/113092>