[Bug 747965] rtppayload: payload type could be inconsistent in some payloader, which have pre-defined pt by RFC standard

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=747965

Thiago Sousa Santos <thiagossantos at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #308781|none                        |needs-work
             status|                            |

--- Comment #2 from Thiago Sousa Santos <thiagossantos at gmail.com> ---
Review of attachment 308781:
 --> (https://bugzilla.gnome.org/review?bug=747965&attachment=308781)

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.

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