diff mbox

[FFmpeg-devel] libstr.c: Fixed rendezvous mode

Message ID DB8PR09MB29692763876CD366A90BC048C5AA0@DB8PR09MB2969.eurprd09.prod.outlook.com
State New
Headers show

Commit Message

Adrian Grzeca Dec. 7, 2018, 2:01 p.m. UTC 
---
 libavformat/libsrt.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

Comments

Moritz Barsnick Dec. 7, 2018, 4:48 p.m. UTC | #1 
On Fri, Dec 07, 2018 at 14:01:03 +0000, Adrian Grzeca wrote:

Please read the coding guidelines here:
https://www.ffmpeg.org/developer.html#Coding-Rules-1

Your indentation is incorrect, incl. use of tabs, which is not
supported in the ffmpeg code base. Also, your brackets and whitespace
are incorrect.

Furthermore, your commit message says "Fixed rendezvous mode", when
your change gives the impression that you're adding features:

> +	{ "localip",     "IP address of network card to use in rendezvous mode (Req. in rendezvous mode)",             OFFSET(localip),       AV_OPT_TYPE_STRING,   { .str = NULL },              .flags = D|E },
> +	{ "localport",     "Source port to use in rendezvous mode (Req. in rendezvous mode)",             OFFSET(localport),       AV_OPT_TYPE_STRING,   { .str = NULL },              .flags = D|E },

You need to explain in more detail what the actual issue is (is there a
trac ticket for this?), and how you are fixing it.

Furthermore:
> Subject: libstr.c: Fixed rendezvous mode

An ffmpeg commit message, or at least its header line, would read:
  avformat/libsrt: fix rendezvous mode

> +	{ "localip",     "IP address of network card to use in rendezvous mode (Req. in rendezvous mode)",             OFFSET(localip),       AV_OPT_TYPE_STRING,   { .str = NULL },              .flags = D|E },
                                                                                ^
What does "Req." mean and why is it capitalized?

> +		if(s->localip == NULL || s->localport == NULL) {

ffmpeg's style would be
    if (!s->localip || !s->localport) {

Your code may have further issues which I cannot judge on.

Cheers,
Moritz
Marton Balint Dec. 8, 2018, 12:16 a.m. UTC | #2 
On Fri, 7 Dec 2018, Moritz Barsnick wrote:

> On Fri, Dec 07, 2018 at 14:01:03 +0000, Adrian Grzeca wrote:
>
> Please read the coding guidelines here:
> https://www.ffmpeg.org/developer.html#Coding-Rules-1
>
> Your indentation is incorrect, incl. use of tabs, which is not
> supported in the ffmpeg code base. Also, your brackets and whitespace
> are incorrect.
>
> Furthermore, your commit message says "Fixed rendezvous mode", when
> your change gives the impression that you're adding features:
>
>> +	{ "localip",     "IP address of network card to use in rendezvous mode (Req. in rendezvous mode)",             OFFSET(localip),       AV_OPT_TYPE_STRING,   { .str = NULL },              .flags = D|E },

Please use option name "localaddr" instead because I believe UDP protocol 
has a similar option.

>> +	{ "localport",     "Source port to use in rendezvous mode (Req. in rendezvous mode)",             OFFSET(localport),       AV_OPT_TYPE_STRING,   { .str = NULL },              .flags = D|E },

AV_OPT_TYPE_INT

>
> You need to explain in more detail what the actual issue is (is there a
> trac ticket for this?), and how you are fixing it.
>
> Furthermore:
>> Subject: libstr.c: Fixed rendezvous mode
>
> An ffmpeg commit message, or at least its header line, would read:
>  avformat/libsrt: fix rendezvous mode
>
>> +	{ "localip",     "IP address of network card to use in rendezvous mode (Req. in rendezvous mode)",             OFFSET(localip),       AV_OPT_TYPE_STRING,   { .str = NULL },              .flags = D|E },
>                                                                                ^
> What does "Req." mean and why is it capitalized?

I'd just drop the extra info parenthesis, the text before that makes it 
clear that these options are used in rendezvous mode only.

>
>> +		if(s->localip == NULL || s->localport == NULL) {
>
> ffmpeg's style would be
>    if (!s->localip || !s->localport) {
>
> Your code may have further issues which I cannot judge on.

doc/protocols.texi update is missing.

Thanks,
Marton
diff mbox

Patch

diff --git a/libavformat/libsrt.c b/libavformat/libsrt.c
index fe3b312151..fced18fb92 100644
--- a/libavformat/libsrt.c
+++ b/libavformat/libsrt.c
@@ -84,6 +84,8 @@  typedef struct SRTContext {
     char *smoother;
     int messageapi;
     SRT_TRANSTYPE transtype;
+	char *localip;
+	char *localport;
 } SRTContext;
 
 #define D AV_OPT_FLAG_DECODING_PARAM
@@ -128,6 +130,8 @@  static const AVOption libsrt_options[] = {
     { "transtype",      "The transmission type for the socket",                                 OFFSET(transtype),        AV_OPT_TYPE_INT,      { .i64 = SRTT_INVALID }, SRTT_LIVE, SRTT_INVALID, .flags = D|E, "transtype" },
     { "live",           NULL, 0, AV_OPT_TYPE_CONST,  { .i64 = SRTT_LIVE }, INT_MIN, INT_MAX, .flags = D|E, "transtype" },
     { "file",           NULL, 0, AV_OPT_TYPE_CONST,  { .i64 = SRTT_FILE }, INT_MIN, INT_MAX, .flags = D|E, "transtype" },
+	{ "localip",     "IP address of network card to use in rendezvous mode (Req. in rendezvous mode)",             OFFSET(localip),       AV_OPT_TYPE_STRING,   { .str = NULL },              .flags = D|E },
+	{ "localport",     "Source port to use in rendezvous mode (Req. in rendezvous mode)",             OFFSET(localport),       AV_OPT_TYPE_STRING,   { .str = NULL },              .flags = D|E },
     { NULL }
 };
 
@@ -355,6 +359,7 @@  static int libsrt_setup(URLContext *h, const char *uri, int flags)
     char portstr[10];
     int open_timeout = 5000000;
     int eid;
+	struct sockaddr_in la;
 
     eid = srt_epoll_create();
     if (eid < 0)
@@ -395,7 +400,25 @@  static int libsrt_setup(URLContext *h, const char *uri, int flags)
     }
 
     cur_ai = ai;
-
+	
+	//Fix for rendezvous mode. Not good only for IPv4 but working.
+	if (s->mode == SRT_MODE_RENDEZVOUS) {
+		if(s->localip == NULL || s->localport == NULL) {
+			av_log(h, AV_LOG_ERROR, "Invalid adapter configuration\n");
+			return AVERROR(EIO);
+		}
+		av_log(h, AV_LOG_DEBUG , "Adapter options %s:%s\n", s->localip, s->localport);
+		
+		int lp = strtol(s->localport, NULL, 10); //TODO: What if localport do not contain numers
+		if (lp <= 0 || lp >= 65536) {
+			av_log(h, AV_LOG_ERROR, "Local port missing in uri\n");
+			return AVERROR(EINVAL);
+		}
+		
+		la.sin_family = AF_INET; //TODO: Add support for IPv6
+		la.sin_port = htons(port);
+		la.sin_addr.s_addr = inet_addr(s->localip); //TODO: Check if localip is IP of local interface
+	}
  restart:
 
     fd = srt_socket(cur_ai->ai_family, cur_ai->ai_socktype, 0);
@@ -423,7 +446,7 @@  static int libsrt_setup(URLContext *h, const char *uri, int flags)
         fd = ret;
     } else {
         if (s->mode == SRT_MODE_RENDEZVOUS) {
-            ret = srt_bind(fd, cur_ai->ai_addr, cur_ai->ai_addrlen);
+            ret = srt_bind(fd, (struct sockaddr *)&la, sizeof(struct sockaddr_in));
             if (ret)
                 goto fail1;
         }
@@ -579,6 +602,12 @@  static int libsrt_open(URLContext *h, const char *uri, int flags)
             } else {
                 return AVERROR(EINVAL);
             }
+        }
+		if (av_find_info_tag(buf, sizeof(buf), "localip", p)) {
+            s->localip = av_strndup(buf, strlen(buf));
+        }
+		if (av_find_info_tag(buf, sizeof(buf), "localport", p)) {
+            s->localport = av_strndup(buf, strlen(buf));
         }
     }
     return libsrt_setup(h, uri, flags);