[FFmpeg-devel] [PATCH] Make RTP work with IPv6 enabled v.2

[Michael Niedermayer]

On Sun, Oct 28, 2007 at 10:39:00AM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On 10/28/07, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> >
> > If I split the sendto() into a separate connect() in udp_open() followed
> > by send() in udp_write(), the whole thing works as expected, but I guess
> > that won't go together with apps calling udp_set_remote_url() manually...
> > Best way may be to re-arrange udp.c a bit to first set up the local part
> > in udp_open() and then call connect() in set_remote_url() and send() /
> > recv() instead of sendto() / recvfrom() in udp_read/write(), or is that a
> > bad idea?
> 
> 
> Since I'm not getting any replies, here's a new thread with all the patches
> needed to make it work on a Mac, I hope this catches attention and gets the
> patches in. Please consider committing all of these such that ffserver and
> udp in general works and is worth its LOC again.
> 
> 1 - ffmpeg-bind.patch (from Nicolas), required such that we bind even if no
> port number was supplied, in which case the we let the OS choose one for us.
> Without the patch, such cases will not result in a bind in the ipv6-case,
> which makes that case fail. With the patch, that works correctly.
> 2 - ffmpeg-rtsp-allow_two_instances.patch (from Luca, adapted), required
> such that we always increase the port number that we try to connect to,
> otherwise we may end up binding to the same port for multiple instances of
> clients, which will obviously fail.
> 3 - ffmpeg-udp-connect_and_send.patch - replaces sendto() by connect() and
> send(), which makes it work for MacOSX, where for some reason sendto() fails
> with EINVAL (undocumented) for no obvious reason, where connect() and send()
> with the same parameters works just fine. recvfrom() stays the way it is
> because the man page suggests so.
> 
> With these patches, I am able to stream rtsp using ffserver to clients such
> as ffplay and vlc over udp (tcp always worked already) with CONFIG_IPV6
> enabled. Please apply all three.
> 
> Ronald

> Index: ffmpeg/libavformat/udp.c
> ===================================================================
> --- ffmpeg.orig/libavformat/udp.c	2007-10-14 21:49:42.000000000 -0400
> +++ ffmpeg/libavformat/udp.c	2007-10-14 22:02:54.000000000 -0400
> @@ -111,7 +111,7 @@
>      struct addrinfo hints, *res = 0;
>      int error;
>      char sport[16];
> -    const char *node = 0, *service = 0;
> +    const char *node = 0, *service = "0";
>  
>      if (port > 0) {
>          snprintf(sport, sizeof(sport), "%d", port);
> @@ -120,14 +120,12 @@
>      if ((hostname) && (hostname[0] != '\0') && (hostname[0] != '?')) {
>          node = hostname;
>      }
> -    if ((node) || (service)) {
> -        memset(&hints, 0, sizeof(hints));
> -        hints.ai_socktype = type;
> -        hints.ai_family   = family;
> -        hints.ai_flags = flags;
> -        if ((error = getaddrinfo(node, service, &hints, &res))) {
> -            av_log(NULL, AV_LOG_ERROR, "udp_ipv6_resolve_host: %s\n", gai_strerror(error));
> -        }
> +    memset(&hints, 0, sizeof(hints));
> +    hints.ai_socktype = type;
> +    hints.ai_family   = family;
> +    hints.ai_flags = flags;
> +    if ((error = getaddrinfo(node, service, &hints, &res))) {
> +	av_log(NULL, AV_LOG_ERROR, "udp_ipv6_resolve_host: %s\n", gai_strerror(error));

tabs are forbidden in svn, also the reindentions do not belong in a
patch together with functional changes


[...]

> Index: ffmpeg/libavformat/rtsp.c

iam not rtsp maintainer :)

[...]

> @@ -303,15 +314,6 @@
>      /* fill the dest addr */
>      url_split(NULL, 0, NULL, 0, hostname, sizeof(hostname), &port, NULL, 0, uri);
>  
> -    /* XXX: fix url_split */
> -    if (hostname[0] == '\0' || hostname[0] == '?') {
> -        /* only accepts null hostname if input */
> -        if (s->is_multicast || (flags & URL_WRONLY))
> -            goto fail;
> -    } else {
> -        udp_set_remote_url(h, uri);
> -    }
> -
>      if(!ff_network_init())
>          return AVERROR(EIO);
>  
> @@ -380,6 +382,16 @@
>      }
>  #endif /* CONFIG_IPV6 */
>  
> +    /* XXX: fix url_split */
> +    s->udp_fd = udp_fd;
> +    if (hostname[0] == '\0' || hostname[0] == '?') {
> +        /* only accepts null hostname if input */
> +        if (s->is_multicast || (flags & URL_WRONLY))
> +            goto fail;
> +    } else {
> +        udp_set_remote_url(h, uri);
> +    }
> +
>      if (is_output) {
>          /* limit the tx buf size to limit latency */
>          tmp = UDP_TX_BUF_SIZE;
> @@ -394,7 +406,6 @@
>          setsockopt(udp_fd, SOL_SOCKET, SO_RCVBUF, &tmp, sizeof(tmp));
>      }
>  
> -    s->udp_fd = udp_fd;
>      return 0;
>   fail:
>      if (udp_fd >= 0)

this looks independant of the rest of the patch, can it be split?
note, i dont know the code at all, iam just reviewing it as it doesnt seem
anyone else is volunteering ...
also the more verbose the descriptions of the patches are the more likely
they will be accepted, things like X doesnt work Y does work are not good
explanations
something, like: 'see page 15 of "portable networking for idiots" this
explains why what udp.c does is wrong and why my patch is correct' 
or similar would be much better

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071028/7e539612/attachment.pgp>