GNOME Bugzilla – Bug 747965
rtppayload: payload type could be inconsistent in some payloader, which have pre-defined pt by RFC standard
Last modified: 2015-08-16 13:40:44 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.
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.
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.
(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)
(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.
Created attachment 308827 [details] [review] rtppayload: set standard payload type as default and update dynamic flag in setcaps Applied to thiago's review.
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