[webkit-reviews] review canceled: [Bug 33403] [OpenVG] Add (EGL) surface/context management : [Attachment 46283] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 11 11:39:43 PST 2010
Nikolas Zimmermann <zimmermann at kde.org> has canceled review:
Bug 33403: [OpenVG] Add (EGL) surface/context management
https://bugs.webkit.org/show_bug.cgi?id=33403
Attachment 46283: Patch
https://bugs.webkit.org/attachment.cgi?id=46283&action=review
------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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 :-)
More information about the webkit-reviews
mailing list