Bug 42905 – Implement OpenGLES2 helper classes.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Stephen White]

Implement OpenGLES2 helper classes.

[Stephen White]

Created attachment 62458 [details]
Patch

[James Robinson]

The ChangeLog format is off.

I think Canvas2DLayerChromium should be a separate patch.

You can use OwnArrayPtr<> instead of doing new[]/delete[] yourself.

GLES2Canvas.cpp loadShader should use some sort of LOG macro instead of printf() to display the shader log.  This function also has a new[]/delete mismatch - another good time to use OwnArrayPtr<> :).

Otherwise looks great to me!

[WebKit Review Bot]

Attachment 62458 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3566409

[James Robinson]

The .h has its contents behind #if guards but the cpp isn't guarded.

[Stephen White]

Created attachment 62467 [details]
Patch

[Stephen White]

Created attachment 62468 [details]
Patch

[James Robinson]

Comment on attachment 62468 [details]
Patch

Some nits below, but I think this looks great otherwise.

> Index: WebCore/platform/graphics/chromium/GLES2Canvas.cpp
> ===================================================================
> --- WebCore/platform/graphics/chromium/GLES2Canvas.cpp	(revision 0)
> +++ WebCore/platform/graphics/chromium/GLES2Canvas.cpp	(revision 0)
> +
> +#include "config.h"
> +
> +#if USE(GLES2_RENDERING)
> +
> +#include "GLES2Canvas.h"
> +
> +#include "FloatRect.h"
> +#include "GLES2Context.h"
> +#include "GLES2Texture.h"
> +#include "OwnArrayPtr.h"

#include <wtf/OwnArrayPtr.h>, otherwise I'm not sure this will compile on all systems.

> +void GLES2Canvas::rotate(float angleInRadians)
> +{
> +    m_state->m_ctm.rotate(angleInRadians * (180.0f / 3.14159265f));
> +}

Sucks to hard-code the value of pi.

> Index: WebCore/platform/graphics/chromium/GLES2Canvas.h
> ===================================================================
> --- WebCore/platform/graphics/chromium/GLES2Canvas.h	(revision 0)
> +++ WebCore/platform/graphics/chromium/GLES2Canvas.h	(revision 0)
> +class GLES2Canvas : public Noncopyable {
> +public:
> +    GLES2Canvas(GLES2Context* context, int width, int height);
> +    ~GLES2Canvas();
> +
> +    void fillRect(const FloatRect& rect, const Color& color,
> +                  ColorSpace colorSpace);
> +    void fillRect(const FloatRect& rect);
> +    void clearRect(const FloatRect& rect);
> +    void setFillColor(const Color& color, ColorSpace colorSpace);
> +    void setAlpha(float alpha);
> +    void setCompositeOperation(CompositeOperator op);
> +    void translate(float x, float y);
> +    void rotate(float angleInRadians);
> +    void scale(const FloatSize& size);
> +    void concatCTM(const AffineTransform& affine);

Leave out the parameter names in these declarations unless they are useful (i.e. 'GLES2Context* context' 'FloatRect rect' are useless, 'int width' is useful).

> Index: WebCore/platform/graphics/chromium/GLES2Texture.cpp
> ===================================================================
> --- WebCore/platform/graphics/chromium/GLES2Texture.cpp	(revision 0)
> +++ WebCore/platform/graphics/chromium/GLES2Texture.cpp	(revision 0)
> +
> +#include "OwnArrayPtr.h"

#include <wtf/OwnArrayPtr.h>

> +void GLES2Texture::load(void* pixels)
> +{
> +        glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, m_width, m_height, glFormat, glType, buf.get());
> +    } else
> +      glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, m_width, m_height, glFormat, glType, pixels);
> +}

Indentation off here

[Stephen White]

(In reply to comment #7)
> (From update of attachment 62468 [details])
> Some nits below, but I think this looks great otherwise.
> 
> > Index: WebCore/platform/graphics/chromium/GLES2Canvas.cpp
> > ===================================================================
> > --- WebCore/platform/graphics/chromium/GLES2Canvas.cpp	(revision 0)
> > +++ WebCore/platform/graphics/chromium/GLES2Canvas.cpp	(revision 0)
> > +
> > +#include "config.h"
> > +
> > +#if USE(GLES2_RENDERING)
> > +
> > +#include "GLES2Canvas.h"
> > +
> > +#include "FloatRect.h"
> > +#include "GLES2Context.h"
> > +#include "GLES2Texture.h"
> > +#include "OwnArrayPtr.h"
> 
> #include <wtf/OwnArrayPtr.h>, otherwise I'm not sure this will compile on all systems.

Done.

> > +void GLES2Canvas::rotate(float angleInRadians)
> > +{
> > +    m_state->m_ctm.rotate(angleInRadians * (180.0f / 3.14159265f));
> > +}
> 
> Sucks to hard-code the value of pi.

(I blame PlatformContextSkia.cpp:1262).  Fixed.

> > Index: WebCore/platform/graphics/chromium/GLES2Canvas.h
> > ===================================================================
> > --- WebCore/platform/graphics/chromium/GLES2Canvas.h	(revision 0)
> > +++ WebCore/platform/graphics/chromium/GLES2Canvas.h	(revision 0)
> > +class GLES2Canvas : public Noncopyable {
> > +public:
> > +    GLES2Canvas(GLES2Context* context, int width, int height);
> > +    ~GLES2Canvas();
> > +
> > +    void fillRect(const FloatRect& rect, const Color& color,
> > +                  ColorSpace colorSpace);
> > +    void fillRect(const FloatRect& rect);
> > +    void clearRect(const FloatRect& rect);
> > +    void setFillColor(const Color& color, ColorSpace colorSpace);
> > +    void setAlpha(float alpha);
> > +    void setCompositeOperation(CompositeOperator op);
> > +    void translate(float x, float y);
> > +    void rotate(float angleInRadians);
> > +    void scale(const FloatSize& size);
> > +    void concatCTM(const AffineTransform& affine);
> 
> Leave out the parameter names in these declarations unless they are useful (i.e. 'GLES2Context* context' 'FloatRect rect' are useless, 'int width' is useful).

Done.

> > Index: WebCore/platform/graphics/chromium/GLES2Texture.cpp
> > ===================================================================
> > --- WebCore/platform/graphics/chromium/GLES2Texture.cpp	(revision 0)
> > +++ WebCore/platform/graphics/chromium/GLES2Texture.cpp	(revision 0)
> > +
> > +#include "OwnArrayPtr.h"
> 
> #include <wtf/OwnArrayPtr.h>

Done.

> > +void GLES2Texture::load(void* pixels)
> > +{
> > +        glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, m_width, m_height, glFormat, glType, buf.get());
> > +    } else
> > +      glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, m_width, m_height, glFormat, glType, pixels);
> > +}
> 
> Indentation off here

Fixed.

[Stephen White]

Created attachment 62505 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 62505 [details]
Patch

WebCore/platform/graphics/chromium/GLES2Canvas.cpp:211
 +      m_stateStack.removeLast();
debug assert that there is a last to remove?

WebCore/platform/graphics/chromium/GLES2Texture.cpp:126
 +  };
nit: no semi-colon on the closing bracket of a namespace

WebCore/platform/graphics/chromium/GLES2Texture.h:43
 +      ~GLES2Texture();
nit: consider making the destructor private and make RefCounted<GLES2Texture> be a friend.
that way someone cannot accidentally delete a GLES2Texture pointer.

WebCore/platform/graphics/chromium/GLES2Texture.h:59
 +  };
nit: no semi-colon on the closing bracket of a namespace


R=me w/ these nits fixed

[Stephen White]

(In reply to comment #10)
> (From update of attachment 62505 [details])
> WebCore/platform/graphics/chromium/GLES2Canvas.cpp:211
>  +      m_stateStack.removeLast();
> debug assert that there is a last to remove?

Done.

> WebCore/platform/graphics/chromium/GLES2Texture.cpp:126
>  +  };
> nit: no semi-colon on the closing bracket of a namespace

Done.

> WebCore/platform/graphics/chromium/GLES2Texture.h:43
>  +      ~GLES2Texture();
> nit: consider making the destructor private and make RefCounted<GLES2Texture> be a friend.
> that way someone cannot accidentally delete a GLES2Texture pointer.

Good idea, but it doesn't seem to work (although I could be doing something wrong; see below).  It seems that the compiler ignores the friend declaration during template instantiation.  From looking around, it seems that other classes derived from RefCounted<> have public destructors as well.

3>d:\src\chromium1\src\third_party\WebKit\JavaScriptCore\wtf\RefCounted.h(139) : error C2248: 'WebCore::GLES2Texture::~GLES2Texture' : cannot access private member declared in class 'WebCore::GLES2Texture'
3>        d:\src\chromium1\src\third_party\webkit\webcore\platform\graphics\chromium\GLES2Texture.h(58) : compiler has generated 'WebCore::GLES2Texture::~GLES2Texture' here
3>        d:\src\chromium1\src\third_party\webkit\webcore\platform\graphics\chromium\GLES2Texture.h(41) : see declaration of 'WebCore::GLES2Texture'
3>        d:\src\chromium1\src\third_party\WebKit\JavaScriptCore\wtf\RefCounted.h(137) : while compiling class template member function 'void WTF::RefCounted<T>::deref(void)'
3>        with
3>        [
3>            T=WebCore::GLES2Texture
3>        ]
3>        d:\src\chromium1\src\third_party\webkit\webcore\platform\graphics\chromium\GLES2Texture.h(41) : see reference to class template instantiation 'WTF::RefCounted<T>' being compiled
3>        with
3>        [
3>            T=WebCore::GLES2Texture
3>        ]

> 
> WebCore/platform/graphics/chromium/GLES2Texture.h:59
>  +  };
> nit: no semi-colon on the closing bracket of a namespace

Fixed.

> R=me w/ these nits fixed

[Stephen White]

Committed r64046: <http://trac.webkit.org/changeset/64046>

[Darin Fisher (:fishd, Google)]

(In reply to comment #11)
> Good idea, but it doesn't seem to work (although I could be doing something wrong; see below).  It seems that the compiler ignores the friend declaration during template instantiation.  From looking around, it seems that other classes derived from RefCounted<> have public destructors as well.

Did you remember WTF:: in the friend declaration?

  friend class WTF::RefCounted<GLES2Texture>;

We used this trick successfully elsewhere.  See WebViewImpl.h.