GNOME Bugzilla – Bug 768266
vaapi: migrate GstVaapiDisplay as a GstObject descendant
Last modified: 2016-11-04 13:06:39 UTC
I've been surveying for gst tracer for leak to adjust vaapi. I think this tracer is very useful for gstreamer developers. But, unfortunately, gst vaapi mini object is not inherited from gst(mini)object. But it would be great if we could use this infrastructure. IMHO, There are 2 options 1. Replace gst(mini)object from current own vaapi object. I think this is the best option. We can use not only gst tracer but also all things supported, eg. gir, including future improvement! But it'll be very tough time to code and verify. 2. Make vaapi tracer to trace vaapi object. Actually, I've been doing this for a couple of days. I prepared vaapi tracer but it's not perfect for several reasons. - vaapi mini object doesn't have GType, which means i can't recognize by the name of leaked memory. ( I can recognize by only address) - Needs to modify gst-core lib (gsttracerutils) as far as I see. I want to listen to other's opinion.:)
I'm all for option 1\ But I would do it gradually and aiming to release 1.12 The first step would be GstVaapiDisplay to a GstObject in order to fix bug 766704. And then move onwards to other objects, choosing between use either GstObject or GstMiniObject.
Yeah I agree 1) is definitely the way to go.
I'm working on this. The first step, as Victor mentioned, is GstVaapiDisplay. For now, I finished with GstVaapiDisplay and GstVaapiDisplayX11/Wayland. Look into this. https://github.com/zzoon/gstreamer-vaapi/commits/gstcontext And it provides apis to share display, can be verified with testsuite. Once I finished, I'll uploads all patches in this thread.
We wrote the vaapi-mini object to make it as light as possible sine we create 100s of objects per second depends on framerate, number of slices and many other factors, and that is the reason we even avoided default memset to zero operations. I am not sure how much overhead gst_mini_object()/GType will introduce... :)
(In reply to sreerenj from comment #4) > We wrote the vaapi-mini object to make it as light as possible sine we > create 100s of objects per second depends on framerate, number of slices and > many other factors, and that is the reason we even avoided default memset to > zero operations. > > I am not sure how much overhead gst_mini_object()/GType will introduce... :) We have to do this step by step, first the objects exposed to the user (GstVaapiDisplay) and the minimum amount of mini-objects exposed to the tracer. All the slice related structures should be kept until we have measured and compared the overhead.
FWIW, the GstMiniObject does not use GType to to allocation, you're supposed to allocate some memory (usually with the slice allocator) and then do gst_mini_object_init() on it to set some values and destroy functions. It doesn't do memset or locks or anything like that.
(In reply to sreerenj from comment #4) > We wrote the vaapi-mini object to make it as light as possible sine we > create 100s of objects per second depends on framerate, number of slices and > many other factors, and that is the reason we even avoided default memset to > zero operations. > > I am not sure how much overhead gst_mini_object()/GType will introduce... :) Yes. Performance is very critical as you said. We have to be careful and verify how much the changes affect the performance. I started on GstVaapiDisplay first, which wouldn't kill performance as far as I think :)
Created attachment 331457 [details] [review] [PATCH 1/6] vaapidisplay: changes to be inherited from GstObject
Created attachment 331458 [details] [review] [PATCH 2/6] vaapidisplay_x11: changes to be inherited from GstObject
Created attachment 331459 [details] [review] [PATCH 3/6] vaapidisplay_wayland: changes to be inherited from GstObject
Created attachment 331460 [details] [review] [PATCH 4/6] vaapidisplay_drm: changes to be inherited from GstObject
Created attachment 331461 [details] [review] [PATCH 5/6] vaapidisplay_glx: changes to be inherited from GstObject
Created attachment 331462 [details] [review] [PATCH 6/6] vaapivideocontext: fix to use the existed vaapidisplay gtype Since changes in vaapidisplay, it doesn't need to define vaapidisplay gtype here.
Done except for vaapidisplay_egl. Maybe, it could be done once bug #767203 is closed.
Created attachment 331949 [details] [review] [PATCH 4/6] vaapidisplay_drm: changes to be inherited from GstObject by GstVaapiDisplay's change Rework on vaapidisplay_drm
Three things: 1\ I wonder if we should keep gstvaapidisplay*_priv.h header files for GstVaapiDisplay with this change. If we keep them, then we should use them by hiding all the non-public data structures. Or if we follow the normal gstreamer convertion, removing the gstvaapidisplay*_priv.h and exposing the data structures/methods. 2\ I think we should try to use the API for g_autoptr() 3\ The commit logs should be more detailed about what is the commit about and if that specific commit breaks the build or the normal usage of the elements. This important if we later need to run a bisect.
(In reply to Víctor Manuel Jáquez Leal from comment #16) > Three things: > > 1\ I wonder if we should keep gstvaapidisplay*_priv.h header files for > GstVaapiDisplay with this change. If we keep them, then we should use them > by hiding all the non-public data structures. Or if we follow the normal > gstreamer convertion, removing the gstvaapidisplay*_priv.h and exposing the > data structures/methods. That's what I worked at first. - expose public structures in *.h - hide private structures in *.c and provide apis to access private members. This would be much better than my current patches. But, it needs much more change from current code. We should do this at once? or do step by step. > 2\ I think we should try to use the API for g_autoptr() You mean g_autoptr support for vaapidisplay type? Good idea, but IMO, it needs in the second step when making it public. > 3\ The commit logs should be more detailed about what is the commit about > and if that specific commit breaks the build or the normal usage of the > elements. This important if we later need to run a bisect. Okay
Ah, another thing to use is, instead of G_DEFINE_TYPE, G_DEFINE_TYPE_WITH_PRIVATE or even perhaps G_DEFINE_TYPE_WITH_CODE (GstVaapiDisplay, gst_vaapi_display, GST_TYPE_OBJECT, G_ADD_PRIVATE (GstVaapiDisplay); GST_DEBUG_CATEGORY_INIT (GST_CAT_DEFAULT, "vaapidevice", 0, "VA-API Device")); If we want to have a debug category only for the GstVaapiDisplay
Created attachment 332760 [details] [review] [PATCH 1/7] vaapidisplay: changes to be inherited from GstObject
Created attachment 332761 [details] [review] [PATCH 2/7] vaapidisplay_x11: changes to be inherited from GstObject
Created attachment 332762 [details] [review] [PATCH 3/7] vaapidisplay_wayland: changes to be inherited from GstObject
Created attachment 332763 [details] [review] [PATCH 4/7] vaapidisplay_glx: changes to be inherited from GstObject
Created attachment 332764 [details] [review] [PATCH 5/7] vaapidisplay_egl: changes to be inherited from GstObject
Created attachment 332765 [details] [review] [PATCH 6/7] vaapidisplay_drm: changes to be inherited from GstObject
Created attachment 332766 [details] [review] [PATCH 7/7] vaapivideocontext: fix to use existed vaapidisplay gtype
Created attachment 332767 [details] [review] vaapidisplay: add g_autoptr support to GstVaapiDisplay type
Created attachment 336321 [details] [review] vaapidisplay: changes to be inherited from GstObject instead of GstVaapiMiniObject This patch is to be inherited from GstObject, instead of GstVaapiMiniObject, so that it can use GstObject's infrastructures such as GstTracer, GIR, etc. In addition, new debug category for GstVaapiDisplay is created to make it easier to trace debug messages. When trying to expose GstVaapiDisplay to public, this patch is also necessary to bind other languages through GIR. - Rebased on master. - I think it'd be better to make one patch than several seperated patches, because build should fail without all patches together.
Created attachment 336322 [details] [review] vaapidisplay: add g_autoptr support to GstVaapiDisplay type
Created attachment 336345 [details] [review] vaapidisplay: changes to be inherited from GstObject instead of GstVaapiMiniObject This patch is to be inherited from GstObject, instead of GstVaapiMiniObject, so that it can use GstObject's infrastructures such as GstTracer, GIR, etc. In addition, new debug category for GstVaapiDisplay is created to make it easier to trace debug messages. When trying to expose GstVaapiDisplay to public, this patch is also necessary to bind other languages through GIR. Reupload new version. Sorry for noise.
Created attachment 336962 [details] [review] vaapidisplay: changes to be inherited from GstObject instead of GstVaapiMiniObject This patch is to be inherited from GstObject, instead of GstVaapiMiniObject, so that it can use GstObject's infrastructures such as GstTracer, GIR, etc. In addition, new debug category for GstVaapiDisplay is created to make it easier to trace debug messages. When trying to expose GstVaapiDisplay to public, this patch is also necessary to bind other languages through GIR. - added to support g_autoptr to one patch - rework according to discuss with victor. 1. Change parameter type in gst_vaapi_display_new 2. Use one debug category in every type of vaapi display 3. Do not expose structures in vaapidisplay in this stage.
Created attachment 337550 [details] [review] vaapidisplay: changes to be inherited from GstObject instead of GstVaapiMiniObject Rebased on master
Created attachment 337551 [details] [review] vaapiobject: change vaapiobject to be inherited from GstMiniObject For performance, use GstMiniObject not GstObject. To use GstMiniObject for GstVaapiObject, 1. Cut the relation with GstVaapiMiniObject and inherit from GstMiniObject 2. Keep it up using object_class to support for subclassing 3. Make children of GstVaapiObject to be GBoxed type to give type name to each object.
(In reply to Hyunjun Ko from comment #32) > Created attachment 337551 [details] [review] [review] > vaapiobject: change vaapiobject to be inherited from GstMiniObject > > For performance, use GstMiniObject not GstObject. > > To use GstMiniObject for GstVaapiObject, > 1. Cut the relation with GstVaapiMiniObject and inherit from GstMiniObject > 2. Keep it up using object_class to support for subclassing > 3. Make children of GstVaapiObject to be GBoxed type to give type name to > each object. Note that the patch on #772554 should be merged, otherwise crash happens. I was trying to work on VaapiSurface first, but realized it's much more difficult. It's because vaapivideopool handles every type of vaapi object without type checking. Now I'm testing with this patch, I didn't see any problem yet. But we should keep verifying as much as we can. By the way, I found a leak in case of using wayland backend thanks to this patch :) I'm going to file this issue.
Created attachment 338939 [details] [review] vaapiwindow_egl: fix to pass native vaapidisplay during creation of native window When creating egl vaapiwindow, it also creates native window by its own native display. Fix to pass correct native display at this moment.
Review of attachment 337550 [details] [review]: while compiling I got this warning: gstvaapidisplay_wayland.c:212:27: warning: unused variable '_display' [-Wunused-variable] GstVaapiDisplayWayland *_display = GST_VAAPI_DISPLAY_WAYLAND_CAST (display); ^ Anyway, as these are simple changes I'll change them and push this patch. ::: gst-libs/gst/vaapi/gstvaapidisplay.c @@ +1113,2 @@ GstVaapiDisplay * +gst_vaapi_display_new (GstVaapiDisplay * display, 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. ::: gst-libs/gst/vaapi/gstvaapidisplay_drm_priv.h @@ +79,3 @@ + gchar *device_path; + gint drm_device; + guint use_foreign_display:1; // Foreign native_display? Let's keep the C90 comment style ::: gst-libs/gst/vaapi/gstvaapidisplay_glx_priv.h @@ +48,3 @@ /*< private >*/ GstVaapiDisplayX11 parent_instance; + GstVaapiTextureMap * texture_map; This change is not required. ::: gst-libs/gst/vaapi/gstvaapidisplay_wayland.c @@ +38,3 @@ +#define _do_init \ + G_ADD_PRIVATE (GstVaapiDisplayWayland); Did you forget the debug category initialization? ::: gst-libs/gst/vaapi/gstvaapidisplay_x11.c @@ +48,3 @@ +#define _do_init \ + G_ADD_PRIVATE (GstVaapiDisplayX11); Did you forget the debug category initialization?
Review of attachment 337550 [details] [review]: According to GObject conventions [1]: Create a macro named PREFIX_TYPE_OBJECT which always returns the GType for the associated object type. Our prefix is GstVaapi, not Gst, we have to change all the macros. 1. https://developer.gnome.org/gobject/stable/gtype-conventions.html ::: gst-libs/gst/vaapi/gstvaapidisplay_priv.h @@ +40,1 @@ +GType gst_vaapi_display_get_type (void); we should set the attribute G_GNUC_CONST to all the _get_type() functions http://wingolog.org/archives/2005/03/24/98
Review of attachment 337551 [details] [review]: I don't feel this right. GstVaapi{codecbuffer, context, image, subpicture, surface, pixmap_*, texture_*, window_*} should be GstMiniObject directly, not this convoluted way to share code with GstVaapiObject/GstVaapiMiniObject. The purpose it to remove, in the future GstVaapiObject and GstVaapiMiniObject.
@Hyunjun: I have pushed in my repo a dev branch https://github.com/ceyusa/gstreamer-vaapi/commits/dev There I pushed a modified version of your GstVaapiDisplay. Please check it and tell me if that's OK.
Review of attachment 337550 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapidisplay.c @@ +1113,2 @@ GstVaapiDisplay * +gst_vaapi_display_new (GstVaapiDisplay * display, Okay. I'll keep it in mind ::: gst-libs/gst/vaapi/gstvaapidisplay_priv.h @@ +40,1 @@ +GType gst_vaapi_display_get_type (void); Aha, good catch! ::: gst-libs/gst/vaapi/gstvaapidisplay_wayland.c @@ +38,3 @@ +#define _do_init \ + G_ADD_PRIVATE (GstVaapiDisplayWayland); Well, I thought it's not neccesary because this category is already initialized in gstvaapidisplay.c and could be refered by GST_DEBUG_CATEGORY_EXTERN. ::: gst-libs/gst/vaapi/gstvaapidisplay_x11.c @@ +48,3 @@ +#define _do_init \ + G_ADD_PRIVATE (GstVaapiDisplayX11); same here.
(In reply to Víctor Manuel Jáquez Leal from comment #37) > Review of attachment 337551 [details] [review] [review]: > > I don't feel this right. > > GstVaapi{codecbuffer, context, image, subpicture, surface, pixmap_*, > texture_*, window_*} should be GstMiniObject directly, not this convoluted > way to share code with GstVaapiObject/GstVaapiMiniObject. > > The purpose it to remove, in the future GstVaapiObject and > GstVaapiMiniObject. I understand what you mean. But IIUC, GstMiniObject doesn't support inheritance properly. Probably, this is also the reason why GstVaapiObject exists, because GstVaapiMiniObject doesn't either. If we remove GstVaapiObject totally, it means, we need to create something similar to what GstVaapiObject's doing.
(In reply to Víctor Manuel Jáquez Leal from comment #38) > @Hyunjun: > > I have pushed in my repo a dev branch > > https://github.com/ceyusa/gstreamer-vaapi/commits/dev > > There I pushed a modified version of your GstVaapiDisplay. Please check it > and tell me if that's OK. I've been testing this branch since this morning, and I think it is okay.
(In reply to Víctor Manuel Jáquez Leal from comment #36) > Review of attachment 337550 [details] [review] [review]: > > According to GObject conventions [1]: > > Create a macro named PREFIX_TYPE_OBJECT which always returns the GType for > the associated object type. > > Our prefix is GstVaapi, not Gst, we have to change all the macros. The convention in GStreamer seems to be that PREFIX is Gst as they all start with GST_TYPE_*. I'm about to finish changing libgstgl to the same format for the four or so #defines that are left. i.e for GST_TYPE_* find /usr/include/gstreamer-1.0/ -type f -name '*.h' -exec grep '#define\s+GST_TYPE_.*' \{\} \; for GST_*_TYPE_* find /usr/include/gstreamer-1.0/ -type f -name '*.h' -exec grep '#define\s+GST_[a-zA-Z]*_TYPE_.*' \{\} \; Only false-positives and the few libgstgl ones.
(In reply to Matthew Waters (ystreet00) from comment #42) > (In reply to Víctor Manuel Jáquez Leal from comment #36) > > Review of attachment 337550 [details] [review] [review] [review]: > > > > According to GObject conventions [1]: > > > > Create a macro named PREFIX_TYPE_OBJECT which always returns the GType for > > the associated object type. > > > > Our prefix is GstVaapi, not Gst, we have to change all the macros. > > The convention in GStreamer seems to be that PREFIX is Gst as they all start > with GST_TYPE_*. Gee! Thanks Matthew! You were right from the beginning, Hjunyun!
(In reply to Hyunjun Ko from comment #39) > > +#define _do_init \ > + G_ADD_PRIVATE (GstVaapiDisplayWayland); > > Well, I thought it's not neccesary because this category is already > initialized in gstvaapidisplay.c and could be refered by > GST_DEBUG_CATEGORY_EXTERN. Again, you were right. Sorry for the noise.
@Hyunjun I'm going to close the bug, targeted only to GstVaapiDisplay migration. Please open another bug for the other base class migrations.