[gstreamer-bugs] [Bug 317438] [speed] fix 0.9 port

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

Tim-Philipp Müller changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Patch to fixe               |[speed] fix 0.9 port
                   |gst/speed/gstspeed.c        |



------- Additional Comments From Tim-Philipp Müller  2005-10-04 15:26 -------
Some more comments:

 * There are still  references to the old buffer-frames stuff:
     - in GST_SPEED_AUDIO_CAPS
     - in the _GstSpeed structure in gstspeed.h
     - in speed_parse_caps()


 * in speed_setcaps():
     - This line
         filter = GST_SPEED (gst_pad_get_parent (pad));
       should be
         filter = GST_SPEED (GST_PAD_PARENT (pad));
       as _get_parent() increases the refcount in 0.9
     - These two assertions are not really necessary:
         g_return_val_if_fail (filter != NULL, GST_PAD_LINK_REFUSED);
         g_return_val_if_fail (GST_IS_SPEED (filter), GST_PAD_LINK_REFUSED);
       as GST_SPEED() will already throw a warning if filter isn't a
       GstSpeed object


  * stylistic issue in gst_speed_parse_caps():
     - This can be written nicer in 0.9:
         mimetype = gst_structure_get_name (structure);
         if (strcmp (mimetype, "audio/x-raw-float") == 0)
       can be replaced with
         if (gst_structure_has_name (structure, "audio/x-raw-float"))

  * in speed_src_event():
     - gst_pad_get_parent => GST_PAD_PARENT
     - you should only rewrite seek events as you do for formats you know, 
       ie. GST_FORMAT_DEFAULT, GST_FORMAT_TIME, GST_FORMAT_BYTES. For other
       formats, either unref the event and return FALSE or pray and send it
       on unmodified (example: you have cddasrc ! speed ! sink and the seek
       events seeks to CD track N - you don't really want to modify that).
     - you need to adjust stop_value as well if, and only if, stop_stype is
       not SEEK_TYPE_NONE.
     - break after return is not needed in the case statement
     - instead of
           event = newevent;
           return gst_pad_send_event (foopad, event);
       just do 
           return gst_pad_send_event (foopad, newevent);

  * in gst_speed_convert():
     - gst_pad_get_parent => GST_PAD_PARENT

  * in speed_src_query():
     - gst_pad_get_parent => GST_PAD_PARENT
     - must unref peer if gst_pad_query() fails as well
     - 'cur' is used uninitialised (no idea why the compiler
       doesn't warn about it in this case)
     - all variable declarations should be at the beginning
       of the code block (here: conv_format)
     - why do you query the peer in BYTES format and then convert
       to TIME format, instead of querying directly in TIME format?
     - you can use gst_pad_query_position() directly on the peer pad,
       without modifying the query object (less confusing maybe).
     - why don't you retrieve the current position when calling
         gst_query_parse_position()?

  * speed_change_state() isn't thread-safe, see porting guide in
    Plugin Writer's Guide appendix for details (first handle
    upwards state changes, then chain up to parent class, then
    handle downwards state changes)

  * gst_speed_chain() 
      - still leaks the input buffer
      - the code to align out_size to sample_size is wrong,
        which is not your fault though, but it's in 0.8 as well

  * speed_sink_event():
      - should handle and forward EOS (taking the STREAM_LOCK)
      - take the STREAM_LOCK while handling and forwarding the
        newsegment event
      - stop_value in event should be adjusted as well
      - should handle GST_FORMAT_DEFAULT (=samples)

That's all I could find for now :)

Hope you're not discouraged by the long list, most of it is trivial stuff or
style issues after all.

Btw, next time please add the patch as attachement in bugzilla (easier to download).

Cheers
 -Tim


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