[FFmpeg-devel] [PATCH] HTTP options in HLS (again)
Stefano Sabatini
stefasab at gmail.com
Fri Jan 18 20:14:58 CET 2013
On date Thursday 2013-01-17 21:06:54 -0500, Micah Galizia encoded:
> On Wed, Jan 16, 2013 at 12:29 PM, Stefano Sabatini <stefasab at gmail.com>wrote:
>
> > On date Tuesday 2013-01-15 21:39:09 -0500, Micah Galizia encoded:
> >
> [...]
>
> > > static int read_chomp_line(AVIOContext *s, char *buf, int maxlen)
> > > @@ -216,6 +218,9 @@ static int parse_playlist(HLSContext *c, const char
> > *url,
> > > close_in = 1;
> > > /* Some HLS servers dont like being sent the range header */
> > > av_dict_set(&opts, "seekable", "0", 0);
> > > + /* Broker prior HTTP options that should be consistant across
> > reqs */
> > > + if (c->user_agent) av_dict_set(&opts, "user-agent",
> > c->user_agent, 0);
> > > + if (c->cookies) av_dict_set(&opts, "cookies", c->cookies, 0);
> >
> > nit: vertical align
> >
> > I think you can skip the if() checks.
> >
>
> I'm going to assume that "vertical align" means add a blank line?
No I mean:
if (c->user_agent) av_dict_set(&opts, "user-agent", c->user_agent, 0);
if (c->cookies) av_dict_set(&opts, "cookies", c->cookies, 0);
but that's a nit so you're free to ignore it (or I'll apply it before
pushing).
[...]
> > (uint8_t**)&(c->user_agent));
> > > + if (!strlen(c->user_agent))
> > > + av_freep(c->user_agent);
> >
> > Note: an av_opt_get_string() may solve the ""/NULL confusion.
> >
>
> IDK if that used to exist and has been removed but I couldn't find it or
> anything like it. Or did you want me to add it? Also, av_freep should be on
> &c->user_agent and &c->cookies (fixed in this patch).
Sorry I was not clear, the function doesn't exist but would be trivial
to implement. I'm not asking to do it for this patch though (unless
you want to).
> > > +
> > > + // get the previous cookies & set back to null if string size
> > is zero
> > > + av_opt_get(u->priv_data, "cookies", 0,
> > (uint8_t**)&(c->cookies));
> > > + if (!strlen(c->cookies))
> > > + av_freep(c->cookies);
> >
> > Aren't you leaking c->user_agent/cookies?
> >
>
> Yes. I free them with all of the variants now.
> --
> "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/hls.c b/libavformat/hls.c
> index f515dfb..426226a 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -103,6 +103,8 @@ typedef struct HLSContext {
> int64_t seek_timestamp;
> int seek_flags;
> AVIOInterruptCB *interrupt_callback;
> + char *user_agent;
> + char *cookies;
optional: add a short doxy ///< describing the fields
> } HLSContext;
>
> static int read_chomp_line(AVIOContext *s, char *buf, int maxlen)
> @@ -139,6 +141,8 @@ static void free_variant_list(HLSContext *c)
> av_free(var);
> }
> av_freep(&c->variants);
> + av_freep(&c->cookies);
> + av_freep(&c->user_agent);
> c->n_variants = 0;
> }
>
> @@ -216,6 +220,11 @@ static int parse_playlist(HLSContext *c, const char *url,
> close_in = 1;
> /* Some HLS servers dont like being sent the range header */
> av_dict_set(&opts, "seekable", "0", 0);
> +
> + // Broker prior HTTP options that should be consistant across reqs
nit: reqs -> requests
typo: consistant -> consistent
> + av_dict_set(&opts, "user-agent", c->user_agent, 0);
> + av_dict_set(&opts, "cookies", c->cookies, 0);
> +
> ret = avio_open2(&in, url, AVIO_FLAG_READ,
> c->interrupt_callback, &opts);
> av_dict_free(&opts);
> @@ -328,12 +337,17 @@ fail:
> return ret;
> }
>
> -static int open_input(struct variant *var)
> +static int open_input(HLSContext *c, struct variant *var)
> {
> AVDictionary *opts = NULL;
> int ret;
> struct segment *seg = var->segments[var->cur_seq_no - var->start_seq_no];
> +
> + // Broker prior HTTP options that should be consistant across reqs
ditto
> + av_dict_set(&opts, "user-agent", c->user_agent, 0);
> + av_dict_set(&opts, "cookies", c->cookies, 0);
> av_dict_set(&opts, "seekable", "0", 0);
> +
> if (seg->key_type == KEY_NONE) {
> ret = ffurl_open(&var->input, seg->url, AVIO_FLAG_READ,
> &var->parent->interrupt_callback, &opts);
> @@ -429,7 +443,7 @@ reload:
> goto reload;
> }
>
> - ret = open_input(v);
> + ret = open_input(c, v);
> if (ret < 0)
> return ret;
> }
> @@ -461,11 +475,25 @@ reload:
>
> static int hls_read_header(AVFormatContext *s)
> {
> + URLContext *u = s->pb->opaque;
> HLSContext *c = s->priv_data;
> int ret = 0, i, j, stream_offset = 0;
>
> c->interrupt_callback = &s->interrupt_callback;
>
> + // if the URL context is good, read important options we must broker later
> + if (u) {
> + // get the previous user agent & set back to null if string size is zero
> + av_opt_get(u->priv_data, "user-agent", 0, (uint8_t**)&(c->user_agent));
> + if (!strlen(c->user_agent))
> + av_freep(&c->user_agent);
> +
> + // get the previous cookies & set back to null if string size is zero
> + av_opt_get(u->priv_data, "cookies", 0, (uint8_t**)&(c->cookies));
> + if (!strlen(c->cookies))
> + av_freep(&c->cookies);
maybe an optional variable would be cleaner:
char *val;
av_opt_get(u->priv_data, "user-agent", 0, &val);
if (!strlen(val))
c->user_agent = val;
LGTM otherwise.
--
FFmpeg = Fiendish Fostering MultiPurpose Ecstatic Game
More information about the ffmpeg-devel
mailing list