[FFmpeg-devel] [PATCH] HTTP options in HLS (again)

[Micah Galizia]

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.

Cheers!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-broker-http-options.patch
Type: application/octet-stream
Size: 3534 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130118/770cb58f/attachment.obj>