[FFmpeg-devel] Observe Set-Cookie headers in HLS streams

Signed-off-by: Micah Galizia <micahgalizia@gmail.com>
---
 libavformat/hls.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

On Thu,  6 Apr 2017 22:48:59 -0400
Micah Galizia <micahgalizia@gmail.com> wrote:

> Signed-off-by: Micah Galizia <micahgalizia@gmail.com>
> ---
>  libavformat/hls.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index bac53a4..ab81863 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -630,8 +630,14 @@ static int open_url(AVFormatContext *s, AVIOContext **pb, const char *url,
>      ret = s->io_open(s, pb, url, AVIO_FLAG_READ, &tmp);
>      if (ret >= 0) {
>          // update cookies on http response with setcookies.
> -        void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb;
> -        update_options(&c->cookies, "cookies", u);
> +        char *new_cookies = NULL;
> +
> +        av_opt_get(*pb, "cookies", AV_OPT_SEARCH_CHILDREN, (uint8_t**)&new_cookies);
> +        if (new_cookies) {
> +            av_free(c->cookies);
> +            c->cookies = new_cookies;
> +        }
> +
>          av_dict_set(&opts, "cookies", c->cookies, 0);
>      }
>  

So far, all code is doing the same thing by calling update_options()
on pb, or NULL if it's a custom IO context.

What you seem to change is always using the pb (even if it's a custom
context), duplicating the update_options() code (subtly changing some
corner case behavior), and, this is probably the important one, you use
*pb instead of s->pb.

I suspect the latter is the only change that matters, and you probably
want:

void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : *pb;

Now I have no idea why it checks "s->flags & AVFMT_FLAG_CUSTOM_IO", or
what it means, but it feels very wrong. A common case is probably that
the main playlist is accessed through a custom AVIOContext, but further
nested playlists or actual .ts data probably use libavformat's code.

I'm thinking that all uses of AVFMT_FLAG_CUSTOM_IO anywhere are
probably bugs.

On 2017-04-07 02:48 AM, wm4 wrote:
> On Thu,  6 Apr 2017 22:48:59 -0400
> Micah Galizia<micahgalizia@gmail.com> <mailto:micahgalizia@gmail.com>  wrote:
>
>> Signed-off-by: Micah Galizia<micahgalizia@gmail.com> <mailto:micahgalizia@gmail.com>
>> ---
>>   libavformat/hls.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>> index bac53a4..ab81863 100644
>> --- a/libavformat/hls.c
>> +++ b/libavformat/hls.c
>> @@ -630,8 +630,14 @@ static int open_url(AVFormatContext *s, AVIOContext **pb, const char *url,
>>       ret = s->io_open(s, pb, url, AVIO_FLAG_READ, &tmp);
>>       if (ret >= 0) {
>>           // update cookies on http response with setcookies.
>> -        void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb;
>> -        update_options(&c->cookies, "cookies", u);
>> +        char *new_cookies = NULL;
>> +
>> +        av_opt_get(*pb, "cookies", AV_OPT_SEARCH_CHILDREN, (uint8_t**)&new_cookies);
>> +        if (new_cookies) {
>> +            av_free(c->cookies);
>> +            c->cookies = new_cookies;
>> +        }
>> +
>>           av_dict_set(&opts, "cookies", c->cookies, 0);
>>       }
>>   
> So far, all code is doing the same thing by calling update_options()
> on pb, or NULL if it's a custom IO context.
>
> What you seem to change is always using the pb (even if it's a custom
> context), duplicating the update_options() code (subtly changing some
> corner case behavior), and, this is probably the important one, you use
> *pb instead of s->pb.

True. But, the cookies option of *pb comes back NULL _unless_ they are 
updated, so we should check first before replacing. I could also update 
update_options, but that is probably overkill.  I haven't really chased 
down the reason why the cookies sometimes come back (if they get 
changed) and sometimes don't.

> I suspect the latter is the only change that matters, and you probably
> want:
>
> void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : *pb;

Yes, but then IIRC the flag was always set and we would actually try to 
pull cookies from NULL, which you allude to below.  Also, without the 
ternary assignment, u is just *pb, so why create another variable?

> Now I have no idea why it checks "s->flags & AVFMT_FLAG_CUSTOM_IO", or
> what it means, but it feels very wrong. A common case is probably that
> the main playlist is accessed through a custom AVIOContext, but further
> nested playlists or actual .ts data probably use libavformat's code.
>
> I'm thinking that all uses of AVFMT_FLAG_CUSTOM_IO anywhere are
> probably bugs.

I'm happy to take it out the check for AVFMT_FLAG_CUSTOM_IO -- there are 
only two and on the streams I tested it had no effect. However, I was 
hoping someone could explain why its there. If someone wants to write 
back and explain I'd appreciate it, otherwise, I'll send a patch to get 
rid of it.

Thanks,

Hi,

This one gets rid of the check against AVFMT_FLAG_CUSTOM_IO. Let me know if anything else needs changing.

Thanks,

Hi,

I was hoping to get this one in too so I've named the patch appropriately.

With this change, cookie authenticated streams (eg: Neulion) should play properly again.

Thanks,