Il 23/02/2015 18:44, Derek Foreman ha scritto:
-               if (gl_renderer->create(&compositor->base, EGL_DEFAULT_DISPLAY,
+               if (gl_renderer->create(&compositor->base, 0, 
EGL_DEFAULT_DISPLAY,
                                        gl_renderer->opaque_attribs,
                                        NULL) < 0) {

Why is fbdev treated differently than the other compositors?  I guess no
associated platform extension?

Yes exactly, so we fallback to eglGetDisplay.

        if (!c->use_pixman) {
-               if (gl_renderer->create(&c->base, c->parent.wl_display,
-                               gl_renderer->alpha_attribs,
-                               NULL) < 0) {
+               if (!gl_renderer->supports ||
+                   (gl_renderer->supports(&c->base, "EGL_KHR_platform_wayland") < 0 
&&
+                    gl_renderer->supports(&c->base, 
"EGL_EXT_platform_wayland"))) {
+                       weston_log("No support for EGL_KHR_platform_wayland; "
+                                  "falling back to pixman.\n");
+                       c->use_pixman = 1;
+
extra blank line?  ^

I thought it looked clearer, but okay, removed.

+/* returns negative:
+ *  1. if EGL client extensions aren't supported.
+ * returns positive:
+ *  1. if the EGL version is >= 1.5,
+ *  2. if the supplied EGL client extension is supported,
+ *  3. in the fallback case to using eglGetDisplay after logging a
+ *     warning. */

Would prefer to see the above comments in a doxygen format.

OK, done.

+#ifdef EGL_EXT_platform_base
+               get_platform_display =
+                       (void *) eglGetProcAddress("eglGetPlatformDisplayEXT");
+#endif

It's a little counter intuitive that a function called
"gl_renderer_supports" actually does initialization.  Please document it
in the comments above the function (or do it somewhere else :)

Yeah fair, I've put it in gl_renderer_create as that's the first place we need it anyway.

+       weston_log("warning: unable to check for %s support; "
+                  "could fall back to eglGetDisplay.\n", extension);

I kind of don't like having the logging here.  I'd rather see it logged
in the compositor only if we're actually doing the fallback.  My EGL
doesn't support the KHR variants and I found it a little confusing to
see warnings logged despite the EXT variant being available.

Yeah this is a fair point, I've done as you asked.

Another option would be just to pass in the tail of the string (x11,
gbm, wayland) and concatenate it (with EXT, MESA, KHR expecting some not
to exist...) in this function and do all testing here without the
compositor making potentially multiple calls.  Whether that's a
reasonable idea or not is up to you. :)

OK yeah this reduces the code duplication; I've done it. Let's see what anyone else thinks.

(New patch set to follow.)

--
Jonny Lamb
_______________________________________________
wayland-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Reply via email to