[gstreamer-bugs] [Bug 303975] Add tar support

[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=303975
 GStreamer | gst-plugins-bad | Ver: HEAD CVS





------- Comment #12 from Tim-Philipp Müller  2006-02-19 19:43 UTC -------
That's quite cool, I wonder though ... how does tarenc know when to start a new
file in this scenario? Glancing at the code it looks like it starts a new file
whenever there's a TAG event. If that's the case, then that's not really
entirely right, even if it happens to work in this scenario. Tag events can be
sent at any time, sometimes multiple tag events are sent for different
information, and sometimes tag events are sent to update information (e.g. mad
_could_ send a tag event whenever the averate bitrate of an VBR mp3 file
changes, that would be perfectly legitimate).

I'm not sure what the solution to this problem is though. Maybe a new event
that signals the start of a new stream unit? We don't really have a good
abstraction for multiple streams yet in GStreamer, be it on the input side or
on the sink side. That should probably be solved first. A great thing to bring
up on the mailing list with different use-cases that need to be handled.

(On a side note, 'continuous mode' in cdiocddasrc is supposed to treat the
entire CD as one single stream, so one might even argue that it would be wrong
if tarenc split this up into separate files; I also imagine there might be
issues with missing ogg headers for the files that are created after the first
one; could easily be solved by introducing a third mode of course once we know
how to signal these things downstream properly).

Writing a temporary file isn't really great either, it kind of breaks the whole
concept of GStreamer pipelines as I see it. If you just need to fill the total
number of bytes into some header when you're done, you could send a NEWSEGMENT
event downstream at the end with a seek position as start and then send a small
buffer containing the final length in bytes. That will update the header with
the correct information.


Finally, allow me some minor nitpicks about the code:

 * all files should have a line with your copyright in the header

 * elements usually store the sink and source pads they create in their
   element structure, so they can easily be accessed; it tends to make
   code more readable and avoids
     foo = gst_element_get_pad (GST_ELEMENT (b), "sink")
   all over the place (incl. the refcounting involved with that).  

 * assertions like 

     g_return_if_fail (GST_IS_TARENC (b));

   aren't really necessary for internal functions. Some people like
   to use them for external ones, but I have yet to see a single case
   where any such assertion was ever triggered.

 * in the event function you do (stylized):

     gst_tarenc_event (GstPad * pad, GstEvent * e)
     {
       GstTarenc *b = GST_TARENC (gst_pad_get_parent (pad));

       gst_object_unref (b);

       ... function code ...
     }

   That kind of defeats the purpose of refcounting. The reference
   should be held until the end of the function, especially in
   event and query functions.

 * you use 'GstTarenc *b' everywhere - IMHO something like
   'GstTarenc *enc' or 'GstTarenc *tar' would be a tad nicer and
   would increase code readability, but that might just be me.


 *   parent_class = g_type_class_ref (GST_TYPE_ELEMENT);
   should be
     parent_class = g_type_class_peek_parent (klass);
   even if you'll still find the _ref() in hundreds of old
   plugins. You can also use GST_BOILERPLATE, then you don't
   have to do the parent_class thing or the _get_type()
   function at all :)

 * there's a gst_structure_has_name() call now in 0.10 :)

 * it should be GstTarDec and gst_tar_dec_* and GST_TAG_DEC instead
   of GstTardec, gst_tardec and GST_TARDEC (very minor, but makes
   things more consistent with the rest of gst).


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