Bug 31169 – Need to implement WebGLContextAttributes

• Component: WebGL
• Status: RESOLVED FIXED

[Chris Marrin]

This object allows characteristics of the drawing buffer to be passed to getContext

[Kenneth Russell]

*** Bug 31176 has been marked as a duplicate of this bug. ***

[Kenneth Russell]

Created attachment 46194 [details]
Patch

Added the WebGLContextAttributes class and custom JavaScript bindings to accept a native object as the second argument to getContext("experimental-webgl") per the WebGL specification. Passed new attributes object down through HTMLCanvasElement::getContext to WebGLRenderingContext and GraphicsContext3D. Added getContextAttributes() to WebGLRenderingContext. Added test case ensuring that context attributes can be passed down and returned.  Tested in Safari and Chromium. The attributes will be hooked up to the creation of the OpenGL context in bug 33416.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/HTMLCanvasElement.h:72:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1

[Kenneth Russell]

Created attachment 46195 [details]
Revised patch

Revised patch fixing style error

[Chris Marrin]

Comment on attachment 46195 [details]
Revised patch

> Index: WebCore/platform/graphics/GraphicsContext3D.h
> ===================================================================
> --- WebCore/platform/graphics/GraphicsContext3D.h	(revision 53013)
> +++ WebCore/platform/graphics/GraphicsContext3D.h	(working copy)
> @@ -55,6 +55,7 @@ namespace WebCore {
>      class WebGLActiveInfo;
>      class WebGLArray;
>      class WebGLBuffer;
> +    class WebGLContextAttributes;
>      class WebGLUnsignedByteArray;
>      class WebGLFloatArray;
>      class WebGLFramebuffer;
> @@ -382,7 +383,7 @@ namespace WebCore {
>              INVALID_FRAMEBUFFER_OPERATION = 0x0506
>          };
>          
> -        static PassOwnPtr<GraphicsContext3D> create();
> +        static PassOwnPtr<GraphicsContext3D> create(WebGLContextAttributes* attrs);
>          virtual ~GraphicsContext3D();
>  
>  #if PLATFORM(MAC)
> @@ -459,6 +460,12 @@ namespace WebCore {
>  
>          void getBufferParameteriv(unsigned long target, unsigned long pname, int* value);
>  
> +        void getContextAttributes(bool& alpha,
> +                                  bool& depth,
> +                                  bool& stencil,
> +                                  bool& antialias,
> +                                  bool& premultipliedAlpha);
> +
>          unsigned long getError();
>  
>          void getFloatv(unsigned long pname, float* value);
> @@ -602,11 +609,16 @@ namespace WebCore {
>          void synthesizeGLError(unsigned long error);
>  
>      private:        
> -        GraphicsContext3D();
> +        GraphicsContext3D(WebGLContextAttributes* attrs);
>  
>          int m_currentWidth, m_currentHeight;
>          
>  #if PLATFORM(MAC)
> +        bool m_alphaAttr;
> +        bool m_depthAttr;
> +        bool m_stencilAttr;
> +        bool m_antialiasAttr;
> +        bool m_premultipliedAlphaAttr;
>          Vector<Vector<float> > m_vertexArray;

It might be better to put these into a little helper class to make it easier to pass them in and out. It would make it easier to expand in the future as well.In fact you could use an instance of this class to hold the data in the WebGLContextAttributes object to avoid a lot of copying.

[WebKit Review Bot]

Attachment 46195 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/176398

[Kenneth Russell]

(In reply to comment #5)
> It might be better to put these into a little helper class to make it easier to
> pass them in and out. It would make it easier to expand in the future as
> well.In fact you could use an instance of this class to hold the data in the
> WebGLContextAttributes object to avoid a lot of copying.

I agree, but do you have a suggestion on where that class should go? It seems that putting it in WebCore/html/canvas is not allowed, because headers in WebCore/platform/graphics are not supposed to refer upward. I suppose it could be a nested class of GraphicsContext3D, such as GraphicsContext3D::Attributes.

[Oliver Hunt]

Comment on attachment 46195 [details]
Revised patch

I think a problem here is that you're making GraphicsContext3D depend on the DOM type WebGLContextAttributes -- it should not be.  The dependence on DOM types in GraphicsContext3D is a serious design bug that we need to fix, adding yyet more dependencies is a bad thing.

I would suggest that these arguments simply be passed as either bool or enum parameters to GraphicsContext3D constructor.  I'd also like to get shared storage and implementation of these attributes, rather than having platform ifdefs scattered throughout the code.  We spent a lot of time rectifying this in the 2d context, doing it in the 3d context seems bad.

[Chris Marrin]

Put the class in GraphicsContext3D.h and use it in WebGLContextAttributes. Then you can pass the class to GraphicsContext3D APIs and avoid any WebGLContextAttributes dependencies.

[Kenneth Russell]

Created attachment 46313 [details]
Revised patch

Added GraphicsContext3D::Attributes struct as the bridge between the DOM and graphics layers to avoid abstraction violations. Revised ChangeLog descriptions. Also fixed up build failures on qt and gtk bots from last patch.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/HTMLCanvasElement.h:72:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1

[Chris Marrin]

It might be better to use a class for GraphicsContext3D so it can have proper initializers and you don't have to worry about the members having a rational value.

[Kenneth Russell]

(In reply to comment #12)
> It might be better to use a class for GraphicsContext3D so it can have proper
> initializers and you don't have to worry about the members having a rational
> value.

I considered that, but that will duplicate the setter/getter code in WebGLContextAttributes. It's simpler to leave it as a struct since at the top level these values come in via WebGLContextAttributes.

I'll upload a new patch fixing the style issue.

[Kenneth Russell]

Created attachment 46330 [details]
Revised patch

Fixed style issue in previous patch.

[Chris Marrin]

(In reply to comment #13)
> (In reply to comment #12)
> > It might be better to use a class for GraphicsContext3D so it can have proper
> > initializers and you don't have to worry about the members having a rational
> > value.
> 
> I considered that, but that will duplicate the setter/getter code in
> WebGLContextAttributes. It's simpler to leave it as a struct since at the top
> level these values come in via WebGLContextAttributes.

I'm not sure what you mean. You can keep the member variables public and just add a ctor which will init them to the correct default. In fact, you can just leave it as a struct and add a ctor, since the only difference between the struct and class keywords is that everything in a struct is implicitly public.

I just like to avoid the possibility of uninitialized variables when we have the mechanisms to avoid it, mostly for free.

[Kenneth Russell]

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > It might be better to use a class for GraphicsContext3D so it can have proper
> > > initializers and you don't have to worry about the members having a rational
> > > value.
> > 
> > I considered that, but that will duplicate the setter/getter code in
> > WebGLContextAttributes. It's simpler to leave it as a struct since at the top
> > level these values come in via WebGLContextAttributes.
> 
> I'm not sure what you mean. You can keep the member variables public and just
> add a ctor which will init them to the correct default. In fact, you can just
> leave it as a struct and add a ctor, since the only difference between the
> struct and class keywords is that everything in a struct is implicitly public.
> 
> I just like to avoid the possibility of uninitialized variables when we have
> the mechanisms to avoid it, mostly for free.

I assumed it would be bad form to add a constructor to a struct. Since it isn't I'll make this change.

[Kenneth Russell]

Created attachment 46385 [details]
Revised patch

Moved initialization of attribute flags into constructor on GraphicsContext3D::Attributes struct.

[Oliver Hunt]

Comment on attachment 46385 [details]
Revised patch

r=me

[WebKit Commit Bot]

Comment on attachment 46385 [details]
Revised patch

Clearing flags on attachment: 46385

Committed r53238: <http://trac.webkit.org/changeset/53238>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

This checkin broke Tiger.  Sadly the commit-queue can't detect that yet.
http://build.webkit.org/builders/Tiger%20Intel%20Release/builds/7737/steps/compile-webkit/logs/stdio

I'll fix it.  It should be a very simple fix.

[Eric Seidel (no email)]

(This, btw, is no negative reflection on you kbr.  Simply logging everything in the bug for posterity.  The commit-queue should be bullet-proof and know how to clean up after itself when it break things.  I'm working on teaching it how to roll out its own commits when it can be sure they broke things.)

[Eric Seidel (no email)]

Committed r53243: <http://trac.webkit.org/changeset/53243>

[Kenneth Russell]

Sorry about that. I'm surprised the qt/gtk bots didn't catch that mistake since they caught other areas where I hadn't built with ENABLE(3D_CANVAS) turned off.

[Eric Seidel (no email)]

The EWS bots don't build debug yet.  They should.  We'll make them do so as soon as they're fast enough. :)  I've filed bug 33681 about this.