[FFmpeg-devel] [PATCH 1/4] lavu/bprint: implement av_bprint_strftime().

[Nicolas George]

Le quintidi 25 brumaire, an CCXXI, Stefano Sabatini a écrit :
> This is a bit weird, you divide by 2 and then multiply room by 2, I'd
> find more readable if you just store the wanted room value (checked
> for overflow) in room.

Done. That looks nicer indeed. We could do with a macro:

#define FF_DOUBLE_SIZE(size, max, default) \
    size ? size < max / 2 ? size * 2 : max : default

(with a lot more parentheses)

> Note: a <0 check looks more robust in case the function is extended to
> return >= 0 in case of success.

All the other tests for the same function are like that, and would need to
be changed if that happens: I would go for consistency right now.

> 
> > +            /* impossible to grow, try to manage something useful anyway */
> > +            room = av_bprint_room(buf);
> > +            if (room < 1024) {
> > +                char buf2[1024];
> > +                if ((l = strftime(buf2, sizeof(buf2), fmt, tm))) {
> > +                    av_bprintf(buf, "%s", buf2);
> > +                    return;
> > +                }
> > +            }
> > +            if (room) {
> 
> else? If not (room < 1024) this is always true.

Yes, but we want to enter the case if the "room < 1024" case failed.

The logic is:

if we can extend the bprint buffer enough to receive the text, we win;
else
    (either the buffer is already truncated or it is very near its max size)
    if we can format the string in a local 1k buffer, we win
    else
        if the buffer is not already truncated, append a dummy string and
	force it to be truncated

Note that with that logic, the strftime misdesign hits us only if the bprint
buffer is very near its max size or already truncated (due to max size or
memory exhaustion) _and_ the expanded string does not fit 1k (which is long
for a date+time).

> Also why the <1024 case?

The point of this branch is to try to get strftime to succeed in a local 1k
buffer taken on the stack. If we already have more than 1k in the bprint
buffer, there is no point in trying that.

Note that this code is quite complex because it tries to work even in insane
conditions. This was needed when strftime was used to expand the whole text
of drawtext, but now, we could state that a date+time that expands to more
than 1k is insane, and therefore write:

void av_bprint_strftime(AVBPrint *buf, const char *fmt, const struct tm *tm)
{
    char exp[1024];

    if (*fmt && !strftime(exp, sizeof(exp), fmt, tm))
	snprintf(exp, sizeof(exp), "[insane date+time string]");
    av_bprintf(buf, "%s", exp);
}

That is somewhat simpler.

> Since this is going to be used together with struct tm, maybe it would
> be a good idea to add the header which defined struct tm.

Most people will use bprint without using av_bprint_strftime() nor any
time-related functions. Including <time.h> would be a waste of time for
them. OTOH, code that uses av_bprint_strftime() will probably also use
localtime() or gmtime() and other time-related function and therefore should
include <time.h> for that.

> "formatted" ?

Fixed. There is the same typo in the doxy for av_bprintf(), I do not think
it would be to severe an heresy to fix it in the same patch.

> Uhm, this is not very clear (why can it overflow, what is "size_max huge"?).

Clarified.

Thanks for the review, new patch in the pipes.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121115/eb6e7392/attachment.asc>