[Webkit-unassigned] [Bug 33403] [OpenVG] Add (EGL) surface/context management

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 11 11:39:44 PST 2010


https://bugs.webkit.org/show_bug.cgi?id=33403


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #46283|review-                     |
               Flag|                            |




--- Comment #9 from Nikolas Zimmermann <zimmermann at kde.org>  2010-01-11 11:39:44 PST ---
(From update of attachment 46283)
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 :-)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list