[FFmpeg-devel] [PATCH] rtsp: proper implementation of RFC4570

Michael Niedermayer michaelni at gmx.at
Sun Jul 28 17:02:48 CEST 2013


On Fri, Jul 26, 2013 at 11:38:00AM +0100, Ed Torbett wrote:
> RFC4570 introduces the source-filter attribute to SDP files.
> This patch fully implements this SDP file attribute according
> to the syntax defined within the RFC.
> 
> Signed-off-by: Edward Torbett <ed.torbett at simulation-systems.co.uk>
> ---
>  libavformat/rtpproto.c | 135 ++++++++++++++++++++++++++++++++++++++-----------
>  libavformat/rtsp.c     | 122 ++++++++++++++++++++++++++++++++++++++++----
>  libavformat/rtsp.h     |   9 +++-
>  libavformat/udp.c      |  71 ++++++++++++++++----------
>  4 files changed, 270 insertions(+), 67 deletions(-)
> 
> diff --git a/libavformat/rtpproto.c b/libavformat/rtpproto.c
> index 33affc3..53ad6d0 100644
> --- a/libavformat/rtpproto.c
> +++ b/libavformat/rtpproto.c
> @@ -42,8 +42,8 @@
>  
>  typedef struct RTPContext {
>      URLContext *rtp_hd, *rtcp_hd;
> -    int rtp_fd, rtcp_fd, ssm;
> -    struct sockaddr_storage ssm_addr;
> +    int rtp_fd, rtcp_fd, nb_ssm_include_addrs, nb_ssm_exclude_addrs;
> +    struct sockaddr_storage **ssm_include_addrs, **ssm_exclude_addrs;
>  } RTPContext;
>  
>  /**
> @@ -95,13 +95,13 @@ static struct addrinfo* rtp_resolve_host(const char *hostname, int port,
>      return res;
>  }
>  
> -static int compare_addr(const struct sockaddr_storage *a,
> +static int rtp_compare_addr(const struct sockaddr_storage *a,
>                          const struct sockaddr_storage *b)

i suggest to choose a function name from which the meaning of 1 and
0 is clear, like something with "equal/match" in it

then code like if (address_matches(a,b)) make immedeate sense and
the reader doesnt need to check the function implementation

also such change could be in a seperate patch if you want


>  {
>      if (a->ss_family != b->ss_family)
> -        return 1;
> +        return 0;
>      if (a->ss_family == AF_INET) {
> -        return (((const struct sockaddr_in *)a)->sin_addr.s_addr !=
> +        return (((const struct sockaddr_in *)a)->sin_addr.s_addr ==
>                  ((const struct sockaddr_in *)b)->sin_addr.s_addr);
>      }
>  
> @@ -109,10 +109,30 @@ static int compare_addr(const struct sockaddr_storage *a,
>      if (a->ss_family == AF_INET6) {
>          const uint8_t *s6_addr_a = ((const struct sockaddr_in6 *)a)->sin6_addr.s6_addr;
>          const uint8_t *s6_addr_b = ((const struct sockaddr_in6 *)b)->sin6_addr.s6_addr;
> -        return memcmp(s6_addr_a, s6_addr_b, 16);
> +        return (0 == memcmp(s6_addr_a, s6_addr_b, 16));
>      }
>  #endif
> -    return 1;
> +    return 0;
> +}
> +

> +static int rtp_check_source_lists(RTPContext *s, struct sockaddr_storage *source_addr_ptr) {
> +    int i;
> +    if (s->nb_ssm_exclude_addrs) {
> +        for (i = 0; i < s->nb_ssm_exclude_addrs; i++) {
> +            if (rtp_compare_addr(source_addr_ptr, s->ssm_exclude_addrs[i]))
> +                return 1;
> +        }
> +    }
> +    if (s->nb_ssm_include_addrs) {
> +        int no_match = 1;
> +        for (i = 0; no_match && i < s->nb_ssm_include_addrs; i++) {
> +            if (rtp_compare_addr(source_addr_ptr, s->ssm_include_addrs[i]))

> +                no_match = 0;

this could be replaced by a return 0


> +        }
> +        if (no_match)
> +            return 1;
> +    }
> +    return 0;
>  }




>  
>  /**
> @@ -139,7 +159,8 @@ static void build_udp_url(char *buf, int buf_size,
>                            const char *hostname, int port,
>                            int local_port, int ttl,
>                            int max_packet_size, int connect,
> -                          const char* sources)
> +                          const char* include_sources,
> +                          const char* exclude_sources)
>  {
>      ff_url_join(buf, buf_size, "udp", NULL, hostname, port, NULL);
>      if (local_port >= 0)
> @@ -151,8 +172,10 @@ static void build_udp_url(char *buf, int buf_size,
>      if (connect)
>          url_add_option(buf, buf_size, "connect=1");
>      url_add_option(buf, buf_size, "fifo_size=0");
> -    if (sources && sources[0])
> -        url_add_option(buf, buf_size, "sources=%s", sources);
> +    if (include_sources && include_sources[0])
> +        url_add_option(buf, buf_size, "sources=%s", include_sources);
> +    if (exclude_sources && exclude_sources[0])
> +        url_add_option(buf, buf_size, "block=%s", exclude_sources);
>  }
>  
>  /**

> @@ -173,17 +196,56 @@ static void build_udp_url(char *buf, int buf_size,
>   * if the local rtcp port is not set it will be the local rtp port + 1
>   */
>  
> +static void rtp_parse_addr_list(char *buf, struct sockaddr_storage ***address_list_ptr, int *address_list_size_ptr) {
> +    struct addrinfo *sourceaddr = NULL;
> +    struct sockaddr_storage *source_addr;
> +    char tmp = '\0', *p = buf, *next;
> +
> +    /* Resolve all of the IPs */
> +            
> +    while (p && p[0]) {
> +        next = strchr(p, ',');
> +
> +        if (next) {
> +            tmp = *next;
> +            *next = '\0';
> +        }
> +
> +        source_addr = av_mallocz(sizeof(struct sockaddr_storage));
> +        if (!source_addr)
> +            break;
> +        
> +        sourceaddr = rtp_resolve_host(p, 0,
> +                        SOCK_DGRAM, AF_UNSPEC,
> +                        0);
> +                
> +        memcpy(source_addr, sourceaddr->ai_addr, sourceaddr->ai_addrlen);
> +        freeaddrinfo(sourceaddr);
> +
> +        dynarray_add(address_list_ptr, address_list_size_ptr, source_addr);
> +   
> +        if (next) {
> +            *next = tmp;
> +            p = next + 1;
> +        } else {
> +            p = 0;
> +        }
> +    }
> +}

maybe this can be simpified with av_strtok()


> +
>  static int rtp_open(URLContext *h, const char *uri, int flags)
>  {
>      RTPContext *s = h->priv_data;
>      int rtp_port, rtcp_port,
>          ttl, connect,
>          local_rtp_port, local_rtcp_port, max_packet_size;
> -    char hostname[256], sources[1024] = "";
> +    char hostname[256], include_sources[1024] = "", exclude_sources[1024] = "";
>      char buf[1024];
>      char path[1024];
>      const char *p;
>  

> +    av_log(h, AV_LOG_DEBUG, "Opening URL: %s\n", uri);
> +
>      av_url_split(NULL, 0, NULL, 0, hostname, sizeof(hostname), &rtp_port,
>                   path, sizeof(path), uri);
>      /* extract parameters */

this looks unrelated to the patch


> @@ -218,26 +280,18 @@ static int rtp_open(URLContext *h, const char *uri, int flags)
>              connect = strtol(buf, NULL, 10);
>          }
>          if (av_find_info_tag(buf, sizeof(buf), "sources", p)) {
> -            struct addrinfo *sourceaddr = NULL;
> -            av_strlcpy(sources, buf, sizeof(sources));
> -
> -            /* Try resolving the IP if only one IP is specified - we don't
> -             * support manually checking more than one IP. */
> -            if (!strchr(sources, ','))
> -                sourceaddr = rtp_resolve_host(sources, 0,
> -                                              SOCK_DGRAM, AF_UNSPEC,
> -                                              AI_NUMERICHOST);
> -            if (sourceaddr) {
> -                s->ssm = 1;
> -                memcpy(&s->ssm_addr, sourceaddr->ai_addr, sourceaddr->ai_addrlen);
> -                freeaddrinfo(sourceaddr);
> -            }
> +            av_strlcpy(include_sources, buf, sizeof(include_sources));
> +            rtp_parse_addr_list(buf, &s->ssm_include_addrs, &s->nb_ssm_include_addrs);
> +        }
> +        if (av_find_info_tag(buf, sizeof(buf), "block", p)) {
> +            av_strlcpy(exclude_sources, buf, sizeof(exclude_sources));
> +            rtp_parse_addr_list(buf, &s->ssm_exclude_addrs, &s->nb_ssm_exclude_addrs);
>          }
>      }
>  
>      build_udp_url(buf, sizeof(buf),
>                    hostname, rtp_port, local_rtp_port, ttl, max_packet_size,
> -                  connect, sources);
> +                  connect, include_sources, exclude_sources);
>      if (ffurl_open(&s->rtp_hd, buf, flags, &h->interrupt_callback, NULL) < 0)
>          goto fail;
>      if (local_rtp_port>=0 && local_rtcp_port<0)
> @@ -245,7 +299,7 @@ static int rtp_open(URLContext *h, const char *uri, int flags)
>  
>      build_udp_url(buf, sizeof(buf),
>                    hostname, rtcp_port, local_rtcp_port, ttl, max_packet_size,
> -                  connect, sources);
> +                  connect, include_sources, exclude_sources);
>      if (ffurl_open(&s->rtcp_hd, buf, flags, &h->interrupt_callback, NULL) < 0)
>          goto fail;
>  
> @@ -291,8 +345,9 @@ static int rtp_read(URLContext *h, uint8_t *buf, int size)
>                          continue;
>                      return AVERROR(EIO);
>                  }
> -                if (s->ssm && compare_addr(&from, &s->ssm_addr))
> +                if (rtp_check_source_lists(s, &from))
>                      continue;
> +
>                  break;
>              }
>              /* then RTP */
> @@ -306,8 +361,9 @@ static int rtp_read(URLContext *h, uint8_t *buf, int size)
>                          continue;
>                      return AVERROR(EIO);
>                  }
> -                if (s->ssm && compare_addr(&from, &s->ssm_addr))
> +                if (rtp_check_source_lists(s, &from))

>                      continue;
> +                
>                  break;
>              }
>          } else if (n < 0) {

i agree it looks better with the empty line but this should be in a
seperate patch



    > @@ -340,6 +396,27 @@ static int rtp_write(URLContext *h, const uint8_t *buf, int size)
>  static int rtp_close(URLContext *h)
>  {
>      RTPContext *s = h->priv_data;
> +    struct sockaddr_storage *source_addr;
> +    int i;

> +    if (s->nb_ssm_include_addrs) {
> +        for (i = 0; i < s->nb_ssm_include_addrs; i++) {
> +            source_addr = s->ssm_include_addrs[i];
> +            if (source_addr)
> +                av_free(source_addr);
> +        }
> +        av_freep(&s->ssm_include_addrs);
> +        s->nb_ssm_include_addrs = 0;
> +    }
> +
> +    if (s->nb_ssm_exclude_addrs) {
> +        for (i = 0; i < s->nb_ssm_exclude_addrs; i++) {
> +            source_addr = s->ssm_exclude_addrs[i];
> +            if (source_addr)
> +                av_free(source_addr);
> +        }
> +        av_freep(&s->ssm_exclude_addrs);
> +        s->nb_ssm_exclude_addrs = 0;
> +    }

the list freeing could be factored out instead of duplicated
null checks before av_free* are unneeded


>  
>      ffurl_close(s->rtp_hd);
>      ffurl_close(s->rtcp_hd);
> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> index af574f1..ddc7d88 100644
> --- a/libavformat/rtsp.c
> +++ b/libavformat/rtsp.c
> @@ -289,6 +289,10 @@ typedef struct SDPParseState {
>      struct sockaddr_storage default_ip;
>      int            default_ttl;
>      int            skip_media;  ///< set if an unknown m= line occurs
> +    int nb_default_include_source_addrs; /**< Number of source-specific multicast include source IP address (from SDP content) */
> +    struct RTSPSource **default_include_source_addrs; /**< Source-specific multicast include source IP address (from SDP content) */
> +    int nb_default_exclude_source_addrs; /**< Number of source-specific multicast exclude source IP address (from SDP content) */
> +    struct RTSPSource **default_exclude_source_addrs; /**< Source-specific multicast exclude source IP address (from SDP content) */
>  } SDPParseState;
>  
>  static void sdp_parse_line(AVFormatContext *s, SDPParseState *s1,
> @@ -301,6 +305,7 @@ static void sdp_parse_line(AVFormatContext *s, SDPParseState *s1,
>      int payload_type, i;
>      AVStream *st;
>      RTSPStream *rtsp_st;
> +    RTSPSource *rtsp_src, *rtsp_src2;
>      struct sockaddr_storage sdp_ip;
>      int ttl;
>  

> @@ -369,6 +374,21 @@ static void sdp_parse_line(AVFormatContext *s, SDPParseState *s1,
>          rtsp_st->sdp_ip = s1->default_ip;
>          rtsp_st->sdp_ttl = s1->default_ttl;
>  
> +        for (i = 0; i < s1->nb_default_include_source_addrs; i++) {
> +            rtsp_src = s1->default_include_source_addrs[i];
> +            if (rtsp_src) {
> +                rtsp_src2 = av_memdup(rtsp_src, sizeof(RTSPSource));
> +                dynarray_add(&rtsp_st->include_source_addrs, &rtsp_st->nb_include_source_addrs, rtsp_src2);
> +            }
> +        }
> +        for (i = 0; i < s1->nb_default_exclude_source_addrs; i++) {
> +            rtsp_src = s1->default_exclude_source_addrs[i];
> +            if (rtsp_src) {
> +                rtsp_src2 = av_memdup(rtsp_src, sizeof(RTSPSource));
> +                dynarray_add(&rtsp_st->exclude_source_addrs, &rtsp_st->nb_exclude_source_addrs, rtsp_src2);
> +            }
> +        }

maybe this duplication could be avoided too and there are a few
similar cases of per list duplication later that are candidates for
simplification too

[...]
> @@ -572,12 +570,29 @@ static int udp_open(URLContext *h, const char *uri, int flags)
>                  char *next = strchr(source_start, ',');
>                  if (next)
>                      *next = '\0';
> -                sources[num_sources] = av_strdup(source_start);
> -                if (!sources[num_sources])
> +                include_sources[num_include_sources] = av_strdup(source_start);
> +                if (!include_sources[num_include_sources])
>                      goto fail;
>                  source_start = next + 1;
> -                num_sources++;
> -                if (num_sources >= FF_ARRAY_ELEMS(sources) || !next)
> +                num_include_sources++;
> +                if (num_include_sources >= FF_ARRAY_ELEMS(include_sources) || !next)
> +                    break;
> +            }

> +         }
> +        if (av_find_info_tag(buf, sizeof(buf), "block", p)) {

something is wrong with the indention level here

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

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130728/4372ff75/attachment.asc>


More information about the ffmpeg-devel mailing list