GNOME Bugzilla – Bug 794331
log_omx_performance: GstOMXBuffer and OMX-buffer fields always displayed as NULL
Last modified: 2018-04-06 11:41:16 UTC
Here is the logging code used by the OMX_PERFORMANCE category to log infos about OMX buffers: https://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomx.c#n648 Unfortunately this is not working with G_TYPE_POINTER fields which appear as NULL and raise warnings: 0:00:00.443497969 11812 0x2538d40 WARN structure gststructure.c:1832:priv_gst_structure_append_to_gstring: No value transform to serialize field 'GstOMXBuffer' of type 'gpointer' 0:00:00.443510229 11812 0x2538d40 WARN structure gststructure.c:1832:priv_gst_structure_append_to_gstring: No value transform to serialize field 'OMX-buffer' of type 'gpointer' 0:00:00.443465370 11812 0x2538d40 TRACE OMX_PERFORMANCE gstomx.c:673:log_omx_performance:<omxh264enc-omxh264enc0> FillThisBuffer, GstOMXBuffer=(gpointer)NULL, OMX-buffer=(gpointer)NULL, TimeStamp=(guint64)0, AllocLen=(uint)360448, FilledLen=(uint)0, flags=(uint)0, flags-str=(string)""; I initially assumed this was because I was using a old gst version on my board as I added support to display pointers a while ago in the leaks tracer ( https://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=aa336008b378ebe24d56896f96240fc3bd96d5d4 ) But actually tracers use a different code path, priv__gst_structure_append_template_to_gstring(), which is a private API while GST_PTR_FORMAT() uses gst_value_serialize() not supporting pointers serialization. It doesn't really make sense for gst_value_serialize() to serialize pointers so I think we have two options here: a) convert pointers to strings using g_strdup_printf() and store them as G_TYPE_STRING in the structure. b) manually handle pointers in priv_gst_structure_append_to_gstring() (used by gst_structure_to_string()) to display them properly
Created attachment 369681 [details] [review] log_omx_performance: convert pointers to strings G_TYPE_POINTER are not serialized in logs.
Comment on attachment 369681 [details] [review] log_omx_performance: convert pointers to strings So in this code we're creating a structure and printing that so that a log parser can easily deserialise it right? How about instead of using a temporary GstStructure and printing it via GST_PTR_FORMAT, we just print the entire thing in GST_CAT_TRACE_OBJECT() in a format that can be deserialised later? It's not like we have complicated fields here such as strings that need to be escaped or such?
(In reply to Tim-Philipp Müller from comment #2) > So in this code we're creating a structure and printing that so that a log > parser can easily deserialise it right? Yep. > How about instead of using a temporary GstStructure and printing it via > GST_PTR_FORMAT, we just print the entire thing in GST_CAT_TRACE_OBJECT() in > a format that can be deserialised later? It's not like we have complicated > fields here such as strings that need to be escaped or such? That was my initiale approach but I switched to GstStructure as we already have from/to string API saving to re-implement them in gst-omx and log parsers. I think the first question is: should gst_structure_to_string() handle pointers? There is a FIXME about that: https://cgit.freedesktop.org/gstreamer/gstreamer/tree/gst/gststructure.c#n1833 If yes then it makes sense to fix it and keep using it here. If not we can either convert to string (my patch) or manually display each field. I don't have a strong opinion here but still think we should re-use existing serialization code if possible.
I don't know yet about that FIXME, I haven't looked at that yet. However, just doing the printing directly would be less code and also be more efficient as it avoids lots of alloc/free churn for the structure and its contents here. So while it might be cleaner to use GstStructure, it has overhead that seems unnecessary.
From memory, I think pointers being serialised to NULL is very confusing in general when reading debug logs, so I think we should probably fix that to print the actual pointer value. (However, I would be opposed to deserializing any pointers from the string values later.)
Agreed, I've fixed that in bug #794355
While my patch in bug #794355 makes sense, I'd still prefer to convert pointers to strings here, so my patch above still hold. My main reason for using a GstStructure here is to be able to use gst_structure_from_string() in my log parser. But it will fail atm because of the pointer fields. It doesn't make sense to deserialize pointers so, in this case, converting to strings seems the right thing to do. Tim: so, any objection if I merge this patch?
I'm not sure I follow. Your comment doesn't seem relevant as to which approach is preferable for printing. You can still use gst_structure_from_string() in your log parser even if you print stuff into the log without creating a GstStructure. The string in the debug log would be identical in either case, just that it's less overhead and less code if you don't create a throw-away GstStructure. If you prefer your patch with the throwaway GstStructure, by all means go for it, in the grand scheme of things it doesn't really matter much :)
Oh sorry, I misunderstood what you were proposing. I tried your suggestion but I ended up hitting escaping issues as the flags string can contain ','. So I decided to not bother and I'm just gonna merge this patch. :)
Attachment 369681 [details] pushed as 021b668 - log_omx_performance: convert pointers to strings
Also pushed to 1.14: 89e04661f71fe4d3501cf7c2a2f54ec0c00af3fc