[Bug 796470] New: vaapidisplay: remove calling gst_vaapi_display_new in each descendant.

["GStreamer" (GNOME Bugzilla)]

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

Bug ID: 796470
Summary: vaapidisplay: remove calling gst_vaapi_display_new in
each descendant.
Classification: Platform
Product: GStreamer
Version: git master
OS: Linux
Status: NEW
Severity: normal
Priority: Normal
Component: gstreamer-vaapi
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@igalia.com
QA Contact: gstreamer-***@lists.freedesktop.org
CC: ***@gmail.com, ***@igalia.com
GNOME version: ---

For more information, see the following comment,
https://bugzilla.gnome.org/show_bug.cgi?id=768266#c35

"Let's keep we are breaking the gobject code conventions. This method is not
intended to be used by the API user, but the internal display decorators.

I would remove this method and use gst_vaapi_display_create() directly by the
decorator. But let's keep it for another iteration."


In addition, we can avoid multiple initialization of VADisplay.
See bug #795391

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #1 from Hyunjun Ko <***@igalia.com> ---
Created attachment 372500
--> https://bugzilla.gnome.org/attachment.cgi?id=372500&action=edit
libs: display: remove unused code

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #2 from Hyunjun Ko <***@igalia.com> ---
Created attachment 372501
--> https://bugzilla.gnome.org/attachment.cgi?id=372501&action=edit
libs: display: remove unnecessary legacy code since gobjectification

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #3 from Hyunjun Ko <***@igalia.com> ---
Created attachment 372502
--> https://bugzilla.gnome.org/attachment.cgi?id=372502&action=edit
libs: display: replace calling gst_vaapi_display_new with
gst_vaapi_display_create method

Gobjectification for GstVaapiDisplay was almost done by the commit 185da3d1.
But still something breaking GObject code convention remains, which is
calling gst_vaapi_display_new in each decendants.

This patch replaces it with gst_vaapi_display_create, defined in private
header.

In addition, this patch avoids duplicate calls of VAInitialize in case of
GstVaapiDisplayEGL.

["GStreamer" (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #372500|none |accepted-commit_now
status| |

--- Comment #5 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 372500:
--> (https://bugzilla.gnome.org/review?bug=796470&attachment=372500)

lgtm

["GStreamer" (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #372501|none |accepted-commit_now
status| |

--- Comment #6 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 372501:
--> (https://bugzilla.gnome.org/review?bug=796470&attachment=372501)

lgtm

["GStreamer" (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #372502|none |reviewed
status| |

--- Comment #9 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 372502:
--> (https://bugzilla.gnome.org/review?bug=796470&attachment=372502)

finally, perhaps it would be nice remove gst_vaapi_display_new() to avoid
confusion.

::: gst-libs/gst/vaapi/gstvaapidisplay.c
@@ +1378,3 @@

+void
+gst_vaapi_display_set_display (GstVaapiDisplay * display, VADisplay
va_display)

As far as I understand, this is not enough, since it is required to update all
the private data using the vmethod get_display_info()

Something like this:

GstVaapiDisplayInfo info = { NULL, };
GstVaapiDisplayClass *const klass = GST_VAAPI_DISPLAY_GET_CLASS (display);

if (klass->get_display && !klass->get_display (display, &info))
return FALSE;

priv->display = info.va_display;
priv->native_display = info.native_display;
g_free (priv->display_name);
priv->display_name = g_strdup (info.display_name);

Also, this function can be reused by gst_vaapi_display_setup() (aka
gst_vaapi_display_create())

::: gst-libs/gst/vaapi/gstvaapidisplay_priv.h
@@ +205,3 @@

+gboolean
+gst_vaapi_display_create (GstVaapiDisplay * display,

I would rename this function to gst_vaapi_display_setup()

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #8 from Hyunjun Ko <***@igalia.com> ---
(In reply to Víctor Manuel Jáquez Leal from comment #4)

overall looks good, but, as we need also to backport it to 1.14, I have the
feeling that thist patch set it is too invasive for stable.

I agree totally. Looks risky to backport.

["GStreamer" (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #372500|accepted-commit_now |committed
status| |

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #4 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
overall looks good, but, as we need also to backport it to 1.14, I have the
feeling that thist patch set it is too invasive for stable.

["GStreamer" (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Blocks| |795391

["GStreamer" (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #372501|accepted-commit_now |committed
status| |

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #7 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Attachment 372500 pushed as 3056f06 - libs: display: remove unused code
Attachment 372501 pushed as aa77862 - libs: display: remove unnecessary legacy
code since gobjectification

["GStreamer" (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Blocks|795391 |
Depends on| |795391

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #10 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
I have posted a new set of patches for bug 795391. Your patch is still required
to clean up the code, but not as a way to fix the double initialization of the
display.

I'll rebase you patch after the commit of bug 795391.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=796470
Bug 796470 depends on bug 795391, which changed state.

Bug 795391 Summary: vaapi: problems when playing with glimagesink with egl
https://bugzilla.gnome.org/show_bug.cgi?id=795391

What |Removed |Added
----------------------------------------------------------------------------
Status|REOPENED |RESOLVED
Resolution|--- |FIXED

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #11 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 372670
--> https://bugzilla.gnome.org/attachment.cgi?id=372670&action=edit
libs: display: redefine gst_vaapi_display_create()

The function name was gst_vaapi_display_create_unlocked(), nonetheless
it wasn't called unlocked. In order to keep the semantics this patch
renames the gst_vaapi_display_create_unlocked() as
gst_vaapi_display_create(), removing the previous function
gst_vaapi_display_create().

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #13 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 372672
--> https://bugzilla.gnome.org/attachment.cgi?id=372672&action=edit
vaapibufferpool: declare parameter display as object

We have neglected to update this code since GstVaapiDisplay turned
into a GstObject descendant.

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #12 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 372671
--> https://bugzilla.gnome.org/attachment.cgi?id=372671&action=edit
libs: display: replace gst_vaapi_display_new() with gst_vaapi_display_config()

Gobjectification for GstVaapiDisplay was almost done by the commit 185da3d1.
But still something breaking GObject code convention remains, which is
calling gst_vaapi_display_new() in each decendants.

This patch replaces it with gst_vaapi_display_config(), defined in private
header.

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #14 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 372673
--> https://bugzilla.gnome.org/attachment.cgi?id=372673&action=edit
libs: display: remove gst_vaapi_display_unref()

Use gst_object_unref() instead.

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #15 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 372674
--> https://bugzilla.gnome.org/attachment.cgi?id=372674&action=edit
libs: display: remove gst_vaapi_display_ref()

Replace it with gst_object_ref()

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #16 from Hyunjun Ko <***@igalia.com> ---
(In reply to Víctor Manuel Jáquez Leal from comment #10)

Post by "GStreamer" (GNOME Bugzilla)
I have posted a new set of patches for bug 795391. Your patch is still
required to clean up the code, but not as a way to fix the double
initialization of the display.
I'll rebase you patch after the commit of bug 795391.

Thanks for your work!

["GStreamer" (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #372672|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #372674|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED

--- Comment #17 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Attachment 372670 pushed as 8de7dcf - libs: display: redefine
gst_vaapi_display_create()
Attachment 372671 pushed as a6881b9 - libs: display: replace
gst_vaapi_display_new() with gst_vaapi_display_config()
Attachment 372672 pushed as b1d8c68 - vaapibufferpool: declare parameter
display as object
Attachment 372673 pushed as fb1c4c5 - libs: display: remove
gst_vaapi_display_unref()
Attachment 372674 pushed as 6ccd5d6 - libs: display: remove
gst_vaapi_display_ref()

["GStreamer" (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #372670|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #372502|0 |1
is obsolete| |

["GStreamer" (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #372671|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #372673|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|git master |1.15.1