[FFmpeg-devel] [PATCH] Replace old cookies with new cookies of the same name

[Micah Galizia]

On Sun, Aug 17, 2014 at 9:29 PM, Nicolas George <george at nsup.org> wrote:
> Le decadi 30 thermidor, an CCXXII, Micah Galizia a écrit :
>> When new cookie values (with the same name as an existing cookie) are
>> returned in an HLS stream, the current implementation will append the
>> new cookie to the list of cookies. This causes FFMPEG to send multiple
>> cookie values for the same cookie and in some cases exceed the http
>> header size in some cases.
>>
>> The patch attached resolves the issue.
>
> Thanks for the patch. Below are a few technical remarks. But before that, a
> general remark:
>
> char *cookies;  ///< holds newline (\n) delimited Set-Cookie header field
>                      values (without the "Set-Cookie: " field name)
>
> Well, this is just awful. This is obviously not your fault, this is existing
> code, but it is no way of building anything: it can do for a quick-and-dirty
> first implementation to get things working, but it makes a very bad ground
> to build upon.
>
> It seems to me that most of your patch is there to deal with that awful data
> structure. A large part of the existing code too, for that matter.
>
> IMHO, it would be much better to rework the current code to use a proper
> data structure. A dynarray of structures with name, value, path and domain
> already split seems like the best option.
>
> I suspect it would even be simpler to do that than making sure your patch is
> correct in its current form.
>
> Are you interested in that?

Yes & no. I agree its not an ideal implementation (it actually was
mine to begin with) to just use a string full of cookies. But we can't
pass around complex structures through avopts, which is where we would
really see the benefit. As you & Mark pointed out, we need to maintain
compatibility with the current format, so why why go through parsing
the cookie list each time if 1 in 100 requests actually sends back an
updated cookie, and even then you'd have to dump it back out to a
string and parse it again to pass it between http requests.... all
this considered, I'd say I'm definitely leaning to "no" unless there
is something I've overlooked.

I'll follow up separately on the issues in the patch.