[FFmpeg-devel] [PATCH] Maintain HTTP options across multiple requests in HLS demuxer
Stefano Sabatini
stefasab at gmail.com
Fri Jan 11 12:27:53 CET 2013
On date Wednesday 2013-01-09 20:48:15 -0500, Micah Galizia encoded:
> On Tue, Jan 8, 2013 at 9:20 PM, Micah Galizia <micahgalizia at gmail.com>wrote:
>
> > ./ffplay -user-agent micah "
> > http://nlds119.cdnak.neulion.com/nlds_vod/tsn/vod/2012/12/22/4/1_4_can_swe_2012_h_whole_1_android.mp4.m3u8?ffmpeg=neato
> > "
> >
>
>
> OK, as requested on IRC I have fixed av_free->av_freep and I've documented
> (hopefully sufficiently) the new HTTP option and retested. There are now
> two patches, one for http and one for hls.
>
> Thanks for the review!
> --
> "The mark of an immature man is that he wants to die nobly for a cause,
> while the mark of the mature man is that he wants to live humbly for
> one." --W. Stekel
> diff --git a/libavformat/http.c b/libavformat/http.c
> index 576875f..ab58765 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -64,6 +64,7 @@ typedef struct {
> int is_akamai;
> int rw_timeout;
> char *mime_type;
> + char *cookies; /**< holds newline (\n) delimited Set-Cookie header field values (without the "Set-Cookie: " field name) */
nit: /**< ... */ => ///<
> } HTTPContext;
>
> #define OFFSET(x) offsetof(HTTPContext, x)
> @@ -80,6 +81,7 @@ static const AVOption options[] = {
> {"post_data", "custom HTTP post data", OFFSET(post_data), AV_OPT_TYPE_BINARY, .flags = D|E },
> {"timeout", "timeout of socket i/o operations", OFFSET(rw_timeout), AV_OPT_TYPE_INT, {.i64 = -1}, -1, INT_MAX, D|E },
> {"mime_type", "", OFFSET(mime_type), AV_OPT_TYPE_STRING, {0}, 0, 0, 0 },
> +{"cookies", "newline delimited Set-Cookie HTTP response field values", OFFSET(cookies), AV_OPT_TYPE_STRING, {0}, 0, 0, 0 },
nit: "set newline..."
Also I'm not really sure about what this is about. In the code I see
that the set value is prepended to the value returned by the
server. Maybe you could clarify what's the use case (and possibly
extend the docs in protocols.texi).
> {NULL}
> };
> #define HTTP_CLASS(flavor)\
> @@ -359,11 +361,103 @@ static int process_line(URLContext *h, char *line, int line_count,
> s->is_akamai = 1;
> } else if (!av_strcasecmp (tag, "Content-Type")) {
> av_free(s->mime_type); s->mime_type = av_strdup(p);
> + } else if (!av_strcasecmp (tag, "Set-Cookie")) {
> + if (!s->cookies) {
> + s->cookies = av_strdup(p);
missing check in case of failed allocation
> + } else {
> + char *tmp = s->cookies;
> + size_t str_size = sizeof(char) * (strlen(tmp) + strlen(p) + 2);
nit: sizeof(char) can be omitted
> + if (!(s->cookies = av_malloc(str_size))) {
> + return -1;
return AVERROR(ENOMEM)
Also you can use av_realloc_f in mem.h and thus avoid overflow exceptions.
> + }
> + av_strlcpy(s->cookies, tmp, str_size);
> + av_strlcat(s->cookies, "\n", str_size);
> + av_strlcat(s->cookies, p, str_size);
> + av_free(tmp);
> + }
> }
> }
> return 1;
> }
>
> +static char *get_cookies(HTTPContext *s, const char *path,
> + const char *domain)
I'm possibly paranoid, but maybe you should return an int, like in:
static int get_cookies(HTTPContext *s, char **cookies, const char *path, const char *domain)
or alternatively provide some way to signal an error (I don't know if
simply returning a NULL is enough in this case).
> +{
> + // cookie strings will look like Set-Cookie header field values. Multiple
> + // Set-Cookie fields will result in multiple values delimited by a newline
> + const char *cookies = s->cookies;
> + char *value = NULL;
> +
> + if (!cookies) return NULL;
> + while (*cookies) {
> + int domain_offset = 0;
> + char *cdomain = NULL, *cpath = NULL, *cvalue = NULL;
> + char *cookie = av_get_token(&cookies, "\n"), *c = cookie;
Assuming that no escaping is supported in Cookies headers, I think
that av_get_token() is safer here.
> +
> + while (*cookie) {
> + char *param = av_get_token((const char**)&cookie, "; ");
> +
> + if (!av_strncasecmp("path=", param, 5))
> + cpath = av_strdup(¶m[5]);
> + else if (!av_strncasecmp("domain=", param, 7))
> + cdomain = av_strdup(¶m[7]);
> + else if (!av_strncasecmp("secure", param, 6) ||
> + !av_strncasecmp("comment", param, 7) ||
> + !av_strncasecmp("max-age", param, 7) ||
> + !av_strncasecmp("version", param, 7)) {
> + // ignore Comment, Max-Age, Secure and Version
> + } else
> + cvalue = av_strdup(param);
> +
> + if (*cookie == ';') cookie++;
> + av_free(param);
> + }
> +
> + // ensure all of the necessary values are valid
> + if (!cdomain || !cpath || !cvalue) {
> + goto done_cookie;
> + }
> +
> + // check if the request path matches the cookie path
> + if (av_strncasecmp(path, cpath, strlen(cpath)))
> + goto done_cookie;
> +
> + // the domain should be at least the size of our cookie domain and
> + domain_offset = strlen(domain) - strlen(cdomain);
> + if (domain_offset < 0)
> + goto done_cookie;
> +
> + // match the cookie domain
> + if (av_strcasecmp(&domain[domain_offset], cdomain))
> + goto done_cookie;
> +
> + // cookie parameters match, so copy the value
> + if (!value)
> + value = av_strdup(cvalue);
> + else {
nit:
if (...) {
...
} else {
...
}
is preferred style. Also missing malloc check.
> + char *tmp = value;
> + size_t str_size = sizeof(char) * (strlen(cvalue) + strlen(value) + 3);
> + value = av_malloc(str_size);
Missing malloc check.
> + av_strlcpy(value, tmp, str_size);
> + av_strlcat(value, "; ", str_size);
> + av_strlcat(value, cvalue, str_size);
given that you checked the size, I think snprintf here would be safe
and slightly simpler.
> + av_free(tmp);
> + }
> +
> + done_cookie:
> + av_free(cdomain);
> + av_free(cpath);
> + av_free(cvalue);
> +
> + av_free(c);
> +
> + // advance the token
> + if (*cookies == '\n') cookies++;
> + }
> +
> + return value;
> +}
> +
> static inline int has_header(const char *str, const char *header)
> {
> /* header + 2 to skip over CRLF prefix. (make sure you have one!) */
> @@ -460,6 +554,12 @@ static int http_connect(URLContext *h, const char *path, const char *local_path,
> if (!has_header(s->headers, "\r\nContent-Type: ") && s->content_type)
> len += av_strlcatf(headers + len, sizeof(headers) - len,
> "Content-Type: %s\r\n", s->content_type);
> + if(!has_header(s->headers, "\r\nCookie: ") && s->cookies) {
nit++: if_(...
> + char *cookies = get_cookies(s, path, hoststr);
> + len += av_strlcatf(headers + len, sizeof(headers) - len,
> + "Cookie: %s\r\n", cookies);
> + av_free(cookies);
> + }
>
> /* now add in custom headers */
> if (s->headers)
> diff --git a/libavformat/hls.c b/libavformat/hls.c
This belongs to a separate patch, please send it as a separate patch.
[...]
Thanks.
--
FFmpeg = Formidable and Faithful Mythic Peaceless Enhanced Ghost
More information about the ffmpeg-devel
mailing list