[FFmpeg-devel] [PATCH] RTP local udp port issue fix (ticket 916)
Dmitry Volyntsev
xeioexception at gmail.com
Tue Jan 17 22:10:31 CET 2012
Could you, please, clarify, should I separate the patch to 1)
patch1:fix of the issue without modification of RTSP_RTP_PORT_MAX and
if(RTSP_RTP_PORT_MIN != 0) thing + patch2: RTSP_RTP_PORT_MAX
modification and if(RTSP_RTP_PORT_MIN != 0) thing OR 2) patch1: fix of
the issue with modification of RTSP_RTP_PORT_MAX +
patch2:if(RTSP_RTP_PORT_MIN != 0) thing ?
Thanks in advance!
2012/1/17 Luca Abeni <lucabe72 at email.it>:
> On 17/01/12 16:03, Dmitry Volyntsev wrote:
>>
>> I'm sorry for bad previous patch quality.
>>
>> Here is in attach fixed one (previous patch with fixes against
>> tools/patcheck and tools/clean-diff)
>
> I think it should be 2 separate patches (one fixing the issue you are
> seeing, and the other one fixing the "if (RTSP_RTP_PORT_MIN != 0) {" thing),
> but the modifications look ok to me (assuming that you tested it and that
> the random number generation is ok).
>
>
> Luca
>
>>
>> 2012/1/17 Dmitry Volyntsev<xeioexception at gmail.com>:
>>>
>>> Here is new patch according Luca's proposals
>>>
>>> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
>>> index d32f49e..ecc0e80 100644
>>> --- a/libavformat/rtsp.c
>>> +++ b/libavformat/rtsp.c
>>> @@ -1103,7 +1103,7 @@ int ff_rtsp_make_setup_request(AVFormatContext
>>> *s, const char *host, int port,
>>> int lower_transport, const char
>>> *real_challenge)
>>> {
>>> RTSPState *rt = s->priv_data;
>>> - int rtx = 0, j, i, err, interleave = 0;
>>> + int rtx = 0, j, i, err, interleave = 0, port_off;
>>> RTSPStream *rtsp_st;
>>> RTSPMessageHeader reply1, *reply =&reply1;
>>> char cmd[2048];
>>> @@ -1120,8 +1120,9 @@ int ff_rtsp_make_setup_request(AVFormatContext
>>> *s, const char *host, int port,
>>> /* for each stream, make the setup request */
>>> /* XXX: we assume the same server is used for the control of each
>>> * RTSP stream */
>>> -
>>> - for (j = RTSP_RTP_PORT_MIN, i = 0; i< rt->nb_rtsp_streams; ++i) {
>>> + port_off = av_get_random_seed()%(RTSP_RTP_PORT_MAX -
>>> RTSP_RTP_PORT_MIN);
>>> + port_off-= port_off& 0x01; /* even random offset */
>>>
>>> + for (j = RTSP_RTP_PORT_MIN + port_off, i = 0; i<
>>> rt->nb_rtsp_streams; ++i) {
>>> char transport[2048];
>>>
>>> /*
>>> @@ -1158,16 +1159,15 @@ int ff_rtsp_make_setup_request(AVFormatContext
>>> *s, const char *host, int port,
>>> }
>>>
>>> /* first try in specified port range */
>>> - if (RTSP_RTP_PORT_MIN != 0) {
>>> - while (j<= RTSP_RTP_PORT_MAX) {
>>> - ff_url_join(buf, sizeof(buf), "rtp", NULL, host, -1,
>>> - "?localport=%d", j);
>>> - /* we will use two ports per rtp stream (rtp and
>>> rtcp) */
>>> - j += 2;
>>> - if (ffurl_open(&rtsp_st->rtp_handle, buf,
>>> AVIO_FLAG_READ_WRITE,
>>> -&s->interrupt_callback, NULL) == 0)
>>>
>>> - goto rtp_opened;
>>> - }
>>> + while (j<= RTSP_RTP_PORT_MAX) {
>>> + ff_url_join(buf, sizeof(buf), "rtp", NULL, host, -1,
>>> + "?localport=%d", j);
>>> + /* we will use two ports per rtp stream (rtp and rtcp) */
>>> + j += 2;
>>> + if (ffurl_open(&rtsp_st->rtp_handle, buf,
>>> + AVIO_FLAG_READ_WRITE,
>>> +&s->interrupt_callback, NULL) == 0)
>>>
>>> + goto rtp_opened;
>>> }
>>>
>>> av_log(s, AV_LOG_ERROR, "Unable to open an input RTP
>>> port\n");
>>> diff --git a/libavformat/rtsp.h b/libavformat/rtsp.h
>>> index 36de7e5..e11dec9 100644
>>> --- a/libavformat/rtsp.h
>>> +++ b/libavformat/rtsp.h
>>> @@ -70,7 +70,7 @@ enum RTSPControlTransport {
>>> #define RTSP_DEFAULT_NB_AUDIO_CHANNELS 1
>>> #define RTSP_DEFAULT_AUDIO_SAMPLERATE 44100
>>> #define RTSP_RTP_PORT_MIN 5000
>>> -#define RTSP_RTP_PORT_MAX 10000
>>> +#define RTSP_RTP_PORT_MAX 65000
>>>
>>> /**
>>> * This describes a single item in the "Transport:" line of one stream as
>>>
>>>
>>> 2012/1/17 Luca Abeni<lucabe72 at email.it>:
>>>>
>>>> On 01/17/2012 09:02 AM, Dmitry Volyntsev wrote:
>>>> [...]
>>>>>
>>>>>
>>>>> 2012/1/17 Luca Abeni<lucabe72 at email.it>:
>>>>>
>>>>>> I have currently no time to look at all the details... But there is
>>>>>> something
>>>>>> wrong in what's happening: when the second ffmpeg instance tries to
>>>>>> use
>>>>>> port
>>>>>> 5000 for the RTP port, ffurl_open() should fail. And so port 5002 is
>>>>>> tried
>>>>>> (thanks to the "while (j<= RTSP_RTP_PORT_MAX) {" loop) and so on...
>>>>>> Hence, there should be no collision.
>>>>>>
>>>>>> Maybe libavformat ends up using SO_REUSEADDR for some reason? I'd
>>>>>> check
>>>>>> that...
>>>>>>
>>>>>> I think the proposed patch might be wrong, because it might end up in
>>>>>> using
>>>>>> odd
>>>>>> numbers for the RTP port (I think RTP ports should be even numbers,
>>>>>> and
>>>>>> RTCP
>>>>>> ports should be odd).
>>>>>>
>>>>>>
>>>>>> Luca
>>>>>> _______________________________________________
>>>>>> ffmpeg-devel mailing list
>>>>>> ffmpeg-devel at ffmpeg.org
>>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> when the second ffmpeg instance tries to use port
>>>>>
>>>>>
>>>>> 5000 for the RTP port, ffurl_open() should fail. And so port 5002 is
>>>>> tried
>>>>> It is true, but only if the seconds instance starts while first
>>>>> instance still active. But the issue little-bit differs.
>>>>>
>>>>> If fact, the second instance starts when first instance already
>>>>> stopped (killed or something), so it closes opened udp ports. But (it
>>>>> is important for the issue), remote side still sends data to rtp/rtcp
>>>>> ports.
>>>>
>>>>
>>>>
>>>> Ah, sorry... I missed this detail.
>>>>
>>>>
>>>>
>>>>>> I think the proposed patch might be wrong, because it might end up in
>>>>>> using
>>>>>> odd numbers for the RTP port (I think RTP ports should be even
>>>>>> numbers,
>>>>>> and RTCP
>>>>>> ports should be odd).
>>>>>
>>>>>
>>>>> Yes, you are right. I'm going consider this requirement in the next
>>>>> patch.
>>>>
>>>>
>>>>
>>>> I suspect the simplest thing to do to avoid this issue is to add a
>>>> random
>>>> offset
>>>> to RTSP_RTP_PORT_MIN: instead of
>>>> for (j = RTSP_RTP_PORT_MIN, i = 0; i< rt->nb_rtsp_streams; ++i)
>>>> {
>>>> you can use
>>>> offset =<random number>
>>>> for (j = RTSP_RTP_PORT_MIN + offset, i = 0; i<
>>>> rt->nb_rtsp_streams;
>>>> ++i) {
>>>>
>>>>
>>>> BTW, unrelated to this issue, but... I think there is something quite
>>>> wrong
>>>> in this code: things like
>>>> if (RTSP_RTP_PORT_MIN != 0) {
>>>> look _very_ strange...
>>>> and there is an
>>>> extern int rtsp_rtp_port_min;
>>>> in rtsp.h... But there is no "rtsp_rtp_port_min" variable anywhere.
>>>>
>>>>
>>>>
>>>> Luca
>>>
>>>
>>
>
--
Be happy,
Best regards,
Dmitry Volyntsev
More information about the ffmpeg-devel
mailing list