[FFmpeg-devel] Reorganizing Cookies (was Re: [PATCH] Replace old cookies with new cookies of the same name)

[Micah Galizia]

On 18/08/14 21:36, Nicolas George wrote:
> Le primidi 1er fructidor, an CCXXII, Micah Galizia a écrit :
>> 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.
>
> First, a small nitpick: if you read the option to get the updated values of
> the cookies after the request, then you are abusing the API, as the option
> does not have AV_OPT_FLAG_EXPORT. But for the sake of discussion we can
> assume it should be there and always was.
>
> But that does not mean that the string must be the authoritative value used
> internally. A lot of your code is spent updating the middle of the string
> while keeping the prefix and suffix constant. That makes a lot of tricky
> code, with reallocations and possible integer overflows everywhere.
>
> I am convinced that parsing the string as a whole, manipulating an internal
> data structure and rebuilding the string as a whole would give much simpler
> code. The parsing is needed for the actual Set-Cookie header; line-splitting
> is relatively easy. As for the rebuilding of the string, we have utilities
> that can take care of the reallocations and integer overflow for you; I
> think especially of AVBPrint. Here is a snippet of code:
>
>      av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
>      for (i = 0; i < http->nb_cookies; i++) {
> 	Cookie *c = &http->cookie[i];
> 	if (i)
> 	    av_bprintf(&bp, "\n");
> 	av_bprintf(&bp, "%s=%s", c->name, c->value);
> 	if (c->domain)
> 	    av_bprintf(&bp, "; domain=%s", c->domain);
> 	if (c->path)
> 	    av_bprintf(&bp, "; path=%s", c->path);
> 	if (c->extra)
> 	    av_bprintf(&bp, "; %s", c->extra);
>      }
>      if (!av_bprint_is_complete(&bp))
> 	return AVERROR(ENOMEM);
>      return av_bprint_finalize(&bp, &http->cookies);
>
>
> Another point: I believe that having a callback on some options values may
> also be quite useful. That could be done as AV_OPT_FLAG_CALLBACK with a
> function pointer in AVClass. In this particular case, the callback could
> parse the string on the fly and re-generate it only when necessary.

You lost me on the callback bit. I'm not sure I get how it'd really help.

> What do people think about that?

I'd like to reorganize the cookies. Where I come up short is how to 
store them once they're parsed. Ideally, whatever their structure, they 
should be tied into the AVOption array for http so hls can copy them 
back and forth. That doesn't really leave me with a lot of choice for 
data structures -- and I'd rather not create a new AV_OPT_TYPE.

Using what is available now I could keep the existing cookies option 
(newline delimited string) to allow external apps to set the cookies 
initially. After they are passed in I could break them into a second 
dictionary option (with the export flag) keyed by cookie name so at 
least replacement is easier. I'd probably unset the original cookie 
string too once they're broken into a dict so that there is no ambiguity 
over where they are stored. Then, hls (or any other code) could pass 
around the dict option instead of the string.

I'm open to suggestions if I've overlooked an obvious way to tie generic 
structs into the AVOptions though. Alternatively, if the dictionary 
method is enough of an improvement I'll put in patchs to get it done 
that way.

-- 
Micah