GNOME Bugzilla – Bug 796470
vaapidisplay: remove calling gst_vaapi_display_new in each descendant.
Last modified: 2018-06-14 14:45:26 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
Created attachment 372500 [details] [review] libs: display: remove unused code
Created attachment 372501 [details] [review] libs: display: remove unnecessary legacy code since gobjectification
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.
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.
Review of attachment 372500 [details] [review]: lgtm
Review of attachment 372501 [details] [review]: lgtm
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
(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.
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()
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.
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().
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.
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.
Created attachment 372673 [details] [review] libs: display: remove gst_vaapi_display_unref() Use gst_object_unref() instead.
Created attachment 372674 [details] [review] libs: display: remove gst_vaapi_display_ref() Replace it with gst_object_ref()
(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!
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()