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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
