After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 415001 - Add a min-ptime property to the GstBaseRTPAudioPayload class
Add a min-ptime property to the GstBaseRTPAudioPayload class
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal enhancement
: 0.10.6
Assigned To: Wim Taymans
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2007-03-05 18:12 UTC by Olivier Crête
Modified: 2007-05-21 09:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Adds a min-ptime property to GstBaseRTPAudioPayload (14.87 KB, patch)
2007-03-05 18:13 UTC, Olivier Crête
none Details | Review
Move min-ptime to the base class and add event callback (7.97 KB, patch)
2007-03-05 18:15 UTC, Olivier Crête
none Details | Review
new version of the patch (16.13 KB, patch)
2007-03-05 23:10 UTC, Olivier Crête
committed Details | Review
patch with wim's suggestions (2.51 KB, patch)
2007-03-15 16:04 UTC, Olivier Crête
none Details | Review
Move min-ptime to basepayloader and remain ABI compatible (9.06 KB, patch)
2007-03-15 16:38 UTC, Olivier Crête
committed Details | Review
Fixes to comments in comment #5 (4.17 KB, patch)
2007-03-15 16:38 UTC, Olivier Crête
committed Details | Review

Description Olivier Crête 2007-03-05 18:12:56 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.
Comment 1 Olivier Crête 2007-03-05 18:13:46 UTC
Created attachment 83989 [details] [review]
Adds a min-ptime property to GstBaseRTPAudioPayload
Comment 2 Olivier Crête 2007-03-05 18:15:18 UTC
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.
Comment 3 Olivier Crête 2007-03-05 23:10:40 UTC
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
Comment 4 Philippe Khalaf 2007-03-14 21:20:06 UTC
Commited second patch. First patch shouldn't break ABI because of available padding. Will review it later.
Comment 5 Tim-Philipp Müller 2007-03-15 09:20:34 UTC
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


Comment 6 Olivier Crête 2007-03-15 16:04:28 UTC
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.
Comment 7 Olivier Crête 2007-03-15 16:38:05 UTC
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..
Comment 8 Olivier Crête 2007-03-15 16:38:58 UTC
Created attachment 84660 [details] [review]
Fixes to comments in comment #5
Comment 9 Philippe Khalaf 2007-03-15 19:14:49 UTC
- 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?
Comment 10 Olivier Crête 2007-05-07 22:49:22 UTC
It would probably be a good idea to push these patches in before the next release, particularly attachment #84660 [details].
Comment 11 Tim-Philipp Müller 2007-05-08 08:09:02 UTC
> 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.
Comment 12 Olivier Crête 2007-05-08 12:47:35 UTC
what about the other patch ?
Comment 13 Wim Taymans 2007-05-21 09:50:09 UTC
        * 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.