After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 796470 - vaapidisplay: remove calling gst_vaapi_display_new in each descendant.
vaapidisplay: remove calling gst_vaapi_display_new in each descendant.
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer-vaapi
git master
Other Linux
: Normal normal
: 1.15.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 795391
Blocks:
 
 
Reported: 2018-05-31 08:56 UTC by Hyunjun Ko
Modified: 2018-06-14 14:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
libs: display: remove unused code (3.90 KB, patch)
2018-06-01 05:45 UTC, Hyunjun Ko
committed Details | Review
libs: display: remove unnecessary legacy code since gobjectification (3.35 KB, patch)
2018-06-01 05:45 UTC, Hyunjun Ko
committed Details | Review
libs: display: replace calling gst_vaapi_display_new with gst_vaapi_display_create method (11.46 KB, patch)
2018-06-01 05:46 UTC, Hyunjun Ko
reviewed Details | Review
libs: display: redefine gst_vaapi_display_create() (1.62 KB, patch)
2018-06-13 16:25 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
libs: display: replace gst_vaapi_display_new() with gst_vaapi_display_config() (11.15 KB, patch)
2018-06-13 16:25 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
vaapibufferpool: declare parameter display as object (1.56 KB, patch)
2018-06-13 16:25 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
libs: display: remove gst_vaapi_display_unref() (12.63 KB, patch)
2018-06-13 16:25 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
libs: display: remove gst_vaapi_display_ref() (6.36 KB, patch)
2018-06-13 16:25 UTC, Víctor Manuel Jáquez Leal
committed Details | Review

Description Hyunjun Ko 2018-05-31 08:56:19 UTC
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
Comment 1 Hyunjun Ko 2018-06-01 05:45:24 UTC
Created attachment 372500 [details] [review]
libs: display: remove unused code
Comment 2 Hyunjun Ko 2018-06-01 05:45:53 UTC
Created attachment 372501 [details] [review]
libs: display: remove unnecessary legacy code since  gobjectification
Comment 3 Hyunjun Ko 2018-06-01 05:46:26 UTC
Created attachment 372502 [details] [review]
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.
Comment 4 Víctor Manuel Jáquez Leal 2018-06-06 10:31:22 UTC
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.
Comment 5 Víctor Manuel Jáquez Leal 2018-06-06 17:50:41 UTC
Review of attachment 372500 [details] [review]:

lgtm
Comment 6 Víctor Manuel Jáquez Leal 2018-06-06 17:51:25 UTC
Review of attachment 372501 [details] [review]:

lgtm
Comment 7 Víctor Manuel Jáquez Leal 2018-06-06 18:11:52 UTC
Attachment 372500 [details] pushed as 3056f06 - libs: display: remove unused code
Attachment 372501 [details] pushed as aa77862 - libs: display: remove unnecessary legacy code since gobjectification
Comment 8 Hyunjun Ko 2018-06-07 01:57:23 UTC
(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.
Comment 9 Víctor Manuel Jáquez Leal 2018-06-07 09:28:11 UTC
Review of attachment 372502 [details] [review]:

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()
Comment 10 Víctor Manuel Jáquez Leal 2018-06-11 18:03:46 UTC
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.
Comment 11 Víctor Manuel Jáquez Leal 2018-06-13 16:25:04 UTC
Created attachment 372670 [details] [review]
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().
Comment 12 Víctor Manuel Jáquez Leal 2018-06-13 16:25:11 UTC
Created attachment 372671 [details] [review]
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.
Comment 13 Víctor Manuel Jáquez Leal 2018-06-13 16:25:17 UTC
Created attachment 372672 [details] [review]
vaapibufferpool: declare parameter display as object

We have neglected to update this code since GstVaapiDisplay turned
into a GstObject descendant.
Comment 14 Víctor Manuel Jáquez Leal 2018-06-13 16:25:25 UTC
Created attachment 372673 [details] [review]
libs: display: remove gst_vaapi_display_unref()

Use gst_object_unref() instead.
Comment 15 Víctor Manuel Jáquez Leal 2018-06-13 16:25:32 UTC
Created attachment 372674 [details] [review]
libs: display: remove gst_vaapi_display_ref()

Replace it with gst_object_ref()
Comment 16 Hyunjun Ko 2018-06-14 02:31:19 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #10)
> 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!
Comment 17 Víctor Manuel Jáquez Leal 2018-06-14 14:44:13 UTC
Attachment 372670 [details] pushed as 8de7dcf - libs: display: redefine gst_vaapi_display_create()
Attachment 372671 [details] pushed as a6881b9 - libs: display: replace gst_vaapi_display_new() with gst_vaapi_display_config()
Attachment 372672 [details] pushed as b1d8c68 - vaapibufferpool: declare parameter display as object
Attachment 372673 [details] pushed as fb1c4c5 - libs: display: remove gst_vaapi_display_unref()
Attachment 372674 [details] pushed as 6ccd5d6 - libs: display: remove gst_vaapi_display_ref()