[vlc-devel] Patch to specify audio/video payload type for RTP streaming

[Andreas Granig]

Hello Rémi,

Thanks for your comments, see mine inline and an updated patch attached.

On 04/13/2014 02:35 PM, Rémi Denis-Courmont wrote:
> Le dimanche 13 avril 2014, 14:15:38 Andreas Granig a écrit :
>> Now if you do SDP offer/answer instead, the requesting client is sending
>> an SDP offer with the codecs it supports, plus the payload type it
>> wishes to use for those codecs.
> 
> Not really. In O/A, the sender must follow the payload format(s) and the 
> transport parameters specified by the receiver. There is a lot more to that 
> than the payload type number. Well it might work in some special case.

Absolutely, it was just meant to provide some context for this
particular patch. It's not like you can now put an SDP offer in and
expect a matching SDP answer out.

However, when doing O/A, there is usually a lot more involved which VLC
in itself is not going to handle (like the signalling part, and stuff
like ICE negotation etc), so you'd construct your own SDP in the
application controlling VLC anyways. Having said that, the only thing
which is necessary to make this work on the streaming level itself is
setting the ptype id, which is in fact the only thing the patch does.

> +#define PTYPE_AUDIO_TEXT N_("Audio payload type")
> +#define PTYPE_AUDIO_LONGTEXT N_( \
> +    "This allows you to specify the payload type of the audio stream " \
> +    "for RTP streaming." )
> +#define PTYPE_VIDEO_TEXT N_("Video payload type")
> +#define PTYPE_VIDEO_LONGTEXT N_( \
> +    "This allows you to specify the payload type of the video stream " \
> +    "for RTP streaming." )
> 
> This is confusing IMHO. This does not select the actual payload type, only the 
> number it is mapped to.

Exactly. The actual payload type is really defined for example in a
transcode{} section preceding the rtp{} section, unless the source is
already available with the right codecs.

I've renamed the parameters to ptype-id-video and ptype-id-audio instead
of ptype-video and ptype-audio, respectively. I hope this makes it more
clear.

>  #define TTL_TEXT N_("Hop limit (TTL)")
>  #define TTL_LONGTEXT N_( \
>      "This is the hop limit (also known as \"Time-To-Live\" or TTL) of " \
> @@ -223,6 +232,10 @@ vlc_module_begin ()
>                   PORT_AUDIO_LONGTEXT, true )
>      add_integer( SOUT_CFG_PREFIX "port-video", 0, PORT_VIDEO_TEXT,
>                   PORT_VIDEO_LONGTEXT, true )
> +    add_integer( SOUT_CFG_PREFIX "ptype-audio", 0, PTYPE_AUDIO_TEXT,
> +                 PTYPE_AUDIO_LONGTEXT, true )
> +    add_integer( SOUT_CFG_PREFIX "ptype-video", 0, PTYPE_VIDEO_TEXT,
> +                 PTYPE_VIDEO_LONGTEXT, true )
> 
> The range 0-127 should be specified, and the default values too, probably 96 
> and 97.


Ok. Didn't know about the add_integer_with_range() macro, adapted the
patch accordingly (my first encounter with the VLC code, so any critics
are welcome). Setting a default value within the valid range is
intentionally avoided, as I really want to leave the ptype ids VLC
chooses in place if the ids are not specified explicitly by the user.
Note that you can specify any ptype id you want, and if you do so, it
will override also the non-dynamic ids with whatever value you give it
to give all flexibility.

Another option would be to only allow values in the range 96-127 and
ignore the user-provided setting with a warning if the original payload
is smaller than 96. But then again, if you're already going to change
the default ptype id, you should probably know what you're doing
anyways, and might have a good reason to change a static payload type id.

> +    uint16_t  i_ptype_audio;
> +    uint16_t  i_ptype_video;
> 
> uint8_t

I changed it to int8_t in order to use a default value of -1 in case the
value is not provided by the user. Is there another option (macro?) to
check whether or not the parameter has been provided by the user? It
works by setting a default value of -1 and a range of 0-127, not sure if
there is a better way though.

> +        if( p_fmt->i_cat == AUDIO_ES && p_sys->i_ptype_audio > 0 )
> +            id->rtp_fmt.payload_type = p_sys->i_ptype_audio;
> +        else if( p_fmt->i_cat == VIDEO_ES && p_sys->i_ptype_video > 0 )
> +            id->rtp_fmt.payload_type = p_sys->i_ptype_video;
> 
> No, zero is a legal value here.

Right. I changed the default value to -1 and adapted the check accordingly.

Thanks again for the feedback,
Andreas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vlc-ptype2.patch
Type: text/x-patch
Size: 3349 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20140413/648b4471/attachment.bin>