[MPlayer-dev-eng] [PATCH] -force-key-frames option

[Nicolas George]

Le sextidi 26 vendémiaire, an CCXIX, Reimar Döffinger a écrit :
> I don't really like that a simple "string in, value out" becomes
> a founction with 3 arguments and one return value.
> Maybe there really is no truly better way, but I don't
> really like it, taht is all.

The problem is that the "value out" is a float, and float is not a
sympathetic type to report additional information, especially errors.

I think the "-1E100 means error" was ugly and should be avoided, especially
if the function is to be used in several places.

The obvious choice would be NAN, but it is a portability risk. Although, now
that I look closer into it, I see that NAN is used in libav* and mplayer is
already linked with isnan. But still, I do not like it a lot.

As for the third argument, I am perfectly fine with dropping it and leaving
the "2 centuries" minor problem for another patch.

That would bring back to something that looks like the first version:

int parse_timestring(const char *str, double *time)

The return value being the length parsed, 0 for error.

> What is rts supposed to mean?
> "time", "seconds", "result" maybe?

That was Return TimeStamp. Changed into "time".

> Inconsistent indentation.

Fixed.

> Also I'd find more explicit error case helpful, e.g.:

I am not sure what you are suggesting. Always store a value in the return
argument, even in case of error?

> Renaming end to len or so might be good, too.

Done.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mplayer-forcekeyframes-20101017-1550-01-parser.diff
Type: text/x-diff
Size: 2301 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20101017/fd631145/attachment-0001.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20101017/fd631145/attachment-0001.pgp>