[FFmpeg-devel] [PATCH] HTTP options in HLS (again)
Micah Galizia
micahgalizia at gmail.com
Tue Jan 22 03:12:22 CET 2013
On Fri, Jan 18, 2013 at 9:24 PM, Micah Galizia <micahgalizia at gmail.com>wrote:
> On Fri, Jan 18, 2013 at 2:14 PM, Stefano Sabatini <stefasab at gmail.com>wrote:
>
>> 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
>>
>
> Done.
>
>
>> > } 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
>>
>
> Done.
>
>
>> > + 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
>>
>
> Done
>
>
>>
>> > + 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;
>>
>
> No, because then I would have to assign or else free, so we add a bunch
> more lines.
>
>
>> LGTM otherwise.
>>
>
> TY, git format-patch attached.
>
Hi, has anyone had a chance to review this?
TIA
--
"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
More information about the ffmpeg-devel
mailing list