Created attachment 332333 [details] [review]
glupload: Use bufferpool to allocate new buffer in UploadMeta

First, I propose this patch for glupload

IMO you should assign this bug to gst-plugins-bad to GstGL libraries and elements. @Matthew should review this patch.

Review of attachment 332333 [details] [review]:

Looks good.  With the warning, it's good to go.

::: gst-libs/gst/gl/gstglupload.c
@@ +882,3 @@
+  if (gst_buffer_pool_acquire_buffer (upload->pool, outbuf,
+          NULL) != GST_FLOW_OK)
+    return GST_GL_UPLOAD_ERROR;

Add a warning here about acquire failing.

Created attachment 332509 [details] [review]
glupload: Use bufferpool to allocate new buffer in GLTextureUploadMeta

Added warning message.

commit 2bb3baa8c9677685215f8ff3d7204f43d750a472
Author: Hyunjun Ko <zzoon@igalia.com>
Date:   Tue Aug 2 17:21:20 2016 +0900

    glupload: Use bufferpool to allocate new buffer in GLTextureUploadMeta
    
    To improve performace of upload with GLTextureUploadMeta,
    use bufferpool instead of allocating new buffer every time.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=769293

Thanks for merge, Matthew!
I reopen this issue, because we can go to the second step,
which is to improve in vaapi side.

Created attachment 332784 [details] [review]
Proof of Concept: Uploading to GL texture optimization

This is proof-of-concept patch to share opinions.

Changes are following:
- Cut the relationship between GstVaapiVideoMetaTexture and VaapiTexture.
- Then VaapiDisplay has hash table of VaapiTexture instances.
- Each upload call, lookup same name of texture in the hash table and then use it.

This is working fine with previous glupload patch, with good performance.

Question is:
1. Is it correct taht placing VaapiTexture apart from GstVaapiVideoMetaTexture?
2. Is it correct that VaapiDisplay has texture instances?
3. If reconfigure happens, texture instances should be removed? If so, where?
4. If we should limit the hash table's size, what's maximum size?

Hyunjun,

And important thing to notice is to tests your patches not only with glimagesink, but with clutter{auto}videosink (clutter-gst) which is the video sink used by totem and totem is an important user of gstreamer-vaapi.

(In reply to Hyunjun Ko from comment #7)
> Created attachment 332784 [details] [review] [review]
> Proof of Concept: Uploading to GL texture optimization
> 
> This is proof-of-concept patch to share opinions.
> 
> Changes are following:
> - Cut the relationship between GstVaapiVideoMetaTexture and VaapiTexture.
> - Then VaapiDisplay has hash table of VaapiTexture instances.
> - Each upload call, lookup same name of texture in the hash table and then
> use it.
> 
> This is working fine with previous glupload patch, with good performance.
> 
> Question is:
> 1. Is it correct taht placing VaapiTexture apart from
> GstVaapiVideoMetaTexture?

Looks correct to have a texture provider independent of the buffers.

> 2. Is it correct that VaapiDisplay has texture instances?

I don't know. I don't thinks so. Perhaps in GstVaapiDisplay{GLX/EGL}

> 3. If reconfigure happens, texture instances should be removed? If so, where?

Yes. Perhaps if the buffer to push has a different size of the current texture provider configuration, we should reset all the cached textures.

> 4. If we should limit the hash table's size, what's maximum size?

I don't know.

(In reply to Víctor Manuel Jáquez Leal from comment #8)
> Hyunjun,
> 
> And important thing to notice is to tests your patches not only with
> glimagesink, but with clutter{auto}videosink (clutter-gst) which is the
> video sink used by totem and totem is an important user of gstreamer-vaapi.

Verified with clutterautovideosink.
Actually, it's working fine even without this patch, since cluttersink is using only one texture. 


> I don't know. I don't thinks so. Perhaps in GstVaapiDisplay{GLX/EGL}
Agree, but we can create base method in GstVaapiDisplay like create_texture.

> > 3. If reconfigure happens, texture instances should be removed? If so, where?
> 
> Yes. Perhaps if the buffer to push has a different size of the current
> texture provider configuration, we should reset all the cached textures.
Agree. I'm going to find out where it does properly.

Created attachment 332982 [details] [review]
vaapidisplay/texture: cache VaapiTexture instances and reuse when uploading to GL texture

This patch is to improve performance of uploading to GL texture.
Caches created VaapiTexture instance to VaapiDisplay,
so it finds matched instance by texture id in each upload method.

Created attachment 332983 [details] [review]
plugins: reset cached vaapitexture during (re)configure

I would merge this after 1.10, because is a big change for an optimization.

I'm revisiting this issue. And I have to rethink this question

(In reply to Víctor Manuel Jáquez Leal from comment #8)
> > Question is:
> > 1. Is it correct taht placing VaapiTexture apart from
> > GstVaapiVideoMetaTexture?
> 
> Looks correct to have a texture provider independent of the buffers.

*If* the GLTexture buffer meta is handled correctly (no memleaks) and the vaapi buffer pool handles correctly a set of buffers, and reuses them, the texture cache is the buffer meta itself! there's no need of an extra cache structure.

Let's see what's failing when glimagesink is used.

aha!

The problem is a design one in gstreamer-vaapi, since it assumes that every buffer/surface will have associated, one and only one, GL texture, though one GL texture can be associated to several surfaces.

But, in the case of glimagesink, it can provides several GL textures, so the GstVaapiTexture (a wrapper for the gl texture) is created every time, since the texture id doesn't correspond to the one in the meta.

Hyunjun's hash map approach is nice, since it decouples the GstVaapiTexture association with the meta, but still need extra code clean up, if we wan to ride of the GstVaapiTexture structure from the GLTextureUpload meta.

Still, lookup in a hash map is a bit more expensive than just derreferencing.

Also, we need to keep the check of "the same display".

(In reply to Víctor Manuel Jáquez Leal from comment #14)
 
> Still, lookup in a hash map is a bit more expensive than just derreferencing.
Indeed.
This should be better regarding to using clutterautovideosink.

> Also, we need to keep the check of "the same display".
I thought that it's not necessary, but I was wrong, yes.

Created attachment 335888 [details] [review]
[PATCH 1/2] vaapidisplay/texture: cache VaapiTexture instances and reuse when uploading to GL texture

This patch is to improve performance of uploading to GL texture.
Caches created VaapiTexture instance to VaapiDisplay,
so it finds matched instance by texture id in each upload method.

Created attachment 335889 [details] [review]
[PATCH 2/2] plugins: reset cached vaapitexture during (re)configure

When reconfigure performs, a buffer to be uploaded might have different size.

Review of attachment 335888 [details] [review]:

Good!

Now I'm a bit worried about the duplicated code, either in gstvaapidisplay_egl.c and gstvaapidisplay_glx.c

I'm wondering, what if a new "object" is created, an gstvaapiobject that wraps GHashTable and the duplicated functions, and its methods will be gst_vaapi_texture_map_{add, lookup, reset}, and its normal constructor and destructor.

Now, GstVaapiDisplayGLX and GstVaapiDisplayEGL, will have and instance of this new object, and a getter for this object, that will be a virtual function in GstVaapiDisplay: gst_vaapi_display_get_texture_map(). And that should be the only new virtual function.

Then, when the user ask for a new texture, the create_texture() vmethod would look for the requested texture in the texture map, if the current GstVaapiDisplay provides one, and if it is not, or the texture is not found either, a new GstVaapiTexture is created, otherwise returns the found one.

What do you think?

(In reply to Víctor Manuel Jáquez Leal from comment #18)
> Review of attachment 335888 [details] [review] [review]:
> 
> Good!
> 
> Now I'm a bit worried about the duplicated code, either in
> gstvaapidisplay_egl.c and gstvaapidisplay_glx.c
> 
> I'm wondering, what if a new "object" is created, an gstvaapiobject that
> wraps GHashTable and the duplicated functions, and its methods will be
> gst_vaapi_texture_map_{add, lookup, reset}, and its normal constructor and
> destructor.
> 
> Now, GstVaapiDisplayGLX and GstVaapiDisplayEGL, will have and instance of
> this new object, and a getter for this object, that will be a virtual
> function in GstVaapiDisplay: gst_vaapi_display_get_texture_map(). And that
> should be the only new virtual function.

Good idea! I don't like this kind of duplicated code either.


> Then, when the user ask for a new texture, the create_texture() vmethod
> would look for the requested texture in the texture map, if the current
> GstVaapiDisplay provides one, and if it is not, or the texture is not found
> either, a new GstVaapiTexture is created, otherwise returns the found one.
> 
> What do you think?

This approach might keep gst_vaapi_texture_upload as-is.
I like this idea. Only thing that i wonder is the name "create_texture" doesn't only mean create any more, but get, which is a little bit ambiguous to read.
But since it's internal function, I'm ok too :)

Created attachment 335975 [details] [review]
[PATCH 1/3] vaapitexturemap: created new vaapi object named GstVaapiTextureMap

Create GstVaapiTextureMap to cache vaapi textures to be able to reuse,
which has hash table of vaapi textures to manage.

Comment on attachment 335975 [details] [review]
[PATCH 1/3] vaapitexturemap: created new vaapi object named GstVaapiTextureMap

>From c6a1985ecdb786d9cb5eb794543fbe26bb7520d0 Mon Sep 17 00:00:00 2001
>From: Hyunjun Ko <zzoon@igalia.com>
>Date: Wed, 21 Sep 2016 15:41:56 +0900
>Subject: [PATCH 1/3] vaapitexturemap: created new vaapi object named
> GstVaapiTextureMap
>
>Create GstVaapiTextureMap to cache vaapi textures to be able to reuse,
>which has hash table of vaapi textures to manage.
>
>https://bugzilla.gnome.org/show_bug.cgi?id=769293
>---
> gst-libs/gst/vaapi/Makefile.am               |   3 +
> gst-libs/gst/vaapi/gstvaapitexturemap.c      | 105 +++++++++++++++++++++++++++
> gst-libs/gst/vaapi/gstvaapitexturemap.h      |  48 ++++++++++++
> gst-libs/gst/vaapi/gstvaapitexturemap_priv.h |  53 ++++++++++++++
> 4 files changed, 209 insertions(+)
> create mode 100644 gst-libs/gst/vaapi/gstvaapitexturemap.c
> create mode 100644 gst-libs/gst/vaapi/gstvaapitexturemap.h
> create mode 100644 gst-libs/gst/vaapi/gstvaapitexturemap_priv.h
>
>diff --git a/gst-libs/gst/vaapi/Makefile.am b/gst-libs/gst/vaapi/Makefile.am
>index fc97c94..54cbc75 100644
>--- a/gst-libs/gst/vaapi/Makefile.am
>+++ b/gst-libs/gst/vaapi/Makefile.am
>@@ -73,6 +73,7 @@ libgstvaapi_source_c =				\
> 	gstvaapisurfacepool.c			\
> 	gstvaapisurfaceproxy.c			\
> 	gstvaapitexture.c			\
>+	gstvaapitexturemap.c			\
> 	gstvaapiutils.c				\
> 	gstvaapiutils_core.c			\
> 	gstvaapiutils_h264.c			\
>@@ -105,6 +106,7 @@ libgstvaapi_source_h =				\
> 	gstvaapisurfacepool.h			\
> 	gstvaapisurfaceproxy.h			\
> 	gstvaapitexture.h			\
>+	gstvaapitexturemap.h			\
> 	gstvaapitypes.h				\
> 	gstvaapiutils_h264.h			\
> 	gstvaapiutils_h265.h			\
>@@ -136,6 +138,7 @@ libgstvaapi_source_priv_h =			\
> 	gstvaapisurface_priv.h			\
> 	gstvaapisurfaceproxy_priv.h		\
> 	gstvaapitexture_priv.h			\
>+	gstvaapitexturemap_priv.h		\
> 	gstvaapiutils.h				\
> 	gstvaapiutils_core.h			\
> 	gstvaapiutils_h264_priv.h		\
>diff --git a/gst-libs/gst/vaapi/gstvaapitexturemap.c b/gst-libs/gst/vaapi/gstvaapitexturemap.c
>new file mode 100644
>index 0000000..b1b3c2d
>--- /dev/null
>+++ b/gst-libs/gst/vaapi/gstvaapitexturemap.c
>@@ -0,0 +1,105 @@
>+/*
>+ *  gstvaapitexture.c - VA texture Hash map
>+ *
>+ *  Copyright (C) 2016 Intel Corporation
>+ *  Copyright (C) 2016 Igalia S.L.
>+ *
>+ *  This library is free software; you can redistribute it and/or
>+ *  modify it under the terms of the GNU Lesser General Public License
>+ *  as published by the Free Software Foundation; either version 2.1
>+ *  of the License, or (at your option) any later version.
>+ *
>+ *  This library is distributed in the hope that it will be useful,
>+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>+ *  Lesser General Public License for more details.
>+ *
>+ *  You should have received a copy of the GNU Lesser General Public
>+ *  License along with this library; if not, write to the Free
>+ *  Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
>+ *  Boston, MA 02110-1301 USA
>+ */
>+
>+/**
>+ * SECTION:gstvaapitexturemap
>+ * @short_description: VA/GLX/EGL texture hash map abstraction
>+ */
>+
>+#include "gstvaapitexturemap.h"
>+#include "gstvaapitexturemap_priv.h"
>+
>+#define DEBUG 1
>+#include "gstvaapidebug.h"
>+
>+#define MAX_NUM_TEXTURE 10
>+
>+static void gst_vaapi_texture_map_finalize (GstVaapiTextureMap * map);
>+
>+GST_VAAPI_OBJECT_DEFINE_CLASS (GstVaapiTextureMap, gst_vaapi_texture_map);
>+
>+/**
>+ * gst_vaapi_texture_map_new:
>+ * @display: a #GstVaapiDisplay
>+ *
>+ * Creates a texture hash map.
>+ *
>+ * Return value: the newly created #GstVaapiTextureMap object
>+ */
>+GstVaapiTextureMap *
>+gst_vaapi_texture_map_new (GstVaapiDisplay * display)
>+{
>+  GstVaapiTextureMap *map;
>+
>+  map = gst_vaapi_object_new (gst_vaapi_texture_map_class (), display);
>+  if (!map)
>+    return NULL;
>+
>+  map->texture_map =
>+      g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL,
>+      (GDestroyNotify) gst_vaapi_texture_unref);
>+
>+  return map;
>+}
>+
>+static void
>+gst_vaapi_texture_map_finalize (GstVaapiTextureMap * map)
>+{
>+  if (map->texture_map) {
>+    g_hash_table_remove_all (map->texture_map);
>+    g_hash_table_destroy (map->texture_map);
>+  }
>+}
>+
>+gboolean
>+gst_vaapi_texture_map_add (GstVaapiTextureMap * map, GstVaapiTexture * texture,
>+    guint id)
>+{
>+  g_return_val_if_fail (map != NULL, FALSE);
>+  g_return_val_if_fail (map->texture_map != NULL, FALSE);
>+  g_return_val_if_fail (texture != NULL, FALSE);
>+
>+  if (g_hash_table_size (map->texture_map) > MAX_NUM_TEXTURE)
>+    gst_vaapi_texture_map_reset (map);
>+
>+  g_hash_table_insert (map->texture_map, GUINT_TO_POINTER (id), texture);
>+
>+  return TRUE;
>+}
>+
>+gpointer
>+gst_vaapi_texture_map_lookup (GstVaapiTextureMap * map, guint id)
>+{
>+  g_return_val_if_fail (map != NULL, NULL);
>+  g_return_val_if_fail (map->texture_map != NULL, NULL);
>+
>+  return g_hash_table_lookup (map->texture_map, GUINT_TO_POINTER (id));
>+}
>+
>+void
>+gst_vaapi_texture_map_reset (GstVaapiTextureMap * map)
>+{
>+  g_return_if_fail (map != NULL);
>+  g_return_if_fail (map->texture_map != NULL);
>+
>+  g_hash_table_remove_all (map->texture_map);
>+}
>diff --git a/gst-libs/gst/vaapi/gstvaapitexturemap.h b/gst-libs/gst/vaapi/gstvaapitexturemap.h
>new file mode 100644
>index 0000000..1b2982b
>--- /dev/null
>+++ b/gst-libs/gst/vaapi/gstvaapitexturemap.h
>@@ -0,0 +1,48 @@
>+/*
>+ *  gstvaapitexturemap.h - VA texture Hash map
>+ *
>+ *  Copyright (C) 2016 Intel Corporation
>+ *  Copyright (C) 2016 Igalia S.L.
>+ *
>+ *  This library is free software; you can redistribute it and/or
>+ *  modify it under the terms of the GNU Lesser General Public License
>+ *  as published by the Free Software Foundation; either version 2.1
>+ *  of the License, or (at your option) any later version.
>+ *
>+ *  This library is distributed in the hope that it will be useful,
>+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>+ *  Lesser General Public License for more details.
>+ *
>+ *  You should have received a copy of the GNU Lesser General Public
>+ *  License along with this library; if not, write to the Free
>+ *  Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
>+ *  Boston, MA 02110-1301 USA
>+ */
>+
>+#ifndef GST_VAAPI_TEXTURE_MAP_H
>+#define GST_VAAPI_TEXTURE_MAP_H
>+
>+#include <gst/vaapi/gstvaapitypes.h>
>+#include <gst/vaapi/gstvaapidisplay.h>
>+#include <gst/vaapi/gstvaapitexture.h>
>+
>+G_BEGIN_DECLS
>+
>+typedef struct _GstVaapiTextureMap GstVaapiTextureMap;
>+
>+GstVaapiTextureMap *
>+gst_vaapi_texture_map_new (GstVaapiDisplay * display);
>+
>+gboolean
>+gst_vaapi_texture_map_add (GstVaapiTextureMap * map, GstVaapiTexture * texture, guint id);
>+
>+gpointer
>+gst_vaapi_texture_map_lookup (GstVaapiTextureMap * map, guint id);
>+
>+void
>+gst_vaapi_texture_map_reset (GstVaapiTextureMap * map);
>+
>+G_END_DECLS
>+
>+#endif /* GST_VAAPI_TEXTURE_MAP_H */
>diff --git a/gst-libs/gst/vaapi/gstvaapitexturemap_priv.h b/gst-libs/gst/vaapi/gstvaapitexturemap_priv.h
>new file mode 100644
>index 0000000..660a994
>--- /dev/null
>+++ b/gst-libs/gst/vaapi/gstvaapitexturemap_priv.h
>@@ -0,0 +1,53 @@
>+/*
>+ *  gstvaapitexturemap_priv.h - VA texture Hash map (private definitions)
>+ *
>+ *  Copyright (C) 2016 Intel Corporation
>+ *  Copyright (C) 2016 Igalia S.L.
>+ *
>+ *  This library is free software; you can redistribute it and/or
>+ *  modify it under the terms of the GNU Lesser General Public License
>+ *  as published by the Free Software Foundation; either version 2.1
>+ *  of the License, or (at your option) any later version.
>+ *
>+ *  This library is distributed in the hope that it will be useful,
>+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>+ *  Lesser General Public License for more details.
>+ *
>+ *  You should have received a copy of the GNU Lesser General Public
>+ *  License along with this library; if not, write to the Free
>+ *  Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
>+ *  Boston, MA 02110-1301 USA
>+ */
>+
>+#ifndef GST_VAAPI_TEXTURE_MAP_PRIV_H
>+#define GST_VAAPI_TEXTURE_MAP_PRIV_H
>+
>+#include "gstvaapiobject_priv.h"
>+
>+G_BEGIN_DECLS
>+
>+typedef struct _GstVaapiTextureMapClass GstVaapiTextureMapClass;
>+
>+/**
>+ * GstVaapiTextureMap:
>+ *
>+ * Base class for API-dependent texture map.
>+ */
>+struct _GstVaapiTextureMap {
>+  GstVaapiObject parent_instance;
>+  GHashTable *texture_map;
>+};
>+
>+/**
>+ * GstVaapiTextureMapClass:
>+ *
>+ * Base class for API-dependent texture map.
>+ */
>+struct _GstVaapiTextureMapClass {
>+  GstVaapiObjectClass parent_class;
>+};
>+
>+G_END_DECLS
>+
>+#endif /* GST_VAAPI_TEXTURE_MAP_PRIV_H */
>-- 
>2.7.4
>

Created attachment 335976 [details] [review]
[PATCH 2/3] vaapidisplay/texture: cache VaapiTexture instances when created and reuse

This patch is to improve performance of uploading to GL texture,
Caches created VaapiTexture instance to VaapiDisplay by using GstVaapiTextureMap
so it could find matched instance by texture id in each upload method.

Created attachment 335977 [details] [review]
[PATCH 3/3] plugins: reset texture hash map during (re)configure

When reconfigure performs, a buffer to be uploaded might have different size.

Oops. Sorry for the noise on comment #21
Please ignore that.

Review of attachment 335977 [details] [review]:

::: gst/vaapi/gstvaapipluginbase.c
@@ +713,3 @@
     return FALSE;
+
+  if (gst_vaapi_display_has_opengl (plugin->display)) {

I'd move up this, and reset the texture map only when the output caps change.

Review of attachment 335976 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapidisplay_egl.c
@@ +244,3 @@
+    texture = gst_vaapi_texture_map_lookup (priv->texture_map, id);
+    if (!texture) {
+      if (texture =

Double parenthesis here too.

::: gst-libs/gst/vaapi/gstvaapidisplay_glx.c
@@ +62,3 @@
+    texture = gst_vaapi_texture_map_lookup (priv->texture_map, id);
+    if (!texture) {
+      if (texture =

Double parenthesis here.

Otherwise the compilation may fail.

Review of attachment 335976 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapidisplay_egl.c
@@ +246,3 @@
+      if (texture =
+          gst_vaapi_texture_egl_new_wrapped (display, id, target, format, width,
+              height))

The code style is not correct here, more spaces are required. Use the gst-indent script to check it out.

@@ +298,3 @@
   dpy_class->display_type = GST_VAAPI_DISPLAY_TYPE_EGL;
+  dpy_class->init = gst_vaapi_display_egl_init;
+  dpy_class->destroy = gst_vaapi_display_egl_destroy;

You did not assigned get_texture_map callback

::: gst-libs/gst/vaapi/gstvaapidisplay_priv.h
@@ +217,3 @@
   /*< public >*/
   GstVaapiDisplayInitFunc init;
+  GstVaapiDisplayDestroyFunc destroy;

Instead of adding a new callback, we should try first to chain up the destructor.

Review of attachment 335975 [details] [review]:

Sorry for saying this now, but I wonder if it worth to use GObject directly, rather than GstVaapiObject, since the conversion is no the roadmap

Created attachment 336053 [details] [review]
[PATCH 1/3] vaapitexturemap: created new object GstVaapiTextureMap

Create GstVaapiTextureMap to cache vaapi textures to be able to reuse,
which has hash table of vaapi textures to manage.
Note that this is inherited from GstObject to be able to be traced by gst tracer.

Created attachment 336054 [details] [review]
[PATCH 2/3] vaapidisplay/texture: cache VaapiTexture instances when created and reuse

This patch is to improve performance of uploading to GL texture,
Caches created VaapiTexture instance to VaapiDisplay by using GstVaapiTextureMap
so it could find matched instance by texture id in each upload method.

Created attachment 336055 [details] [review]
[PATCH 3/3] plugins: reset texture hash map during (re)configure and close

When reconfigure performs, a buffer to be uploaded might have different size.
In case of output caps change, we should reset texture map.

In addition, during pipeline shutdown, textures in texture map need to be released,
since each references vaapi display object.

I have pushed the patches with "some" modifications :)

*** Bug 720727 has been marked as a duplicate of this bug. ***