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

Marton Balint cus at passwd.hu
Sat Dec 8 02:16:51 EET 2018



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


More information about the ffmpeg-devel mailing list