[Webkit-unassigned] [Bug 33403] [OpenVG] Add (EGL) surface/context management
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jan 13 14:48:25 PST 2010
https://bugs.webkit.org/show_bug.cgi?id=33403
--- Comment #11 from Jakob Petsovits <jpetsovits at rim.com> 2010-01-13 14:48:25 PST ---
(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 :)
--
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