[FFmpeg-devel] [PATCH] libavformat/hls: Observe Set-Cookie headers

[Micah Galizia]

On 2017-05-17 05:23 AM, wm4 wrote:
> On Sat, 6 May 2017 14:28:10 -0400
> Micah Galizia <micahgalizia at gmail.com> wrote:
>
>> On 2017-05-05 09:28 PM, wm4 wrote:
>>> On Fri,  5 May 2017 20:55:05 -0400
>>> Micah Galizia <micahgalizia at gmail.com> wrote:
>>>   
>>>> Signed-off-by: Micah Galizia <micahgalizia at gmail.com>
>>>> ---
>>>>    libavformat/hls.c | 12 ++++++++++--
>>>>    1 file changed, 10 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>>>> index bac53a4350..bda9abecfa 100644
>>>> --- a/libavformat/hls.c
>>>> +++ b/libavformat/hls.c
>>>> @@ -630,8 +630,16 @@ 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;
>>>> +
>>>> +        if (s->flags ^ AVFMT_FLAG_CUSTOM_IO)
>>>> +            av_opt_get(*pb, "cookies", AV_OPT_SEARCH_CHILDREN, (uint8_t**)&new_cookies);
>>> Did you mean & instead of ^?
>> No, the original code was structured to set *u to null (and thus did not
>> copy cookies) iff AVFMT_FLAG_CUSTOM_IO was set in the flags.  So using ^
>> is logically equivalent -- cookies are copied only if
>> AVFMT_FLAG_CUSTOM_IO is not set.
>>
>>> Did you find out yet what difference AVFMT_FLAG_CUSTOM_IO is supposed
>>> to make in the existing code?
>> Yes, I wrote back about it a day or two ago... here is the reference to
>> its original inclusion:
>> https://lists.ffmpeg.org/pipermail/ffmpeg-trac/2013-February/013020.html.
>> When the code was originally implemented we were using s->pb->opaque,
>> which in FFMPEG we could assume was a URLContext with options (one of
>> them possibly being "cookies").
>>
>> Based on the email above, that wasn't true for other apps like mplayer,
>> and could cause crashes trying to treat opaque as a URLContext. So that
>> is the purpose of checking the AVFMT_FLAG_CUSTOM_IO flag (way back in 2013).
>>
>> Now though, we don't seem to touch opaque at all. The options are stored
>> in AVIOContext's av_class, which does have options.  Based on this I
>> think both patches are safe: this version has less change, but the
>> original gets rid of the check against AVFMT_FLAG_CUSTOM_IO that I don't
>> think we need anymore.
>>
>> I hope that clears things up.
> Sorry, I missed that. I guess this code is an artifact of some severely
> unclean hook up of the AVOPtion API to AVIOContext, that breaks with
> custom I/O. I guess your patch is fine then.
>
> I just wonder how an API user is supposed to pass along cookies then.
> My code passes an AVDictionary via a "cookies" entry when opening the
> HLS demuxer with custom I/O, so I was wondering whether the patch
> changes behavior here.

I wouldn't have expected anyone to pass the cookies to the HLS demuxer 
directly -- there is an http protocol AVOption that should pass it along 
to the demuxer. But I guess thats the whole point of the custom IO, 
right? It'd replace the http demuxer?

Even so, I don't think this is a good reason to hold up the the patch - 
it corrects the problem for apps that use the http protocol and 
maintains the existing behavior -- cookies are not (and were not) copied 
when the custom flag is set because u was set to null. Am I wrong in 
that interpretation of the existing behavior?

Thanks,