Bug 42905 - Implement OpenGLES2 helper classes.
Summary: Implement OpenGLES2 helper classes.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-23 13:08 PDT by Stephen White
Modified: 2010-07-26 10:02 PDT (History)
4 users (show)

See Also:


Attachments
Patch (39.86 KB, patch)
2010-07-23 13:26 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (32.42 KB, patch)
2010-07-23 15:21 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (31.92 KB, patch)
2010-07-23 15:37 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (31.79 KB, patch)
2010-07-24 09:40 PDT, Stephen White
fishd: review+
fishd: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2010-07-23 13:08:00 PDT
Implement OpenGLES2 helper classes.
Comment 1 Stephen White 2010-07-23 13:26:57 PDT
Created attachment 62458 [details]
Patch
Comment 2 James Robinson 2010-07-23 13:43:51 PDT
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!
Comment 3 WebKit Review Bot 2010-07-23 14:03:50 PDT
Attachment 62458 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3566409
Comment 4 James Robinson 2010-07-23 14:07:30 PDT
The .h has its contents behind #if guards but the cpp isn't guarded.
Comment 5 Stephen White 2010-07-23 15:21:38 PDT
Created attachment 62467 [details]
Patch
Comment 6 Stephen White 2010-07-23 15:37:37 PDT
Created attachment 62468 [details]
Patch
Comment 7 James Robinson 2010-07-23 15:47:53 PDT
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
Comment 8 Stephen White 2010-07-24 09:39:11 PDT
(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.
Comment 9 Stephen White 2010-07-24 09:40:00 PDT
Created attachment 62505 [details]
Patch
Comment 10 Darin Fisher (:fishd, Google) 2010-07-25 20:46:27 PDT
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
Comment 11 Stephen White 2010-07-26 08:26:57 PDT
(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
Comment 12 Stephen White 2010-07-26 08:44:21 PDT
Committed r64046: <http://trac.webkit.org/changeset/64046>
Comment 13 Darin Fisher (:fishd, Google) 2010-07-26 10:02:20 PDT
(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.