[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