[Bug 768160] qtplugins: How to implement qmlglsrc.

[GStreamer (GNOME Bugzilla)]

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

--- Comment #9 from Haihua Hu <jared.hu at nxp.com> ---
(In reply to Matthew Waters (ystreet00) from comment #8)
> Review of attachment 332082 [details] [review]:
> 
> (In reply to Haihua Hu from comment #6)
> > My design of qmlglsrc has no performance concern. 
> > 
> > 1. In my code, qmlglsrc will warp qt OpenGL context and I make glcontext
> > shared with qt warp context so we can copy texture from scene graph to
> > glmemory which is created in glcontext. This will be very fast because it is
> > operated by GPU. 
> > 
> > 2.When scene graph updated, if downstream has not consumed the old buffer
> > and no new buffer set to qt, just skip this  as we cannot block scene graph.
> 
> This effectively makes you a live source which you'll have to advertise in
> the latency query.
> 
> The moving of the example should be in a separate patch.

Ok, I will separate it into another patch and remove ths unix code.

> 
> ::: ext/qt/gstqtsrc.cc
> @@ +95,3 @@
> +  g_object_class_install_property (gobject_class, PROP_WIDGET,
> +      g_param_spec_pointer ("widget", "QQuickWindow",
> +          "The QQuickWindow to place in the object heirachy",
> 
> Isn't this the other way, i.e. the application needs to set the QQuickWindow
> to use?

Yes, just like the qmlglsink usage but pass QQuickWindow pointer.

> 
> @@ +219,3 @@
> +  }
> +
> +  caps = GST_BASE_SRC_CLASS (parent_class)->get_caps (bsrc, filter);
> 
> This strikes me as odd.  Can you explain why you need to call the base class
> before setting the width/height/...?

I just want to get caps template, so maybe need to use another way to achieve
the goal.

> 
> @@ +228,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);
> 
> par? framerate?

I think framerate is unnecessary since scene graph refresh frequency is not a
constant value at runtime.

> 
> @@ +239,3 @@
> +
> +static GstCaps *
> +gst_qt_src_src_fixate (GstBaseSrc * bsrc, GstCaps * caps)
> 
> empty implementation isn't necessary.
> 
> @@ +513,3 @@
> +        g_print ("qmlglsrc Total refresh frames (%lld), playing for
> (%"GST_TIME_FORMAT"), fps (%.3f).\n",
> +                frame_showed, GST_TIME_ARGS (qt_src->run_time),
> +                (gfloat)GST_SECOND * frame_showed / qt_src->run_time);
> 
> no prints.
> 
> This should be gst debugging if necessary.

I will change it to GST_DEBUG.

> 
> ::: ext/qt/qtitem.cc
> @@ +35,3 @@
>  #include <qpa/qplatformnativeinterface.h>
>  
> +#if GST_GL_HAVE_WINDOW_X11 && GST_GL_HAVE_PLATFORM_EGL && defined
> (HAVE_QT_X11)
> 
> This isn't right and will cause errors on GLX.
> 
> If necessary, this should be a separate patch.

Because our i.mx broad don't support GLX, so this is imx specific and i wiil
move this into another patch for me to use internally.

> 
> ::: ext/qt/qtwindow.cc
> @@ +33,3 @@
> +#include <QOpenGLFramebufferObject>
> +
> +#if GST_GL_HAVE_WINDOW_X11 && GST_GL_HAVE_PLATFORM_EGL && defined
> (HAVE_QT_X11)
> 
> and GLX ?
> 
> @@ +56,3 @@
> +#define LINUX
> +#define EGL_API_FB
> +#include <gst/gl/fb/gstgldisplay_fb.h>
> 
> non-existent header/defines.

This is also imx specific, i will remove it.

> 
> @@ +219,3 @@
> +
> +  if (!gst_video_frame_map (&gl_frame, info, this->priv->buffer,
> +          (GstMapFlags) (GST_MAP_READ | GST_MAP_GL))) {
> 
> Shouldn't this be WRITE | GL?

This maybe coding error.

> 
> @@ +236,3 @@
> +      this->source->renderTargetId(), dst_tex, width,height);
> +
> +  gl->GenFramebuffers (1, &fbo);
> 
> You can keep the fbo around here instead of creating/destroying it every
> frame.

sounds good.

> 
> @@ +245,3 @@
> +
> +  gl->ReadBuffer (GL_COLOR_ATTACHMENT0);
> +  gl->BlitFramebuffer ( 0, 0, width, height,
> 
> glBlitFramebuffer isn't available natively in GLES2 so you'll need an
> alternative, like glCopyTexImage2D

I will try to fix this issue.

> 
> @@ +279,3 @@
> +
> +void
> +QtGLWindow::onSceneGraphInitialized()
> 
> This looks like duplicated code with the sink.  Maybe make a utility
> function to make this easier?

Yes, let's make this another patch next time.

> 
> ::: tests/examples/qt/qmlsrc/main.cpp
> @@ +62,3 @@
> +    sigemptyset(&act.sa_mask);
> +    act.sa_flags = 0;
> +    sigaction(sig, &act, NULL);
> 
> Unix specific code which won't work on Win32 at least.
> 
> Is it even necessary?

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