Bug 33403 – [OpenVG] Add (EGL) surface/context management

• Component: Platform
• Status: RESOLVED FIXED

[Jakob Petsovits]

[OpenVG] Add (EGL) surface/context management

[Jakob Petsovits]

Created attachment 46161 [details]
Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46161 [details] without any errors.

[Jakob Petsovits]

A new port is coming up! I finally got the patches rebased by topic and the lawyers' OK, and this is the first patch in my upcoming series of patches adding OpenVG as a graphics backend. Whoo! Also the first time I'm trying out webkit-patch/bugzilla-tool.

Admittedly also, this patch is not among the nicest patches in that series, which mostly stems from the fact that WebKit likes to keep resources like images, fonts and paths around (garbage collecting them later) and also does not currently pass context information to every graphics-related object there is. In part, EGLDisplayOpenVG is designed to work around these issues; by upstreaming the OpenVG port I also hope to find a cleaner solution to those issues eventually.

The nicer parts take care of using only a single EGL context for different EGL surfaces where available, and provide a nice & clean interface for higher-level code that actually uses OpenVG calls. I'll wait for opinions for now while preparing the next couple of patches so you can get an idea what that is going to look like :-)

[Adam Treat]

Comment on attachment 46161 [details]
Patch

EGLDisplayOpenVG ctor and dtor are marked as public.  These should be private as the static methods control creation.

[Jakob Petsovits]

Created attachment 46283 [details]
Patch

[Jakob Petsovits]

Above patch puts constructor and destructor of EGLDisplayOpenVG into the private section. Good catch.

[Adam Treat]

Comment on attachment 46283 [details]
Patch

Looks good. r=me.

[Adam Treat]

Comment on attachment 46283 [details]
Patch

WildFox has further comments.

[Nikolas Zimmermann]

Comment on attachment 46283 [details]
Patch

Some small comments:

> diff --git a/WebCore/platform/graphics/openvg/EGLDisplayOpenVG.cpp b/WebCore/platform/graphics/openvg/EGLDisplayOpenVG.cpp
> new file mode 100644
> index 0000000..5f0357e
> --- /dev/null
> +++ b/WebCore/platform/graphics/openvg/EGLDisplayOpenVG.cpp
> @@ -0,0 +1,385 @@
> +/*
> + * Copyright (C) Research In Motion Limited 2009. All rights reserved.
Update copyright to include 2010.

> +
> +// File-static variables.
> +static WTF::HashMap<EGLDisplay, EGLDisplayOpenVG*>* s_displayManagers = 0;
> +static EGLDisplayOpenVG* s_current = 0;
Please use static helper functions, containing DEFINE_STATIC_LOCAL macros, see existing code for examples. This is the preferred WebKit style.

> +void EGLDisplayOpenVG::registerPlatformSurface(SurfaceOpenVG* platformSurface)
> +{
> +    EGLDisplayOpenVG* displayManager = EGLDisplayOpenVG::forDisplay(platformSurface->eglDisplay());
> +    displayManager->m_platformSurfaces.set(platformSurface->eglSurface(), platformSurface);
> +}
> +
> +void EGLDisplayOpenVG::unregisterPlatformSurface(SurfaceOpenVG* platformSurface)
> +{
> +    EGLDisplayOpenVG* displayManager = EGLDisplayOpenVG::forDisplay(platformSurface->eglDisplay());
> +    displayManager->m_platformSurfaces.remove(platformSurface->eglSurface());
> +}

It would be certainly nicer, to have inlined helper functions in EGLDisplayOpenVG, hiding the access to m_platformSurfaces.
Would be great, if there's no need to operatore on other classes member variables.

> +EGLDisplayOpenVG* EGLDisplayOpenVG::forDisplay(EGLDisplay display)
> +{
> +    if (!s_displayManagers)
> +        s_displayManagers = new WTF::HashMap<EGLDisplay, EGLDisplayOpenVG*>();
> +
> +    if (!s_displayManagers->contains(display))
> +        s_displayManagers->set(display, new EGLDisplayOpenVG(display));
> +
> +    return s_displayManagers->get(display);
> +}
Omit WTF:: prefixes, not needed.
Use DEFINE_STATIC_LOCAL for s_displayManagers.

> +EGLDisplayOpenVG::~EGLDisplayOpenVG()
> +{
> +    eglMakeCurrent(m_display, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT);
> +    ASSERT_EGL_NO_ERROR();
> +
> +    delete m_sharedPlatformSurface;
> +
> +    WTF::HashMap<EGLSurface, EGLint>::const_iterator end = m_surfaceConfigIds.end();
> +    for (WTF::HashMap<EGLSurface, EGLint>::const_iterator it = m_surfaceConfigIds.begin(); it != end; ++it)
> +        destroySurface((*it).first);
> +
> +    eglTerminate(m_display);
> +    ASSERT_EGL_NO_ERROR();
> +}
Omit WTF:: prefixes. Won't repeat this again, just remove them everywhere.

> +void EGLDisplayOpenVG::setDefaultPbufferConfig(EGLConfig config)
How about passing const-references?

> +void EGLDisplayOpenVG::setDefaultWindowConfig(EGLConfig config)
Ditto,

> +EGLSurface EGLDisplayOpenVG::createPbufferSurface(const IntSize& size, EGLConfig config)
> +{
> +    if (!config)
> +        config = defaultPbufferConfig();
Oh EGLConfig, contains operator==? This looks unsafe, on first sight.

> +
> +    m_surfaceConfigIds.set(surface, surfaceConfigId);
How about adding an assertion, that surfaceConfigId, is not yet set?

> +EGLSurface EGLDisplayOpenVG::surfaceForWindow(EGLNativeWindowType wId, const EGLConfig& config)
..
> +    m_surfaceConfigIds.set(surface, surfaceConfigId);
Same here.

> +bool EGLDisplayOpenVG::surfacesCompatible(EGLSurface surface, EGLSurface otherSurface)
Const-references?

> +void EGLDisplayOpenVG::destroySurface(EGLSurface surface)
Ditto.

> +    WTF::HashMap<EGLNativeWindowType, EGLSurface>::iterator end = m_windowSurfaces.end();
> +    for (WTF::HashMap<EGLNativeWindowType, EGLSurface>::iterator it = m_windowSurfaces.begin(); it != end; ++it) {
> +        if ((*it).second == surface)
> +            m_windowSurfaces.remove(it);
> +    }
Hmm, It seems to me a 'break' is misssing here?

> +EGLContext EGLDisplayOpenVG::contextForSurface(EGLSurface surface)
Const-references?

> +    m_contexts.set(surfaceConfigId, context);
Assertion regarding existance of surfaceConfigId?

> diff --git a/WebCore/platform/graphics/openvg/EGLDisplayOpenVG.h b/WebCore/platform/graphics/openvg/EGLDisplayOpenVG.h
> new file mode 100644
> index 0000000..7e2127d
> --- /dev/null
> +++ b/WebCore/platform/graphics/openvg/EGLDisplayOpenVG.h
> @@ -0,0 +1,88 @@
> +/*
> + * Copyright (C) Research In Motion Limited 2009. All rights reserved.
2010 is missing.

> +    friend class SurfaceOpenVG;
Would be great to remove the friendship, by modifying the access to the m_platformSurfaces map.

> +    static SurfaceOpenVG* currentSurface();
> +    static void setCurrentDisplay(EGLDisplay display);
> +    static EGLDisplayOpenVG* current();
> +    static EGLDisplayOpenVG* forDisplay(EGLDisplay display);
> +
> +    void setDefaultPbufferConfig(EGLConfig config);
> +    EGLConfig defaultPbufferConfig();
> +    void setDefaultWindowConfig(EGLConfig config);
> +    EGLConfig defaultWindowConfig();
Omit argument names here.

> +
> +    EGLDisplay display() const { return m_display; }
> +    SurfaceOpenVG* sharedPlatformSurface();
> +
> +    /** Creates a pbuffer surface using the given config. If no surface
> +        * could be created, EGL_NO_SURFACE is returned and errors can be
> +        * caught with eglGetError() (respectively ASSERT_EGL_NO_ERROR()). */
Not lined up correctly.

> +    EGLSurface createPbufferSurface(const IntSize& size, EGLConfig config = 0);
> +
> +    EGLSurface surfaceForWindow(EGLNativeWindowType wId, const EGLConfig& config);
> +
> +    bool surfacesCompatible(EGLSurface surface1, EGLSurface otherSurface);
> +
> +    /** Destroy the surface and its corresponding context (unless another
> +     * surface is still using the same context, in which case the context
> +     * is not destroyed). */
> +    void destroySurface(EGLSurface surface);
> +
> +    /** Return the context corresponding to the surface.
> +     * If no corresponding context exists, one is created automatically. */
> +    EGLContext contextForSurface(EGLSurface surface);
Omit argument names.

> diff --git a/WebCore/platform/graphics/openvg/EGLUtils.h b/WebCore/platform/graphics/openvg/EGLUtils.h
> new file mode 100644
> index 0000000..0851b70
> --- /dev/null
> +++ b/WebCore/platform/graphics/openvg/EGLUtils.h
> @@ -0,0 +1,53 @@
> +/*
> + * Copyright (C) Research In Motion Limited 2009. All rights reserved.
Needs an update.

> +#define ASSERT_EGL_NO_ERROR() \
> +    do { \
> +        EGLint error = eglGetError(); \
> +        if (error != EGL_SUCCESS) { \
> +            const char* txt; \
> +            switch (error) { \
> +            case EGL_NOT_INITIALIZED: txt = "EGL_NOT_INITIALIZED"; break;\
> +            case EGL_BAD_ACCESS: txt = "EGL_BAD_ACCESS"; break;\
> +            case EGL_BAD_ALLOC: txt = "EGL_BAD_ALLOC"; break;\
> +            case EGL_BAD_ATTRIBUTE: txt = "EGL_BAD_ATTRIBUTE"; break;\
> +            case EGL_BAD_CONTEXT: txt = "EGL_BAD_CONTEXT"; break;\
> +            case EGL_BAD_CONFIG: txt = "EGL_BAD_CONFIG"; break;\
> +            case EGL_BAD_CURRENT_SURFACE: txt = "EGL_BAD_CURRENT_SURFACE"; break;\
> +            case EGL_BAD_DISPLAY: txt = "EGL_BAD_DISPLAY"; break;\
> +            case EGL_BAD_SURFACE: txt = "EGL_BAD_SURFACE"; break;\
> +            case EGL_BAD_MATCH: txt = "EGL_BAD_MATCH"; break;\
> +            case EGL_BAD_PARAMETER: txt = "EGL_BAD_PARAMETER"; break;\
> +            case EGL_BAD_NATIVE_PIXMAP: txt = "EGL_BAD_NATIVE_PIXMAP"; break;\
> +            case EGL_BAD_NATIVE_WINDOW: txt = "EGL_BAD_NATIVE_WINDOW"; break;\
> +            case EGL_CONTEXT_LOST: txt = "EGL_CONTEXT_LOST"; break;\
> +            default: txt = "UNKNOWN_ERROR"; break;\
> +            } \
> +            WTFReportAssertionFailureWithMessage(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, "", "Found %s", txt); \
> +            CRASH(); \
> +        } \
> +    } while (0)

Eww, this is an ugly part of the patch :-)
Can you move this to a static inline helper function?

> +
> +#endif // EGLUtils_h
This is deprecated, we're leaving out the // Foo_h parts nowadays.

> diff --git a/WebCore/platform/graphics/openvg/SurfaceOpenVG.cpp b/WebCore/platform/graphics/openvg/SurfaceOpenVG.cpp
> new file mode 100644
> index 0000000..40e9538
> --- /dev/null
> +++ b/WebCore/platform/graphics/openvg/SurfaceOpenVG.cpp
> @@ -0,0 +1,183 @@
> +/*
> + * Copyright (C) Research In Motion Limited 2009. All rights reserved.
Needs an update.

> +SurfaceOpenVG* SurfaceOpenVG::currentSurface()
> +{
> +#if PLATFORM(EGL)
> +    return EGLDisplayOpenVG::currentSurface();
> +#endif
> +}
This will generate an compilation error w/o PLATFORM(EGL). At least add return 0 as fallback.

> +
> +#if PLATFORM(EGL)
> +SurfaceOpenVG::SurfaceOpenVG(const IntSize& size, EGLDisplay display, EGLConfig config)
> +    , m_eglDisplay(display)
> +{
> +    ASSERT(m_eglDisplay != EGL_NO_DISPLAY);
> +
> +    EGLDisplayOpenVG* displayManager = EGLDisplayOpenVG::forDisplay(m_eglDisplay);
> +    if (!config)
> +        config = displayManager->defaultPbufferConfig();
> +
> +    m_eglSurface = displayManager->createPbufferSurface(size, config);
> +    if (m_eglSurface == EGL_NO_SURFACE)
> +        return;
> +
> +    m_eglContext = displayManager->contextForSurface(m_eglSurface);
> +    EGLDisplayOpenVG::registerPlatformSurface(this);
Hm, the 'early return' leaves m_eglContext uninitialized. Maybe use default initializers for all, to be safe than sorry.

> +SurfaceOpenVG::SurfaceOpenVG(EGLNativeWindowType window, EGLDisplay display, EGLConfig config)
> +    , m_eglDisplay(display)
> +{
> +    ASSERT(m_eglDisplay != EGL_NO_DISPLAY);
> +
> +    EGLDisplayOpenVG* displayManager = EGLDisplayOpenVG::forDisplay(m_eglDisplay);
> +    if (!config)
> +        config = displayManager->defaultWindowConfig();
> +
> +    m_eglSurface = displayManager->surfaceForWindow(window, config);
> +    if (m_eglSurface == EGL_NO_SURFACE)
> +        return;
> +
> +    m_eglContext = displayManager->contextForSurface(m_eglSurface);
> +    EGLDisplayOpenVG::registerPlatformSurface(this);
> +}
Same comment as above.

> +bool SurfaceOpenVG::isValid() const
> +{
> +#if PLATFORM(EGL)
> +    return (m_eglSurface != EGL_NO_SURFACE);
> +#endif
> +}
> +
> +int SurfaceOpenVG::width() const
> +{
> +#if PLATFORM(EGL)
> +    ASSERT(m_eglSurface != EGL_NO_SURFACE);
> +
> +    EGLint width;
> +    eglQuerySurface(m_eglDisplay, m_eglSurface, EGL_WIDTH, &width);
> +    ASSERT_EGL_NO_ERROR();
> +    return width;
> +#endif
> +}
> +
> +int SurfaceOpenVG::height() const
> +{
> +#if PLATFORM(EGL)
> +    ASSERT(m_eglSurface != EGL_NO_SURFACE);
> +
> +    EGLint height;
> +    eglQuerySurface(m_eglDisplay, m_eglSurface, EGL_HEIGHT, &height);
> +    ASSERT_EGL_NO_ERROR();
> +    return height;
> +#endif
> +}
> +
> +SurfaceOpenVG* SurfaceOpenVG::sharedSurface() const
> +{
> +#if PLATFORM(EGL)
> +    ASSERT(m_eglSurface != EGL_NO_SURFACE);
> +    return EGLDisplayOpenVG::forDisplay(m_eglDisplay)->sharedPlatformSurface();
> +#endif

Ouch, all these methods need fallbacks for non-egl builds. (Not that it would make any sense to compile these things w/o EGL, it's still not a good style)

> diff --git a/WebCore/platform/graphics/openvg/SurfaceOpenVG.h b/WebCore/platform/graphics/openvg/SurfaceOpenVG.h
> new file mode 100644
> index 0000000..be76ff0
> --- /dev/null
> +++ b/WebCore/platform/graphics/openvg/SurfaceOpenVG.h
> @@ -0,0 +1,120 @@
> +/*
> + * Copyright (C) Research In Motion Limited 2009. All rights reserved.
Needs an update.

> +
> +#include <wtf/Platform.h>
> +
> +#if PLATFORM(EGL)
> +#include <egl.h>
> +#endif

Swap include order, to avoid a warning from the style bot.

> +#if PLATFORM(EGL)
> +class EGLDisplayOpenVG;
> +#endif
No need to guard class forwards.

> +class SurfaceOpenVG {
This class seems to be used as heap object only, so it might be a wise idea to inherit from Noncopyable.


> +public:
> +    static SurfaceOpenVG* currentSurface();
> +
> +#if PLATFORM(EGL)
> +    friend class EGLDisplayOpenVG;
As I mentioned above, usually friendships should be avoided, unless there's an intrinsic reason.

> +    SurfaceOpenVG(const IntSize& size, EGLDisplay display, EGLConfig config = 0);
> +    SurfaceOpenVG(EGLNativeWindowType window, EGLDisplay display, EGLConfig config = 0);
Omit argument names here.

> +} // namespace WebCore
> +
> +#endif // SurfaceOpenVG_h

The trailing // comments are deprecated nowadays. Please remove.

> diff --git a/WebCore/platform/graphics/openvg/VGUtils.h b/WebCore/platform/graphics/openvg/VGUtils.h
> new file mode 100644
> index 0000000..9b55de9
> --- /dev/null
> +++ b/WebCore/platform/graphics/openvg/VGUtils.h
> @@ -0,0 +1,47 @@
> +/*
> + * Copyright (C) Research In Motion Limited 2009. All rights reserved.
Needs an update.
> +
> +#define ASSERT_VG_NO_ERROR() \
> +    do { \
> +        VGErrorCode error = vgGetError(); \
> +        if (error != VG_NO_ERROR) { \
> +            const char* txt; \
> +            switch (error) { \
> +            case VG_BAD_HANDLE_ERROR: txt = "VG_BAD_HANDLE_ERROR"; break;\
> +            case VG_ILLEGAL_ARGUMENT_ERROR: txt = "VG_ILLEGAL_ARGUMENT_ERROR"; break;\
> +            case VG_OUT_OF_MEMORY_ERROR: txt = "VG_OUT_OF_MEMORY_ERROR"; break;\
> +            case VG_PATH_CAPABILITY_ERROR: txt = "VG_PATH_CAPABILITY_ERROR"; break;\
> +            case VG_UNSUPPORTED_IMAGE_FORMAT_ERROR: txt = "VG_UNSUPPORTED_IMAGE_FORMAT_ERROR"; break;\
> +            case VG_UNSUPPORTED_PATH_FORMAT_ERROR: txt = "VG_UNSUPPORTED_PATH_FORMAT_ERROR"; break;\
> +            case VG_IMAGE_IN_USE_ERROR: txt = "VG_IMAGE_IN_USE_ERROR"; break;\
> +            case VG_NO_CONTEXT_ERROR: txt = "VG_NO_CONTEXT_ERROR"; break;\
> +            default: txt = "UNKNOWN_ERROR"; break;\
> +            } \
> +            WTFReportAssertionFailureWithMessage(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, "", "Found %s", txt); \
> +            CRASH(); \
> +        } \
> +    } while (0)
> +
Would be great to move this in a static inline helper function, as well.

Sorry for the long list of comments :-)

[Jakob Petsovits]

Created attachment 46498 [details]
Patch

[Jakob Petsovits]

(In reply to comment #9)

Lots of good points there. The updated patch should take care of most of them, but I have some questions or remarks on the remaining ones.

> > +
> > +// File-static variables.
> > +static WTF::HashMap<EGLDisplay, EGLDisplayOpenVG*>* s_displayManagers = 0;
> > +static EGLDisplayOpenVG* s_current = 0;
> Please use static helper functions, containing DEFINE_STATIC_LOCAL macros, see
> existing code for examples. This is the preferred WebKit style.

I used it for s_displayManagers now, but s_current is still a regular file-static variable as it contains an actual pointer that can change, rather than a persistent static object. Using DEFINE_STATIC_LOCAL for s_current would mean returning a double-pointer from its helper function, and that's clumsy and less readable.

> > +void EGLDisplayOpenVG::registerPlatformSurface(SurfaceOpenVG* platformSurface)
> > +{
> > +    EGLDisplayOpenVG* displayManager = EGLDisplayOpenVG::forDisplay(platformSurface->eglDisplay());
> > +    displayManager->m_platformSurfaces.set(platformSurface->eglSurface(), platformSurface);
> > +}
> > +
> > +void EGLDisplayOpenVG::unregisterPlatformSurface(SurfaceOpenVG* platformSurface)
> > +{
> > +    EGLDisplayOpenVG* displayManager = EGLDisplayOpenVG::forDisplay(platformSurface->eglDisplay());
> > +    displayManager->m_platformSurfaces.remove(platformSurface->eglSurface());
> > +}
> 
> It would be certainly nicer, to have inlined helper functions in
> EGLDisplayOpenVG, hiding the access to m_platformSurfaces.
> Would be great, if there's no need to operatore on other classes member
> variables.

Certainly possible, although I'm really not convinced of the value or readability of two methods that perform a single-liner each. Mind that the functions above don't actually operate on other classes' member variables, both the (static) methods and the m_platformSurfaces variable belong to EGLDisplayOpenVG itself. Imho a single class should know how to deal with its own member variables without requiring trivial wrapper methods, even from static methods.

> > +void EGLDisplayOpenVG::setDefaultPbufferConfig(EGLConfig config)
> How about passing const-references?
>
> > +void EGLDisplayOpenVG::setDefaultWindowConfig(EGLConfig config)
> Ditto,
> 
> > +EGLSurface EGLDisplayOpenVG::createPbufferSurface(const IntSize& size, EGLConfig config)
> > +{
> > +    if (!config)
> > +        config = defaultPbufferConfig();
> Oh EGLConfig, contains operator==? This looks unsafe, on first sight.

This point was kinda ambiguous to me. Usually all EGL (and VG) handles are defined as void* or similar, so in real-world EGL implementations you wouldn't actually pass an actual instance as is. Same thing with operator==(), which is not taken into account because config is actually a pointer.

That said, I looked it up in the EGL specification (1.2, which is the minimum required version for this code) and it doesn't specify that types like EGLConfig, EGLSurface etc. are actually primitive types, so theoretically implementations could still define these types as actual class types. I reworked some pieces involving EGLConfig objects so that the code now relies more on config ids (being plain integers) internally, instead of actual config objects. Parameters have been constified where appropriate.

> > diff --git a/WebCore/platform/graphics/openvg/EGLDisplayOpenVG.h b/WebCore/platform/graphics/openvg/EGLDisplayOpenVG.h
> > new file mode 100644
> > index 0000000..7e2127d
> > --- /dev/null
> > +++ b/WebCore/platform/graphics/openvg/EGLDisplayOpenVG.h
> > +    friend class SurfaceOpenVG;
> Would be great to remove the friendship, by modifying the access to the
> m_platformSurfaces map.

I'm not sure what exactly you've got in mind here, please detail your suggestion. The purpose of this friendship is to enable SurfaceOpenVG to [un]register in the m_platformSurfaces map, and I wanted to make it very clear that the API user is not supposed to call [un]registerPlatformSurface() by herself. The friendship enables me to enforce that, whereas if I make those two methods public then there's the chance that somebody will misunderstand the API and call it manually.

Personally, I think the gain in safety towards outside users is more important than  removing a friendship between two classes that know each other very well and need to interact tightly anyways.

> > diff --git a/WebCore/platform/graphics/openvg/EGLUtils.h b/WebCore/platform/graphics/openvg/EGLUtils.h
> > new file mode 100644
> > index 0000000..0851b70
> > --- /dev/null
> > +++ b/WebCore/platform/graphics/openvg/EGLUtils.h
> > @@ -0,0 +1,53 @@
> > +/*
> > + * Copyright (C) Research In Motion Limited 2009. All rights reserved.
> Needs an update.

There's now a 2010 copyright in all the files because I updated them with your suggestions (and consider them "significant changes" in each), but I wanted to stress that copyright applies to the year when the code was written, not when it was released to the public. Therefore, the 2009 mentions above were correct in the initial patch.

I also noticed that I forgot to add them for EGLUtils/VGUtils in the new patch, but have them in my working copy, so they will be going into any subsequent patches or landings. I won't bother uploading a whole new patch just for that minor detail when there's good chances it'll still be modified anyways.

> > +#define ASSERT_EGL_NO_ERROR() \
> > +    do { \
> > +        EGLint error = eglGetError(); \
> > +        if (error != EGL_SUCCESS) { \
> > +            const char* txt; \
> > +            switch (error) { \
> > +            case EGL_NOT_INITIALIZED: txt = "EGL_NOT_INITIALIZED"; break;\
> > +            case EGL_BAD_ACCESS: txt = "EGL_BAD_ACCESS"; break;\
> > +            case EGL_BAD_ALLOC: txt = "EGL_BAD_ALLOC"; break;\
> > +            case EGL_BAD_ATTRIBUTE: txt = "EGL_BAD_ATTRIBUTE"; break;\
> > +            case EGL_BAD_CONTEXT: txt = "EGL_BAD_CONTEXT"; break;\
> > +            case EGL_BAD_CONFIG: txt = "EGL_BAD_CONFIG"; break;\
> > +            case EGL_BAD_CURRENT_SURFACE: txt = "EGL_BAD_CURRENT_SURFACE"; break;\
> > +            case EGL_BAD_DISPLAY: txt = "EGL_BAD_DISPLAY"; break;\
> > +            case EGL_BAD_SURFACE: txt = "EGL_BAD_SURFACE"; break;\
> > +            case EGL_BAD_MATCH: txt = "EGL_BAD_MATCH"; break;\
> > +            case EGL_BAD_PARAMETER: txt = "EGL_BAD_PARAMETER"; break;\
> > +            case EGL_BAD_NATIVE_PIXMAP: txt = "EGL_BAD_NATIVE_PIXMAP"; break;\
> > +            case EGL_BAD_NATIVE_WINDOW: txt = "EGL_BAD_NATIVE_WINDOW"; break;\
> > +            case EGL_CONTEXT_LOST: txt = "EGL_CONTEXT_LOST"; break;\
> > +            default: txt = "UNKNOWN_ERROR"; break;\
> > +            } \
> > +            WTFReportAssertionFailureWithMessage(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, "", "Found %s", txt); \
> > +            CRASH(); \
> > +        } \
> > +    } while (0)
> 
> Eww, this is an ugly part of the patch :-)
> Can you move this to a static inline helper function?

I have a workable solution now as discussed on IRC, hope this one's ok for you :)

> > +SurfaceOpenVG* SurfaceOpenVG::currentSurface()
> > +{
> > +#if PLATFORM(EGL)
> > +    return EGLDisplayOpenVG::currentSurface();
> > +#endif
> > +}
> This will generate an compilation error w/o PLATFORM(EGL). At least add return 0 as fallback.

I disagree. This was done intentionally, because I strongly believe that code that is known not to work should fail as soon as possible, in this case as compiler warning or error. We know that without a context/surface backend, OpenVG will fail terribly, if there's a compiler error then that's a direct pointer to the programmer that says "you need to implement this before your code can possibly work".

Apart from adding unused lines to the file, an #else path also gives the impression that there is an actual alternative, and makes it harder to find the places that need to be filled out for non-EGL ports. I am not convinced that making style improvements (are they actual improvements really?) is worth dropping the advantages of well-placed compiler errors.

> > diff --git a/WebCore/platform/graphics/openvg/SurfaceOpenVG.h b/WebCore/platform/graphics/openvg/SurfaceOpenVG.h
> > new file mode 100644
> > index 0000000..be76ff0
> > --- /dev/null
> > +++ b/WebCore/platform/graphics/openvg/SurfaceOpenVG.h
> > +#if PLATFORM(EGL)
> > +class EGLDisplayOpenVG;
> > +#endif
> No need to guard class forwards.

But also no harm? Putting this in an ifdef tells the reader at first glance that this is not relevant for anything other than EGL, and is also handy when I grep for PLATFORM(EGL) in order to find all the EGL dependencies.

> > +public:
> > +    static SurfaceOpenVG* currentSurface();
> > +
> > +#if PLATFORM(EGL)
> > +    friend class EGLDisplayOpenVG;
> As I mentioned above, usually friendships should be avoided, unless there's an
> intrinsic reason.

Same as with the other friendship I explained above - I want to make really sure no one will be calling the constructor that only EGLDisplayOpenVG is going to call. (And that one is necessary because EGLDisplayOpenVG creates the first, shared surface which is the only one that does not refer to another surface for sharing resources.) Friendships are usually to be avoided, but I still find them nicer than API users being able to access stuff that they really should not have access to.

> > +    SurfaceOpenVG(const IntSize& size, EGLDisplay display, EGLConfig config = 0);
> > +    SurfaceOpenVG(EGLNativeWindowType window, EGLDisplay display, EGLConfig config = 0);
> Omit argument names here.

I think those argument names make a good point of what's the difference between those two constructors, and therefore are good for readability. I left them in the updated patch, but if you do insist on it, I can take them out, it's not something I'm going to fight about.

> Sorry for the long list of comments :-)

That's alright, it's great to get a thorough review :)

[Jakob Petsovits]

Created attachment 46710 [details]
Patch

[Jakob Petsovits]

Spoke to Niko on IRC, the above patch goes a middle way between his original comments and my replies and should hopefully satisfy Niko's demands.

[Jakob Petsovits]

Created attachment 46930 [details]
Patch

[Jakob Petsovits]

Yay, more style fixes suggested by Niko! By now it should really be ok, I hope. Thanks for the reviews!

[Nikolas Zimmermann]

Comment on attachment 46930 [details]
Patch

Looks great, some small style fixes remaining, please fix before landing:


> +        static const EGLint configAttribs[] = {
> +            EGL_RED_SIZE, 8,
....
> +            EGL_NONE };

The }; needs to go in another line, aligned with the "static const" prefix.
There are several places in the patch that need to be fixed.


> +    WTF::HashMap<EGLSurface, SurfaceOpenVG*> m_platformSurfaces;
> +    WTF::HashMap<EGLNativeWindowType, EGLSurface> m_windowSurfaces;
> +    WTF::HashMap<EGLSurface, EGLint> m_surfaceConfigIds;
> +    WTF::HashMap<EGLint, EGLint> m_compatibleConfigIds;
> +    WTF::HashMap<EGLint, EGLContext> m_contexts;

Remove WTF:: prefixes. HashMap uses "using WTF::HashMap", so it's available in all namespaces.
Thanks for the patience... r=me.

[Jakob Petsovits]

Committed r53499: <http://trac.webkit.org/changeset/53499>

[Jakob Petsovits]

Alright! OpenVG stuff to be continued in bug 33405. See you there!