[FFmpeg-devel] [PATCH 2/2] add socks5 support for tcp clients

[Moritz Barsnick]

On Sat, Jun 13, 2020 at 23:17:41 +0800, levizhao at live.cn wrote:
> From: zhaoyi <levizhao at live.cn>

Is this the name you'd like to appear in the git log, asuming the patch
gets accepted? It cannot be changed afterwards. (I'm saying: Most
people use their "real name" here.)

In your commit message, please also mention "Fixes #5776", as this is
an open ffmpeg ticket:
http://trac.ffmpeg.org/ticket/5776

>  libavformat/tcp.c | 204 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 204 insertions(+)

When adding options, please also kindly update the documentation, in
this case doc/protocols.texi. (We might also want to bump the micro
version of libavformat.)

And make sure to point out there that it's currently only for SOCKS5.

And this may be worth a Changelog entry.

> +    if(use_proxy) {
> +        av_url_split(proto_proxy, sizeof(proto_proxy), NULL, 0, hostname_proxy, sizeof(hostname_proxy),
> +            &port, path_proxy, sizeof(path_proxy), proxy_path);
> +        port = (port > 0 && port < 65536) ? port : 1080;

An out-of-range port should be checked here, or before. You have it ten
lines down.

> +    p = strchr(uri, '?');
> +    if (p) {
> +        if (av_find_info_tag(buf, sizeof(buf), "listen", p)) {
> +            char *endptr = NULL;
> +            s->listen = strtol(buf, &endptr, 10);
> +            /* assume if no digits were found it is a request to enable it */
> +            if (buf == endptr)
> +                s->listen = 1;
> +        }
> +        if (av_find_info_tag(buf, sizeof(buf), "timeout", p)) {
> +            s->rw_timeout = strtol(buf, NULL, 10);
> +        }
> +        if (av_find_info_tag(buf, sizeof(buf), "listen_timeout", p)) {
> +            s->listen_timeout = strtol(buf, NULL, 10);
> +        }
> +    }

Are you duplicating some functionality found elsewhere? I believe the
URI '?' syntax is supported in many places.

Let's not try to duplicate code too much. (I didn't have the time to
double-check which other protocols already use similar code.)


Please also make sure to stick to the ffmpeg code style (e.g.
whitespace between arguments, whitespace in "if (", indentation of
function arguments, and so on).

I can't judge on the rest, hoping others will pitch in.

I'm very willing to test this feature (I need it ;-)), but won't be
able to do so before tomorrow.

Cheers,
Moritz