qmlglsrc as a screen grabber of the qml scene graph is in general a bad idea for performance reasons as you have to perform a readback (bad) of the GL front buffer (also bad).  If possible, you should attempt to retrieve your rendering before qmlglsink and pass that downstream.

That being said, it's possible that screen grabbing may be your only option so something like that would be useful.

(In reply to Matthew Waters (ystreet00) from comment #1)
> qmlglsrc as a screen grabber of the qml scene graph is in general a bad idea
> for performance reasons as you have to perform a readback (bad) of the GL
> front buffer (also bad).  If possible, you should attempt to retrieve your
> rendering before qmlglsink and pass that downstream.
> 
> That being said, it's possible that screen grabbing may be your only option
> so something like that would be useful.

I'm now implementing this plugin which is only for grabing scene graph. But I met a problem in unit test. pipeline for unit test is :

qmlglsrc ! glimagesink

There are two important threads in unit test that thread-1 is scene graph render thread and thread-2 is glimagesink rendering thread. thread-1 warp qt scene graph context into glcontext, glimagesink will query app_context from upstream and get this warp context, then it will create glcontext which is shared by the wrap context. qmlglsrc will do texture copy from scene graph render target in afterrendering() slots of thread-1 and pass that texture downstream. glimagesink will render this texture in thread-2.

But the result is that glimagesink only show black frame not the qmlview.

In generally, two threads, each thread has a context and a render surface(qmlview, glimagesink window).

Is there something wrong with my design?

Created attachment 331754 [details] [review]
code for qmlglsrc

Created attachment 331755 [details] [review]
unit test code just for reference

(In reply to Matthew Waters (ystreet00) from comment #1)
> qmlglsrc as a screen grabber of the qml scene graph is in general a bad idea
> for performance reasons as you have to perform a readback (bad) of the GL
> front buffer (also bad).  If possible, you should attempt to retrieve your
> rendering before qmlglsink and pass that downstream.
> 
> That being said, it's possible that screen grabbing may be your only option
> so something like that would be useful.

Any comments for my code?

(In reply to Matthew Waters (ystreet00) from comment #1)
> qmlglsrc as a screen grabber of the qml scene graph is in general a bad idea
> for performance reasons as you have to perform a readback (bad) of the GL
> front buffer (also bad).  If possible, you should attempt to retrieve your
> rendering before qmlglsink and pass that downstream.
> 
> That being said, it's possible that screen grabbing may be your only option
> so something like that would be useful.

Hi Matthew，

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 will ensure qmlview running smoothly.

Created attachment 332082 [details] [review]
fix some bug, need review

This is the last code which I fixed some bugs and coding error.

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.

::: 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?

@@ +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/...?

@@ +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?

@@ +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.

::: 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.

::: 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.

@@ +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?

@@ +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.

@@ +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

@@ +279,3 @@
+
+void
+QtGLWindow::onSceneGraphInitialized()

This looks like duplicated code with the sink.  Maybe make a utility function to make this easier?

::: 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?

(In reply to Matthew Waters (ystreet00) from comment #8)
> Review of attachment 332082 [details] [review] [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?

(In reply to Haihua Hu from comment #2)
> (In reply to Matthew Waters (ystreet00) from comment #1)
> > qmlglsrc as a screen grabber of the qml scene graph is in general a bad idea
> > for performance reasons as you have to perform a readback (bad) of the GL
> > front buffer (also bad).  If possible, you should attempt to retrieve your
> > rendering before qmlglsink and pass that downstream.
> > 
> > That being said, it's possible that screen grabbing may be your only option
> > so something like that would be useful.
> 
> I'm now implementing this plugin which is only for grabing scene graph. But
> I met a problem in unit test. pipeline for unit test is :
> 
> qmlglsrc ! glimagesink
> 
> There are two important threads in unit test that thread-1 is scene graph
> render thread and thread-2 is glimagesink rendering thread. thread-1 warp qt
> scene graph context into glcontext, glimagesink will query app_context from
> upstream and get this warp context, then it will create glcontext which is
> shared by the wrap context. qmlglsrc will do texture copy from scene graph
> render target in afterrendering() slots of thread-1 and pass that texture
> downstream. glimagesink will render this texture in thread-2.
> 
> But the result is that glimagesink only show black frame not the qmlview.
> 
> In generally, two threads, each thread has a context and a render
> surface(qmlview, glimagesink window).
> 
> Is there something wrong with my design?

Hi Matthew,

Do you know the root cause of this issue if connect to glimagesink?

Are the GL contexts actually shared correctly?

Check with GST_DEBUG=glcontext:5 and look for other_context.

(In reply to Matthew Waters (ystreet00) from comment #11)
> Are the GL contexts actually shared correctly?
> 
> Check with GST_DEBUG=glcontext:5 and look for other_context.

I think qtwarpglconext and glimagesink glcontext actually shared correctly, see below log. Otherwise, scene graph cannot copy texture into another texture create by glimagesink glcontext.

glcontext gstglcontext.c:938:gst_gl_context_create:<glcontextglx0>  other_context:<glwrappedcontext0>
glcontext gstglcontext.c:843:gst_gl_context_set_window:<glcontextglx0> window:<glwindowx11-0>
glcontext gstglcontext.c:1096:gst_gl_context_create_thread:<glcontextglx0> Creating thread
glcontext gstglcontext_glx.c:333:gst_gl_context_glx_choose_format: GLX Version: 1.4
glcontext gstglcontext_glx.c:124:_describe_fbconfig: ID: 124
glcontext gstglcontext_glx.c:126:_describe_fbconfig: double buffering: 1
glcontext gstglcontext_glx.c:128:_describe_fbconfig: red: 8
glcontext gstglcontext_glx.c:130:_describe_fbconfig: green: 8
glcontext gstglcontext_glx.c:132:_describe_fbconfig: blue: 8
glcontext gstglcontext_glx.c:134:_describe_fbconfig: alpha: 0
glcontext gstglcontext_glx.c:136:_describe_fbconfig: depth: 24
glcontext gstglcontext_glx.c:138:_describe_fbconfig: stencil: 8
glcontext gstglcontext.c:1153:gst_gl_context_create_thread:<glcontextglx0> Attempting to create opengl context. user chosen api(s) (any), compiled api support (opengl opengl3 gles2) display api (opengl)
glcontext gstglcontext_glx.c:270:gst_gl_context_glx_create_context: gl context id: 140582936698176

Then you'll have to start looking at GL state in e.g. apitrace and see what's failing.

(In reply to Matthew Waters (ystreet00) from comment #13)
> Then you'll have to start looking at GL state in e.g. apitrace and see
> what's failing.

The root cause has been located. It seems that the glwindow_x11 don't get ConfigureNotify from X11 server, as a result, the glimagesink set glviewport to 0x0 size. Then why?

That sounds abnormal, if that's the problem, then fix your X server.

(In reply to Matthew Waters (ystreet00) from comment #15)
> That sounds abnormal, if that's the problem, then fix your X server.

Will this cause by qt event loop override the x11 event loop?

Created attachment 332185 [details] [review]
qmlglsrc modified by comments

Created attachment 332186 [details] [review]
qmlglsrc code modified by comments

Created attachment 332187 [details] [review]
unit test code

Created attachment 332188 [details] [review]
To fix glimagesink don't show frame when connect to qmlglsrc

Review of attachment 332188 [details] [review]:

Just some style fixes.

::: gst-libs/gst/gl/x11/gstglwindow_x11.c
@@ +390,3 @@
   GstGLWindowX11 *window_x11 = data;
   GstGLWindow *window = GST_GL_WINDOW (window_x11);
+  guint width, height;

Move inside the gst_gl_window_is_running block.

@@ +418,3 @@
+    gst_gl_window_get_surface_dimensions (window, &width, &height);
+    if (attr.width != width || attr.height != height) {
+      gst_gl_window_resize (window, attr.width, attr.height);

Use gst_gl_window_queue_resize() here.

@@ +426,1 @@
         gst_gl_window_get_surface_dimensions (window, &width, &height);

duplicated get_surface_dimensions()

Created attachment 332191 [details] [review]
fix some coding styles for glimagesink issue

Created attachment 332192 [details] [review]
fix some coding styles for glimagesink issue

Review of attachment 332187 [details] [review]:

This looks mostly ok.

::: tests/examples/qt/qmlsrc/main.cpp
@@ +46,3 @@
+  gst_init (&argc, &argv);
+
+  gchar *sink_str = "glimagesink sync=false rotate-method=4";

These parameters should not be needed.  If they are, there's a bug somewhere else.

Created attachment 332193 [details] [review]
refine unit test code

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?

@@ +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.

@@ +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

@@ +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.

@@ +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.

@@ +335,3 @@
+  }
+#endif
+#if GST_GL_HAVE_WINDOW_FB

Still here.

Review of attachment 332193 [details] [review]:

This didn't change anything...

::: tests/examples/qt/qmlsrc/main.cpp
@@ +49,3 @@
+  GstElement *src = gst_element_factory_make ("qmlglsrc", NULL);
+  GstElement *sink = 
+    gst_parse_bin_from_description ("glimagesink sync=false rotate-method=4", TRUE, NULL);

These properties to glimagesink are not necessary, if they are, there's a bug somewhere else.

(In reply to Matthew Waters (ystreet00) from comment #27)
> Review of attachment 332193 [details] [review] [review]:
> 
> This didn't change anything...
> 
> ::: tests/examples/qt/qmlsrc/main.cpp
> @@ +49,3 @@
> +  GstElement *src = gst_element_factory_make ("qmlglsrc", NULL);
> +  GstElement *sink = 
> +    gst_parse_bin_from_description ("glimagesink sync=false
> rotate-method=4", TRUE, NULL);
> 
> These properties to glimagesink are not necessary, if they are, there's a
> bug somewhere else.

Because the texture copied from scene graph is vertical flipped.

(In reply to Matthew Waters (ystreet00) from comment #26)
> Review of attachment 332186 [details] [review] [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.

(In reply to Haihua Hu from comment #28)
> (In reply to Matthew Waters (ystreet00) from comment #27)
> > These properties to glimagesink are not necessary, if they are, there's a
> > bug somewhere else.
> 
> Because the texture copied from scene graph is vertical flipped.

Then you need to account for that in the src by flipping or negotiate GstVideoAffineTransformationMeta, not paper over it in the sink by setting a property on the sink.

sync=false is also wrong.

(In reply to Matthew Waters (ystreet00) from comment #30)
> (In reply to Haihua Hu from comment #28)
> > (In reply to Matthew Waters (ystreet00) from comment #27)
> > > These properties to glimagesink are not necessary, if they are, there's a
> > > bug somewhere else.
> > 
> > Because the texture copied from scene graph is vertical flipped.
> 
> Then you need to account for that in the src by flipping or negotiate
> GstVideoAffineTransformationMeta, not paper over it in the sink by setting a
> property on the sink.
> 
> sync=false is also wrong.

Got it.

Created attachment 332253 [details] [review]
qmlglsrc patch: Modified  by comments

Created attachment 332254 [details] [review]
unit test code remove property setting for glimagesink

Created attachment 332262 [details] [review]
qml patch: modify by comments

Run QtGLWindow::onSceneGraphInitialized() accroding to whether it is initialized.

Created attachment 332271 [details] [review]
qmlglsrc patch: modified by comments

Run QtGLWindow::onSceneGraphInitialized() accroding to whether it is initialized.

Hi Matthew,

Sorry for re-upload this patch, attachment 332271 [details] [review] and 332254 are the latest code, Any more comments for my code?

(In reply to Haihua Hu from comment #36)
> Hi Matthew,
> 
> Sorry for re-upload this patch, attachment 332271 [details] [review] [review] and
> 332254 are the latest code, Any more comments for my code?

comment 30 (affine transformation meta) has not been addressed yet.

i.e. not all elements support the transformation meta. gltransformation and glimagesink are the only elements that do so you need something else to flip when the transformation meta is not supported.

(In reply to Matthew Waters (ystreet00) from comment #38)
> i.e. not all elements support the transformation meta. gltransformation and
> glimagesink are the only elements that do so you need something else to flip
> when the transformation meta is not supported.

How can qmlglsrc get to know if downstream support the transformation meta? The only method that comes to me is to use GST_TAG_IMAGE_ORIENTATION to inform downstream the orientation which is support by videoflip, glvideoflip and glimagesink, but need set property to let them get tag event. If use GST_TAG_IMAGE_ORIENTATION, we should remove transformation meta from output buffer in case that glimagesink will do flip twice.

Created attachment 332456 [details] [review]
Add image orientation tag, remove transform meta

Created attachment 332457 [details] [review]
unit test: get image orientation tag from upstream

Created attachment 332458 [details] [review]
Add image orientation tag, remove transform meta

Hi Matthew, is there anything update?

(In reply to Haihua Hu from comment #39)
> How can qmlglsrc get to know if downstream support the transformation meta?
> The only method that comes to me is to use GST_TAG_IMAGE_ORIENTATION to
> inform downstream the orientation which is support by videoflip, glvideoflip
> and glimagesink, but need set property to let them get tag event. If use
> GST_TAG_IMAGE_ORIENTATION, we should remove transformation meta from output
> buffer in case that glimagesink will do flip twice.

The transformation meta negotiation is exposed through the allocation query that you will receive from upstream (glimagesink) proposed by https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/gl/gstglimagesink.c#n1933 and an example of checking for it's existence: https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/gl/gstgltransformation.c#n644

The affine transformation meta should be preferred over the image-orientation tag.

Review of attachment 332458 [details] [review]:

Variable rename please.

::: ext/qt/gstqtglutility.cc
@@ +112,3 @@
+gboolean
+gst_qt_get_gl_warpcontext (GstGLDisplay * display,
+    GstGLContext **warp_glcontext, GstGLContext **context)

warp_glcontext?

Don't you mean wrap_glcontext?

Review of attachment 332457 [details] [review]:

This looks ok.

(In reply to Matthew Waters (ystreet00) from comment #46)
> Review of attachment 332457 [details] [review] [review]:
> 
> This looks ok.

Thanks a lot.

I will add transformation meta negotiation in allocation query and prefer to use affine transformation meta. So I also need to remove the property setting for glimagesink in unit test code.

(In reply to Matthew Waters (ystreet00) from comment #45)
> Review of attachment 332458 [details] [review] [review]:
> 
> Variable rename please.
> 
> ::: ext/qt/gstqtglutility.cc
> @@ +112,3 @@
> +gboolean
> +gst_qt_get_gl_warpcontext (GstGLDisplay * display,
> +    GstGLContext **warp_glcontext, GstGLContext **context)
> 
> warp_glcontext?
> 
> Don't you mean wrap_glcontext?

wrap_glcontext, I will correct it.

Created attachment 332518 [details] [review]
Add transform meta query and prefer to use transform meta

I realized that we don't need to remove property setting for glimagesink. let it chose the best way to do flip. So only this patch need update.

commit 65a3c3567066a37f61c23d05dbf2c3f68c5d8c98
Author: Haihua Hu <jared.hu@nxp.com>
Date:   Wed Jul 27 09:28:23 2016 +0800

    qmlglsrc: Add qmlglsrc unit test example
    
    https://bugzilla.gnome.org/show_bug.cgi?id=768160

commit 0b23bb5a595c23ba7c1e02219ea6f0d180477fb3
Author: Haihua Hu <jared.hu@nxp.com>
Date:   Wed Jul 27 08:16:47 2016 +0800

    qt: implement qmlglsrc for qml view grab
    
    [Matthew Waters]: gst-indent sources
    
    https://bugzilla.gnome.org/show_bug.cgi?id=768160

commit fa37e3588e42e34165b951b9f91af8c1fd2ee205
Author: Haihua Hu <jared.hu@nxp.com>
Date:   Wed Jul 27 10:55:01 2016 +0800

    glwindow: Fix glimagesink cannot show frame when connect to qmlglsrc
    
    When connect to qmlglsrc, x11 event loop will be replace by qt event loop
    which will cause the window cannot receive event from xserver, such as resize
    
    https://bugzilla.gnome.org/show_bug.cgi?id=768160