[Bug 773463] core(debug): hard to distinguish related log at multi-instance env

[GStreamer (GNOME Bugzilla)]

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

Thibault Saunier <tsaunier at gnome.org> changed:

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

--- Comment #17 from Thibault Saunier <tsaunier at gnome.org> ---
Created attachment 341740
  --> https://bugzilla.gnome.org/attachment.cgi?id=341740&action=edit
(In reply to Tim-Philipp Müller from comment #16)

>Haven't looked at the meat of the patch yet, just some quick drive-by comments:
>>--- a/gst/gst_private.h
>>+++ b/gst/gst_private.h
>> 
>>+/* GstObject debugging representation level */
>>+gint object_repr_level;
>
>Don't think this will work properly everywhere. I think this should be
>   extern gint object_repr_level;
> in the header file and then
>  gint object_repr_level;
>
> in gstinfo.c or wherever.

-> Indeed, DONE.

> >--- a/gst/gstbin.c
> >+++ b/gst/gstbin.c
> 
> >+void
> >+gst_bin_update_children_repr (GstObject * object, GstObject * parent)
> >+{
> >+#ifndef GST_DISABLE_GST_DEBUG
> >+  ...
> >+#endif
> >+}
> 
> I think these #ifndef #endif should be outside of the entire function block
> and there should be a #define for the function in the header that expands
> for nothing if the logging system is disabled (and no prototype of course).
> 
> The reason to do it that way is that if you have code like below, the
> run-time object run-time checks will still be executed even though the
> function is empty. We don't want that in that case, we want it all to be a
> no-op I think.

Thought about it and checked, I think modern compilers should be able to elide
function with empty core, but your solution is safer indeed.

-> DONE anyway.

> >+  if (result)
> >+    gst_bin_update_children_repr (GST_OBJECT (element), GST_OBJECT (bin));
> 
> We know here that the objects are okay, right? So maybe GST_OBJECT_CAST to
> avoid the run-time check.
> 
>  
> >+  gst_object_ref (element);
> >   GST_TRACER_BIN_REMOVE_PRE (bin, element);
> >   result = bclass->remove_element (bin, element);
> >   GST_TRACER_BIN_REMOVE_POST (bin, result);
> > 
> >+  gst_bin_update_children_repr (GST_OBJECT_CAST (element),
> >+      result ? NULL : GST_OBJECT_CAST (bin));
> >+  gst_object_unref (element);
> 
> Why do we not just set it to NULL if result was TRUE? Wouldn't that keep it
> as before if not?

-> DONE

> I'm not a big fan of the "Repr" name, but not so important for now.

I do not really care, I just reused python naming here.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.