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 747965 - rtppayload: payload type could be inconsistent in some payloader, which have pre-defined pt by RFC standard
rtppayload: payload type could be inconsistent in some payloader, which have ...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal normal
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 746445
 
 
Reported: 2015-04-16 05:40 UTC by Hyunjun Ko
Modified: 2015-08-16 13:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
rtppayload: set standard payload type as default and update it in setcaps (11.36 KB, patch)
2015-08-05 11:22 UTC, Hyunjun Ko
none Details | Review
rtppayload: set standard payload type as default and update dynamic flag in setcaps (13.57 KB, patch)
2015-08-06 02:37 UTC, Hyunjun Ko
committed Details | Review

Description Hyunjun Ko 2015-04-16 05:40:49 UTC
This is related to the issue https://bugzilla.gnome.org/show_bug.cgi?id=747839

Sometimes app set dynamic payload type, not standard payload type, the pt is changed to standard later (or not for other payloaders).
This causes inconsistent result during rtp streaming.

[ Things needed to be consistent ]
1.  Its payload type has to be set in caps in src_template
All done

2. _init function has to set payload type initially (for the case that app is not setting pt property)
Not done:
gstrtpg722pay
gstrtph263pay
gstrtppcmapay
gstrtppcmupay
gstrtpmpapay
gstrtpjpegpay
gstrtpL16pay

3. _setcaps needs to update payload type.
Not done:
gstrtpg722pay
gstrtpgsmpay
gstrtpmp2tpay
gstrtpmpapay
gstrtpmpvpay
gstrtpjpegpay
gstrtpL16pay

Is there any other things to be investigated? Or Did I miss something?
If so, let me know please.
Comment 1 Hyunjun Ko 2015-08-05 11:22:20 UTC
Created attachment 308781 [details] [review]
rtppayload: set standard payload type as default and update it in setcaps

To be consistent, set standard payload type as default and update in setcaps.
Comment 2 Thiago Sousa Santos 2015-08-05 14:11:02 UTC
Review of attachment 308781 [details] [review]:

Good findings, most of the cleanups make sense but still some things not right.

The caps on the setcaps is from the sink pad, so it should not have the payload field as it is usually just the media, the payload field is for the RTP caps that will be set on the source pad. Instead of verifying if the caps has the 'payload' field just don't overwrite the pt variable in setcaps and it should be fine. So remove that but keep the having the payload type set in the init function.

Also updating the set_options call to check if the pt is the default or dynamic seems to make sense.

::: gst/rtp/gstrtpg722pay.c
@@ +139,3 @@
   clock_rate = 8000;
 
+  gst_rtp_base_payload_set_options (basepayload, "audio", FALSE, "G722",

Shouldn't this be conditional if the pt being used is dynamic?

::: gst/rtp/gstrtpgsmpay.c
@@ +110,3 @@
 
+  if (!gst_structure_get_int (structure, "payload", &pt))
+    pt = GST_RTP_PAYLOAD_GSM;

This is the sink caps, can we have a "payload" here? Seems unusual.
Comment 3 Hyunjun Ko 2015-08-05 15:18:27 UTC
(In reply to Thiago Sousa Santos from comment #2)
 
> ::: gst/rtp/gstrtpg722pay.c
> @@ +139,3 @@
>    clock_rate = 8000;
>  
> +  gst_rtp_base_payload_set_options (basepayload, "audio", FALSE, "G722",
> 
> Shouldn't this be conditional if the pt being used is dynamic?

As I told you through IRC, I guess dynamic type needs to be added src caps.
And then, it should be conditional.

> 
> ::: gst/rtp/gstrtpgsmpay.c
> @@ +110,3 @@
>  
> +  if (!gst_structure_get_int (structure, "payload", &pt))
> +    pt = GST_RTP_PAYLOAD_GSM;
> 
> This is the sink caps, can we have a "payload" here? Seems unusual.

Agree. 
The existed logic to get payload type in setcaps function needs to be removed?
(eg. g723pay, g729pay)
Comment 4 Thiago Sousa Santos 2015-08-05 15:20:54 UTC
(In reply to Hyunjun from comment #3)
> (In reply to Thiago Sousa Santos from comment #2)
> > 
> > ::: gst/rtp/gstrtpgsmpay.c
> > @@ +110,3 @@
> >  
> > +  if (!gst_structure_get_int (structure, "payload", &pt))
> > +    pt = GST_RTP_PAYLOAD_GSM;
> > 
> > This is the sink caps, can we have a "payload" here? Seems unusual.
> 
> Agree. 
> The existed logic to get payload type in setcaps function needs to be
> removed?
> (eg. g723pay, g729pay)

I think so, I wouldn't expect the sink caps to have the payload set.
Comment 5 Hyunjun Ko 2015-08-06 02:37:18 UTC
Created attachment 308827 [details] [review]
rtppayload: set standard payload type as default and update dynamic flag in setcaps

Applied to thiago's review.
Comment 6 Thiago Sousa Santos 2015-08-06 04:43:12 UTC
Thanks for the update. Please add more details to the commit message in next commits. I fixed this one for you.

commit 5a17572119e4164c93f2ea90fa64e1c179b6e3c5
Author: Hyunjun Ko <zzoon.ko@samsung.com>
Date:   Thu Aug 6 11:33:37 2015 +0900

    rtppayload: set standard payload type as default
    
    Initialize the PT to the default value of the codec and check if
    it is still the default before declaring the pt to be dynamic or
    not when setting the caps.
    
    Also use the PT constants from the rtp lib when possible