[Bug 768160] qtplugins: How to implement qmlglsrc.

[GStreamer (GNOME Bugzilla)]

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

--- Comment #29 from Haihua Hu <jared.hu at nxp.com> ---
(In reply to Matthew Waters (ystreet00) from comment #26)
> Review of attachment 332186 [details] [review]:
> 
> Still some more work necessary
> 
> ::: ext/qt/gstqtsrc.cc
> @@ +91,3 @@
> +
> +  g_object_class_install_property (gobject_class, PROP_WIDGET,
> +      g_param_spec_pointer ("widget", "QQuickWindow",
> 
> would window be better instead of widget?

Will change to use window for property name and symbol.

> 
> @@ +134,3 @@
> +    case PROP_WIDGET:
> +      qwindow = static_cast<QQuickWindow *> (g_value_get_pointer (value));
> +      qt_src->widget = new QtGLWindow (NULL, qwindow);
> 
> What if we set widget more than once?  This will leak.

will add judgment here to avoid memory leak.

> 
> @@ +153,3 @@
> +  switch (prop_id) {
> +    case PROP_WIDGET:
> +      g_value_set_pointer (value, qt_src->widget);
> 
> This doesn't retrieve what was set in set_property()
> 
> @@ +230,3 @@
> +      GstStructure *s = gst_caps_get_structure (temp, i);
> +      gst_structure_set (s, "width", G_TYPE_INT, width, NULL);
> +      gst_structure_set (s, "height", G_TYPE_INT, height, NULL);
> 
> framerate? should be 0/1 if unknown.  pixel-aspect-ratio should also be set
> to 1/1
> 
> ::: ext/qt/qtwindow.cc
> @@ +33,3 @@
> +#include <QOpenGLFramebufferObject>
> +
> +#if GST_GL_HAVE_WINDOW_X11 && GST_GL_HAVE_PLATFORM_EGL && defined
> (HAVE_QT_X11)
> 
> EGL here, should be GLX

Sorry for that.

> 
> @@ +56,3 @@
> +#define LINUX
> +#define EGL_API_FB
> +#include <gst/gl/fb/gstgldisplay_fb.h>
> 
> This is still here
> 
> @@ +79,3 @@
> +  GstGLDisplay *display;
> +  GstGLContext *other_context;
> +  guint64 frame_showed;
> 
> frames_shown or more correctly, frames_grabbed?
> 
> @@ +102,3 @@
> +  this->priv->quit = FALSE;
> +  this->priv->useDefaultFbo = FALSE;
> +  this->priv->frame_showed = 0;
> 
> 0-initializing all these private variables isn't necessary, g_new0 already
> does that.
> 
> @@ +167,3 @@
> +
> +  g_mutex_lock (&this->priv->lock);
> +  gst_gl_context_activate (this->priv->other_context, TRUE);
> 
> Are these really necessary?
> 
> You don't actually use anything that would need/have access to
> this->priv->other_context
> 
> @@ +182,3 @@
> +  }
> +
> +  gst_gl_context_activate (this->priv->other_context, FALSE);
> 
> see above.

will remove this two lines.

> 
> @@ +271,3 @@
> +
> +void
> +QtGLWindow::onSceneGraphInitialized()
> 
> This winsys initialization is out of date with qmlglsink and may fail on
> windows.
> 
> Please make a utility function from qmlglsink's version and use that here as
> well.

Let's add a new file for utility functions.

> 
> @@ +335,3 @@
> +  }
> +#endif
> +#if GST_GL_HAVE_WINDOW_FB
> 
> Still here.

Remove it.

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