GNOME Bugzilla – Bug 415001
Add a min-ptime property to the GstBaseRTPAudioPayload class
Last modified: 2007-05-21 09:50:09 UTC
The Base rtp class supports a max-ptime property, but no min-ptime. In some cases, the applications wants to force the payloader to send bigger packets (which will reduce the total bandwidth used. I'm attaching 2 patches. One which adds the property to the Audio Class (without changing the abi and it defaults to the old behavior if the min-ptime property is not set). The second one moves the min-ptime property to the GstBaseRTPPayload class and adds a callback to be able to handle pad events in the childs of the class if needed.
Created attachment 83989 [details] [review] Adds a min-ptime property to GstBaseRTPAudioPayload
Created attachment 83990 [details] [review] Move min-ptime to the base class and add event callback I forgot to mention that the reason the patch is separated in two phases is that the first phase does not change the ABI while the second one does.
Created attachment 84019 [details] [review] new version of the patch New version of the patch that fixes some bugs and exports the GstAdapter and the push function so that subtypes can use it
Commited second patch. First patch shouldn't break ABI because of available padding. Will review it later.
Couple of general comments: - Olivier: please create patches with cvs diff -p next time, so that the function names appear in the diff. This makes them much easier to read. About the committed patch: - new core/base API should usually be reviewed/approved by a core developer before being committed. - the basertpaudiopayload->priv->disposed stuff is redundant - you need to chain up to the parent class in the dispose function - basertpaudiopayload->priv->adapter is created in the instance init function and exists during the entire life cycle of the payloader object - why is the code littered with if (basertpaudiopayload->priv->adapter) gst_adapter_foo(..) ? - you added this API in gstbasertpaudioplayload.h: GstFlowReturn gst_base_rtp_audio_payload_push (GstBaseRTPPayload * basepayload, const guint8 * data, guint payload_len, GstClockTime timestamp); The first argument should probably be of type GstBaseRTPAudioPayload * About the other patch: - the padding needs to be adjusted if you add fields to the structures
Created attachment 84658 [details] [review] patch with wim's suggestions Ok, I've fixed the disposed and the payload_push function. As for the tests on the existence of the adaper, the GObject documentation states that members functions may be called after the dispose function has been called, so we have to test every time.
Created attachment 84659 [details] [review] Move min-ptime to basepayloader and remain ABI compatible I still have one thing that I'm not happy with my patch.. since I'm adding a guint64, it will be one pointer on a 64bit machine but 2 on a 32bit one.. I'm not sure how to properly reduce the padding..
Created attachment 84660 [details] [review] Fixes to comments in comment #5
- new core/base API should usually be reviewed/approved by a core developer before being committed. Fine by me but who are considered the core developers?
It would probably be a good idea to push these patches in before the next release, particularly attachment #84660 [details].
> It would probably be a good idea to push these patches in before the next > release, particularly attachment #84660 [details] [edit]. Those bits were committed: 2007-04-21 Tim-Philipp Müller <tim at centricular dot net> Patch by: Olivier Crete <tester at tester ca> * gst-libs/gst/rtp/gstbasertpaudiopayload.c: (gst_base_rtp_audio_payload_class_init), (gst_base_rtp_audio_payload_init), (gst_base_rtp_audio_payload_dispose): Chain up to parent class in dispose function; get rid of unnecessary 'diposed' flag in private structure (#415001). 2007-04-21 Tim-Philipp Müller <tim at centricular dot net> Patch by: Zeeshan Ali <zeenix gmail com> * gst-libs/gst/rtp/gstbasertpaudiopayload.c: (gst_base_rtp_audio_payload_handle_frame_based_buffer), (gst_base_rtp_audio_payload_handle_sample_based_buffer), (gst_base_rtp_audio_payload_push): * gst-libs/gst/rtp/gstbasertpaudiopayload.h: The recently-added gst_base_rtp_audio_payload_push() should take an object of type GstBaseRTPAudioPayload as first argument (#431672). Looks like I forgot to update the patch status, sorry.
what about the other patch ?
* gst-libs/gst/rtp/gstbasertpaudiopayload.c: (gst_base_rtp_audio_payload_class_init), (gst_base_rtp_audio_payload_init), (gst_base_rtp_audio_payload_finalize), (gst_base_rtp_audio_payload_handle_frame_based_buffer), (gst_base_rtp_audio_payload_handle_sample_based_buffer), (gst_base_rtp_payload_audio_handle_event): Some cleanups, remove minptime property as it is now in the parent class. Override parent class event function. * gst-libs/gst/rtp/gstbasertppayload.c: (gst_basertppayload_class_init), (gst_basertppayload_init), (gst_basertppayload_event), (gst_basertppayload_set_property), (gst_basertppayload_get_property): * gst-libs/gst/rtp/gstbasertppayload.h: Add min-ptime property. Add handle-event vmethod. Fixes #415001.