[gstreamer-bugs] [Bug 356882] [PLUGIN-MOVE] synaesthesia has been ported

[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=356882

  GStreamer | gst-plugins-ugly | Ver: HEAD CVS





------- Comment #5 from Tim-Philipp Müller  2007-02-10 13:52 UTC -------
> @tim, are you okay if I enable builing it in ugly by default (and add the docs
> at the same time)?

I'd prefer to stick to the procedure outlined in the moving-plugins document.


The code still doesn't really look ready, and you haven't even fixed all of the
things I mentioned above (setcaps still returns GST_PAD_LINK_REFUSED, no
GST_DEBUG_FUNCPTR for gst_pad_set_xyz_func, still leaks references in chain
function). Mostly minor things, but still.

A few more things I noticed just now:

 - in _setcaps(): "fraction" is a GstFraction, not a double

 - if output height and width are really fixed, the caps should just
   reflect that instead of claiming to support any height/width and
   then refusing everything that's not 320x240 (see _setcaps()). It's
   worth noting though that a visualisation plugin that only supports
   a fixed height/width won't work properly with totem in its current
   state. IMHO the plugin should be rewritten to be able to support
   any height/width. If it needs to close/re-init the engine when
   height/width change in order to achieve that, so be it.

 - doesn't use GST_BOILERPLATE macros

 - doesn't use GST_(DEBUG/*)_OBJECT

 - caps nego in the chain function looks dodgy, to say the least
   (lines 373ff); I think it's mostly luck and implicit assumptions
   that make it work here.

 - the entire loop bit in the chain function looks a bit messy. I
   don't really understand why there's a MAX in

    while (gst_adapter_available (synaesthesia->adapter) >
        MAX (bytesperread,
            samples_per_frame * synaesthesia->channels * sizeof (gint16))) {

   but then you gst_adapter_peek(adapter, bytesperread) and then you
    gst_adapter_flush (adapter, samples_per_frame *
        synaesthesia->channels * sizeof (gint16));

   Usually, available/peek/flush should match up.



> for the tests I have no idea yet, but I could do a skeleton test that feeds
> one buffer through, to get the valgrind coverage at least.

Yeah, something simple should be fine, although it would be good if the test
demonstrated on-the-fly output size renegotiation as well (since that's what
e.g. totem does).


-- 
Configure bugmail: http://bugzilla.gnome.org/userprefs.cgi?tab=email