[Bug 782923] gl: Add Mesa3D GBM backend
GStreamer (GNOME Bugzilla)
bugzilla at gnome.org
Tue Aug 15 07:32:23 UTC 2017
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.
More information about the gstreamer-bugs
mailing list