[issue2212] rtsp punch packets sent to wrong IP

2010-09-03 Thread John Wimer 

New submission from John Wimer j...@god.vtic.net:

The rtsp code includes support for punch packets to open some NATs 
when starting the rtp streaming. However, it ignores the source 
attribute in the Transport header [RFC 2326 section 12.39].

The attached patch sends the punch packets to the ip address specified 
in the Transport line, falling back the rtsp server's ip if no source is 
specified.

--
files: 0001-Send-punch-packets-to-the-RTP-source-address-if-it-i.patch
messages: 11841
priority: normal
status: new
substatus: new
title: rtsp punch packets sent to wrong IP
topic: rtsp
type: patch


FFmpeg issue tracker iss...@roundup.ffmpeg.org
https://roundup.ffmpeg.org/issue2212
From 7776d24d5251f86cc4988b2d0b177a70d9fde4a8 Mon Sep 17 00:00:00 2001
From: John Wimer j...@god.vtic.net
Date: Fri, 3 Sep 2010 09:55:04 +0200
Subject: [PATCH] Send punch packets to the RTP source address if it is specified

---
 libavformat/rtsp.c |   19 ---
 libavformat/rtsp.h |4 
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
index c3f4d00..5d87dd3 100644
--- a/libavformat/rtsp.c
+++ b/libavformat/rtsp.c
@@ -701,7 +701,14 @@ static void rtsp_parse_transport(RTSPMessageHeader *reply, const char *p)
 get_word_sep(buf, sizeof(buf), ;,, p);
 get_sockaddr(buf, th-destination);
 }
+} else if (!strcmp(parameter, source)) {
+if (*p == '=') {
+p++;
+get_word_sep(buf, sizeof(buf), ;,, p);
+strncpy(th-source, buf, sizeof(th-source) - 1);
+}
 }
+
 while (*p != ';'  *p != '\0'  *p != ',')
 p++;
 if (*p == ';')
@@ -1154,9 +1161,15 @@ static int make_setup_request(AVFormatContext *s, const char *host, int port,
 case RTSP_LOWER_TRANSPORT_UDP: {
 char url[1024];
 
-/* XXX: also use address if specified */
-ff_url_join(url, sizeof(url), rtp, NULL, host,
-reply-transports[0].server_port_min, NULL);
+/* Use source address if specified */
+if (reply-transports[0].source[0]) {
+ff_url_join(url, sizeof(url), rtp, NULL,
+reply-transports[0].source,
+reply-transports[0].server_port_min, NULL);
+} else {
+ff_url_join(url, sizeof(url), rtp, NULL, host,
+reply-transports[0].server_port_min, NULL);
+}
 if (!(rt-server_type == RTSP_SERVER_WMS  i  1) 
 rtp_set_remote_url(rtsp_st-rtp_handle, url)  0) {
 err = AVERROR_INVALIDDATA;
diff --git a/libavformat/rtsp.h b/libavformat/rtsp.h
index c6c3972..24d5e3b 100644
--- a/libavformat/rtsp.h
+++ b/libavformat/rtsp.h
@@ -66,6 +66,9 @@ enum RTSPControlTransport {
 #define RTSP_RTP_PORT_MIN 5000
 #define RTSP_RTP_PORT_MAX 1
 
+/** Space for text ip address (max of INET_ADDRSTRLEN INET6_ADDRSTRLEN) */
+#define MAX_ADDRSTR_LEN 46
+
 /**
  * This describes a single item in the Transport: line of one stream as
  * negotiated by the SETUP RTSP command. Multiple transports are comma-
@@ -97,6 +100,7 @@ typedef struct RTSPTransportField {
 int ttl;
 
 struct sockaddr_storage destination; /** destination IP address */
+char source[MAX_ADDRSTR_LEN + 1]; /** source IP address */
 
 /** data/packet transport protocol; e.g. RTP or RDT */
 enum RTSPTransport transport;
-- 
1.7.0.4

[issue2212] rtsp punch packets sent to wrong IP

2010-09-03 Thread Martin Storsjö 

Martin Storsjö mar...@martin.st added the comment:

Looks good to me. Luca, Ronald, ok with applying it?

--
nosy: +lu_zero, mstorsjo, rbultje


FFmpeg issue tracker iss...@roundup.ffmpeg.org
https://roundup.ffmpeg.org/issue2212

[issue2212] rtsp punch packets sent to wrong IP

2010-09-03 Thread Ronald S. Bultje 

Ronald S. Bultje rsbul...@gmail.com added the comment:

+} else if (!strcmp(parameter, source)) {
+if (*p == '=') {

(This should be fixed, this is hideous, all over that function, but 
that's ok for now.)

-/* XXX: also use address if specified */
-ff_url_join(url, sizeof(url), rtp, NULL, host,
-reply-transports[0].server_port_min, NULL);
+/* Use source address if specified */
+if (reply-transports[0].source[0]) {
+ff_url_join(url, sizeof(url), rtp, NULL,
+reply-transports[0].source,
+reply-transports[0].server_port_min, 
NULL);
+} else {
+ff_url_join(url, sizeof(url), rtp, NULL, host,
+reply-transports[0].server_port_min, 
NULL);
+}

Please separate the reindenting from the function change while 
committing.

+/** Space for text ip address (max of INET_ADDRSTRLEN INET6_ADDRSTRLEN) 
*/
+#define MAX_ADDRSTR_LEN 46

Please use INET6_ADDRSTRLEN. If undefined, in os_support.h, set it to 
INET_ADDRSTRLEN. If undefined, set it to some random number, e.g. 46. 
Don't hardcode this stuff unnecessarily.

General idea sounds good to me.


FFmpeg issue tracker iss...@roundup.ffmpeg.org
https://roundup.ffmpeg.org/issue2212

[issue2212] rtsp punch packets sent to wrong IP

2010-09-03 Thread Martin Storsjö 

Martin Storsjö mar...@martin.st added the comment:

 -/* XXX: also use address if specified */
 -ff_url_join(url, sizeof(url), rtp, NULL, host,
 -reply-transports[0].server_port_min, NULL);
 +/* Use source address if specified */
 +if (reply-transports[0].source[0]) {
 +ff_url_join(url, sizeof(url), rtp, NULL,
 +reply-transports[0].source,
 +reply-transports[0].server_port_min, NULL);
 +} else {
 +ff_url_join(url, sizeof(url), rtp, NULL, host,
 +reply-transports[0].server_port_min, NULL);
 +}
 
 Please separate the reindenting from the function change while 
 committing.

Ah, yes, I missed that.

 +/** Space for text ip address (max of INET_ADDRSTRLEN INET6_ADDRSTRLEN) */
+#define MAX_ADDRSTR_LEN 46
 
 Please use INET6_ADDRSTRLEN. If undefined, in os_support.h, set it to 
 INET_ADDRSTRLEN. If undefined, set it to some random number, e.g. 46. 
 Don't hardcode this stuff unnecessarily.

In his defense, he asked about this, and I suggested this solution, since it is 
independent of both v4 
and v6, not littering general code with references to whichever address family 
has the largest 
addresses at the moment. But if you prefer that we use INET6_ADDRSTRLEN 
straight away, I'm ok with 
that too.


FFmpeg issue tracker iss...@roundup.ffmpeg.org
https://roundup.ffmpeg.org/issue2212

[issue2212] rtsp punch packets sent to wrong IP

2010-09-03 Thread Ronald S. Bultje 

Ronald S. Bultje rsbul...@gmail.com added the comment:

I think we'd like to use the IPv6 API where-ever possible, including here. 
If it's unavailable, os_support should provide a fallback for the IPv4 
API. If that's unavailable, it should not be compiled in the first place 
because the whole networking stack will be disabled (--disable=network).


FFmpeg issue tracker iss...@roundup.ffmpeg.org
https://roundup.ffmpeg.org/issue2212

[issue2212] rtsp punch packets sent to wrong IP

2010-09-03 Thread Luca Barbato 

Luca Barbato lu_z...@gentoo.org added the comment:

On 09/03/2010 02:29 PM, Martin Storsjö wrote:
 In his defense, he asked about this, and I suggested this solution, since it 
 is independent of both v4 
 and v6, not littering general code with references to whichever address 
 family has the largest 
 addresses at the moment. But if you prefer that we use INET6_ADDRSTRLEN 
 straight away, I'm ok with 
 that too.

I have no strong opinion regarding the macro value, INET6_ADDRSTRLEN
seems fine if is always defined.

The rest of the patch looks ok

lu

-- 

Luca Barbato
Gentoo/linux
http://dev.gentoo.org/~lu_zero


FFmpeg issue tracker iss...@roundup.ffmpeg.org
https://roundup.ffmpeg.org/issue2212

[issue2212] rtsp punch packets sent to wrong IP

2010-09-03 Thread John Wimer 

John Wimer j...@god.vtic.net added the comment:

updated patch to use INET6_ADDRSTRLEN. A second whitespace-changes-only 
patch to follow.


FFmpeg issue tracker iss...@roundup.ffmpeg.org
https://roundup.ffmpeg.org/issue2212
From 8f9ea392e2f1def2fe4f74bac6a2ddc47946b706 Mon Sep 17 00:00:00 2001
From: John Wimer j...@god.vtic.net
Date: Fri, 3 Sep 2010 18:37:13 +0200
Subject: [PATCH 1/2] Send punch packets to the RTP source address if it is specified in the Transport header

---
 libavformat/os_support.h |6 ++
 libavformat/rtsp.c   |   15 ++-
 libavformat/rtsp.h   |1 +
 3 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/libavformat/os_support.h b/libavformat/os_support.h
index 5c9e81b..3207123 100644
--- a/libavformat/os_support.h
+++ b/libavformat/os_support.h
@@ -55,6 +55,12 @@ typedef int socklen_t;
 #define closesocket close
 #endif
 
+#ifndef INET6_ADDRSTRLEN
+#  if defined(INET_ADDRSTRLEN)
+#define INET6_ADDRSTRLEN INET_ADDRSTRLEN
+#  endif
+#endif
+
 #if CONFIG_FFSERVER
 #if !HAVE_POLL_H
 typedef unsigned long nfds_t;
diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
index be3eee6..4ab2c33 100644
--- a/libavformat/rtsp.c
+++ b/libavformat/rtsp.c
@@ -701,7 +701,14 @@ static void rtsp_parse_transport(RTSPMessageHeader *reply, const char *p)
 get_word_sep(buf, sizeof(buf), ;,, p);
 get_sockaddr(buf, th-destination);
 }
+} else if (!strcmp(parameter, source)) {
+if (*p == '=') {
+p++;
+get_word_sep(buf, sizeof(buf), ;,, p);
+av_strlcpy(th-source, buf, sizeof(th-source));
+}
 }
+
 while (*p != ';'  *p != '\0'  *p != ',')
 p++;
 if (*p == ';')
@@ -1154,9 +1161,15 @@ static int make_setup_request(AVFormatContext *s, const char *host, int port,
 case RTSP_LOWER_TRANSPORT_UDP: {
 char url[1024];
 
-/* XXX: also use address if specified */
+/* Use source address if specified */
+if (reply-transports[0].source[0]) {
+ff_url_join(url, sizeof(url), rtp, NULL,
+reply-transports[0].source,
+reply-transports[0].server_port_min, NULL);
+} else {
 ff_url_join(url, sizeof(url), rtp, NULL, host,
 reply-transports[0].server_port_min, NULL);
+}
 if (!(rt-server_type == RTSP_SERVER_WMS  i  1) 
 rtp_set_remote_url(rtsp_st-rtp_handle, url)  0) {
 err = AVERROR_INVALIDDATA;
diff --git a/libavformat/rtsp.h b/libavformat/rtsp.h
index c6c3972..c577618 100644
--- a/libavformat/rtsp.h
+++ b/libavformat/rtsp.h
@@ -97,6 +97,7 @@ typedef struct RTSPTransportField {
 int ttl;
 
 struct sockaddr_storage destination; /** destination IP address */
+char source[INET6_ADDRSTRLEN + 1]; /** source IP address */
 
 /** data/packet transport protocol; e.g. RTP or RDT */
 enum RTSPTransport transport;
-- 
1.7.0.4

[issue2212] rtsp punch packets sent to wrong IP

2010-09-03 Thread John Wimer 

John Wimer j...@god.vtic.net added the comment:

whitespace cleanup


FFmpeg issue tracker iss...@roundup.ffmpeg.org
https://roundup.ffmpeg.org/issue2212
From 3b9d391d562b9d8c835837db40b0664910372433 Mon Sep 17 00:00:00 2001
From: John Wimer j...@god.vtic.net
Date: Fri, 3 Sep 2010 18:39:57 +0200
Subject: [PATCH 2/2] re-indent

---
 libavformat/rtsp.c |6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
index 4ab2c33..1be5a39 100644
--- a/libavformat/rtsp.c
+++ b/libavformat/rtsp.c
@@ -1167,9 +1167,9 @@ static int make_setup_request(AVFormatContext *s, const char *host, int port,
 reply-transports[0].source,
 reply-transports[0].server_port_min, NULL);
 } else {
-ff_url_join(url, sizeof(url), rtp, NULL, host,
-reply-transports[0].server_port_min, NULL);
-}
+ff_url_join(url, sizeof(url), rtp, NULL, host,
+reply-transports[0].server_port_min, NULL);
+}
 if (!(rt-server_type == RTSP_SERVER_WMS  i  1) 
 rtp_set_remote_url(rtsp_st-rtp_handle, url)  0) {
 err = AVERROR_INVALIDDATA;
-- 
1.7.0.4

[issue2212] rtsp punch packets sent to wrong IP

2010-09-03 Thread Ronald S. Bultje 

Ronald S. Bultje rsbul...@gmail.com added the comment:

+#ifndef INET6_ADDRSTRLEN
+#  if defined(INET_ADDRSTRLEN)
+#define INET6_ADDRSTRLEN INET_ADDRSTRLEN
+#  endif
+#endif

The inner if/endif aren't necessary (I was wrong there earlier). 
Otherwise OK.


FFmpeg issue tracker iss...@roundup.ffmpeg.org
https://roundup.ffmpeg.org/issue2212

[issue2212] rtsp punch packets sent to wrong IP

2010-09-03 Thread John Wimer 

John Wimer j...@god.vtic.net added the comment:

new version of 0001 patch, where it doesn't bother checking for 
INET_ADDRSTRLEN.


FFmpeg issue tracker iss...@roundup.ffmpeg.org
https://roundup.ffmpeg.org/issue2212
From 583ff7440e7a62d0623f25de4e9b9499f5a5ed9a Mon Sep 17 00:00:00 2001
From: John Wimer j...@god.vtic.net
Date: Fri, 3 Sep 2010 20:49:55 +0200
Subject: [PATCH 1/2] Send punch packets to the RTP source address if it is specified in the Transport header

---
 libavformat/os_support.h |4 
 libavformat/rtsp.c   |   15 ++-
 libavformat/rtsp.h   |1 +
 3 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/libavformat/os_support.h b/libavformat/os_support.h
index 5c9e81b..1330d3d 100644
--- a/libavformat/os_support.h
+++ b/libavformat/os_support.h
@@ -55,6 +55,10 @@ typedef int socklen_t;
 #define closesocket close
 #endif
 
+#ifndef INET6_ADDRSTRLEN
+#define INET6_ADDRSTRLEN INET_ADDRSTRLEN
+#endif
+
 #if CONFIG_FFSERVER
 #if !HAVE_POLL_H
 typedef unsigned long nfds_t;
diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
index be3eee6..4ab2c33 100644
--- a/libavformat/rtsp.c
+++ b/libavformat/rtsp.c
@@ -701,7 +701,14 @@ static void rtsp_parse_transport(RTSPMessageHeader *reply, const char *p)
 get_word_sep(buf, sizeof(buf), ;,, p);
 get_sockaddr(buf, th-destination);
 }
+} else if (!strcmp(parameter, source)) {
+if (*p == '=') {
+p++;
+get_word_sep(buf, sizeof(buf), ;,, p);
+av_strlcpy(th-source, buf, sizeof(th-source));
+}
 }
+
 while (*p != ';'  *p != '\0'  *p != ',')
 p++;
 if (*p == ';')
@@ -1154,9 +1161,15 @@ static int make_setup_request(AVFormatContext *s, const char *host, int port,
 case RTSP_LOWER_TRANSPORT_UDP: {
 char url[1024];
 
-/* XXX: also use address if specified */
+/* Use source address if specified */
+if (reply-transports[0].source[0]) {
+ff_url_join(url, sizeof(url), rtp, NULL,
+reply-transports[0].source,
+reply-transports[0].server_port_min, NULL);
+} else {
 ff_url_join(url, sizeof(url), rtp, NULL, host,
 reply-transports[0].server_port_min, NULL);
+}
 if (!(rt-server_type == RTSP_SERVER_WMS  i  1) 
 rtp_set_remote_url(rtsp_st-rtp_handle, url)  0) {
 err = AVERROR_INVALIDDATA;
diff --git a/libavformat/rtsp.h b/libavformat/rtsp.h
index c6c3972..c577618 100644
--- a/libavformat/rtsp.h
+++ b/libavformat/rtsp.h
@@ -97,6 +97,7 @@ typedef struct RTSPTransportField {
 int ttl;
 
 struct sockaddr_storage destination; /** destination IP address */
+char source[INET6_ADDRSTRLEN + 1]; /** source IP address */
 
 /** data/packet transport protocol; e.g. RTP or RDT */
 enum RTSPTransport transport;
-- 
1.7.0.4

[issue2212] rtsp punch packets sent to wrong IP

2010-09-03 Thread Luca Barbato 

Luca Barbato lu_z...@gentoo.org added the comment:

On 09/03/2010 08:52 PM, John Wimer wrote:
 
 John Wimer j...@god.vtic.net added the comment:
 
 new version of 0001 patch, where it doesn't bother checking for 
 INET_ADDRSTRLEN.

Looks ok

-- 

Luca Barbato
Gentoo/linux
http://dev.gentoo.org/~lu_zero


FFmpeg issue tracker iss...@roundup.ffmpeg.org
https://roundup.ffmpeg.org/issue2212

[issue2212] rtsp punch packets sent to wrong IP

2010-09-03 Thread Ronald S. Bultje 

Ronald S. Bultje rsbul...@gmail.com added the comment:

OK from me too.


FFmpeg issue tracker iss...@roundup.ffmpeg.org
https://roundup.ffmpeg.org/issue2212

[issue2212] rtsp punch packets sent to wrong IP

2010-09-03 Thread Ronald S. Bultje 

Ronald S. Bultje rsbul...@gmail.com added the comment:

Applied, thanks.

--
status: new - closed
substatus: new - fixed


FFmpeg issue tracker iss...@roundup.ffmpeg.org
https://roundup.ffmpeg.org/issue2212