[gstreamer-bugs] [Bug 333501] [patch] taglib element

[GStreamer (bugzilla.gnome.org)]

Do not reply to this via email (we are currently unable to handle email
responses and they get discarded).  You can add comments to this bug at
http://bugzilla.gnome.org/show_bug.cgi?id=333501
 GStreamer | gst-plugins-good | Ver: HEAD CVS


Tim-Philipp Müller changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1




------- Comment #5 from Tim-Philipp Müller  2006-03-06 11:11 UTC -------
This is quite cool :)

Some random comments about the code:

 * the class description string in the element details should probably be
     Formatter/Metadata  (as per the new gstreamer/docs/design/draft-klass.txt)

 * the parent_class=... line in _class_init() isn't required when you use
   GST_BOILERPLATE, GST_BOILERPLATE will do that for you

 * GST_BOILERPLATE_WITH_INTERFACE is probably not the right thing to use here,
   it's meant for interfaces that are only implemented under special
   circumstances (like GstMixer) and that are wrapped by
GstImplementsInterface.
   GstTagSetter is a normal interface though that doesn't need wrapping with
   GstImplementsInterface, so GST_BOILERPLATE_FULL might be more appropriate.

 * it's always good to use GST_DEBUG_FUNCPTR() IMHO, like this:
    gst_pad_set_chain_function (taglib->sinkpad,
        GST_DEBUG_FUNCPTR (gst_taglib_chain));
   That will make any debug output much more readable later on. Same goes
   for vfuncs. Just a minor detail of course.

 * in gst_taglib_render_tag() I personally wouldn't worry about how expensive
   memcpy() is - it's only done once and the buffer size is pretty small too.
     buf = gst_buffer_new_and_alloc (rendered_tag.size());
     memcpy (GST_BUFFER_DATA (buf), rendered_tag.data(), rendered_tag.size());
   or something similar should do the job just fine IMHO.

 * in gst_taglib_chain():
     - I wouldn't _join() the incoming buffer and the tag buffer, I'd rather
       push out the tag separately as a single buffer, and then push the
       incoming buffer out after that. No particular reason, just seems
       cleaner somehow (and easier to debug later on).
     - you should set new caps on the buffers you send out with
         gst_buffer_set_caps (buf, GST_PAD_CAPS (taglibmux->srcpad));
       or so.

 * in gst_taglib_sink_event():
     - in case of GST_EVENT_TAG, wouldn't we want to send a new TAG event
       with the merged tags downstream, rather than just forwarding the
       tags we got?
     - in case of GST_EVENT_NEWSEGMENT: I think it should be
         result = gst_pad_push_event (...);
       instead of
         gst_pad_push_event (...);
         result = TRUE;
       (not that I can think of a case where it really matters in practice,
        just seems like the right thing to do)

 * in gst_taglib_change_state():
   you need to chain up to the parent class before handling a downwards
   (PAUSED=>READY) state change. Only upwards state changes are handled
   before chaining up to the parent class.


I'm not entirely sure about the element name. Personally, I'd prefer something
like 'tagid3mux' or maybe even 'id3v2mux' or 'tagid3v2mux' - that leaves us
more options to add other muxer elements based on TagLib later on (like
tagapemux or so). I think at least the fact that it is muxing ID3 tags should
somehow be reflected by the name of the element.


-- 
Configure bugmail: http://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.