[Bug 782923] gl: Add Mesa3D GBM backend

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=782923

Matthew Waters (ystreet00) <ystreet00 at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #352297|none                        |needs-work
             status|                            |

--- Comment #4 from Matthew Waters (ystreet00) <ystreet00 at gmail.com> ---
Review of attachment 352297:
 --> (https://bugzilla.gnome.org/review?bug=782923&attachment=352297)

Looks mostly good.

gstgl_gbm_private.c should not exist.  All the relavant code should be placed
in the other files.  None of this API is exported so everything is private
already.  Some of the functions could go into a gstgl_gbm_utils.c/h if you so
prefer.

::: configure.ac
@@ +1007,3 @@
+    if test "x$HAVE_VIV_FB_EGL" = "xno" -o "x$HAVE_GBM_EGL" = "xno"; then
+      if test "x$HAVE_X11_XCB" = "xno"; then
+        if test "x$HAVE_WAYLAND_EGL" = "xno"; then

Why is this different from the if above?

This effectively does if ((!viv || !gbm) && !X11 && !wayland) which doesn't
seem correct.

@@ +1029,3 @@

+    dnl check Mesa GBM *before* X or Wayland since it is
+    dnl possible that both GBM and X/Wayland are present

Any reason this needs to be performed first?  As in, why is this comment
relevant?

::: gst-libs/gst/gl/gbm/gstgl_gbm_private.c
@@ +117,3 @@
+  }
+
+  // No match found

No C++ comments. We conform to C89.

@@ +459,3 @@
+
+  GST_DEBUG ("Attempting to add GBM BO as scanout framebuffer width/height: %"
+      PRIu32 "/%" PRIu32 " pixels  stride: %" PRIu32 " bytes  format: %s "

use G_G(U)INT*_FORMAT instead of PRI* macros.

@@ +525,3 @@
+    const gchar *devnode = g_udev_device_get_device_file (gudevice);
+
+    if (!g_str_has_prefix (devnode, "/dev/dri/card"))

NULL check? devnode could be NULL and will critical inside g_str_has_prefix().

@@ +640,3 @@
+          " hsync/vsync start %" PRIu16 "/%" PRIu16 " hsync/vsync end %"
PRIu16
+          "/%" PRIu16 " htotal/vtotal %" PRIu16 "/%" PRIu16 " hskew %" PRIu16
+          " vscan %" PRIu16 " vrefresh %" PRIu32 " preferred %d", i,

use G_G(U)INT*_FORMAT instead of PRI* macros

::: gst-libs/gst/gl/gbm/gstgl_gbm_private.h
@@ +68,3 @@
+int gst_gl_gbm_find_and_open_drm_node (void);
+
+gboolean gst_gl_display_gbm_setup_drm (GstGLDisplayGBMPrivate *priv);

Passing the private structure directly isn't exactly kosher from a GObject
standpoint.  This should be the actual GstGLDisplayGBM instance instead and the
private structure retrieved from the instance instead.

::: gst-libs/gst/gl/gbm/gstgldisplay_gbm.h
@@ +61,3 @@
+};
+
+//int gst_gl_display_gbm_find_and_open_drm_node (void);

Any reason why this is still here?

::: gst-libs/gst/gl/gbm/gstglwindow_gbm_egl.c
@@ +90,3 @@
+  /* This function is called in here, and not in the open()
+   * vmethod. The reason for this is explained inside the
+   * gst_gl_window_gbm_init_surface() function. */

_init_surface() should be called from a _create_window() function that is
called from GstGLContextEGL like the 4 other winsys implementations that have
this problem.  Should make a window vfunc one of these days.

@@ +362,3 @@
+
+  window_egl = g_object_new (GST_TYPE_GL_WINDOW_GBM_EGL, NULL);
+  window_egl->priv->display_priv = GST_GL_DISPLAY_GBM_PRIVATE (display);

Again, not idiomatic GObject.

Retrieve the display from the window as needed and then retrieve the private
structure.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.