[FFmpeg-devel] [PATCH] Vivo demuxer

[Reimar Döffinger]

On Tue, Jul 17, 2012 at 02:36:44AM +0000, Paul B Mahol wrote:
> +    long duration;

long is basically always the wrong type to use (at least I can't
imagine any piece of code where it would end up with the most sensible
size on all combinations of 32 and 64 bit Windows and Linux).

> +    if (*buf != '0' && *buf != '1' && *buf != '2')
> +        return 0;

< '0' || > '2' ?

> +    unsigned char text[vivo->len + 1];

Variable-sized arrays are problematic (particularly since the maximum
size is more than a few kB), please avoid them.
Since the code restricts len to 14 bits anyway I think you
can just use a statically sized array in the context.

> +    text[vivo->len] = 0;
> +        *line_end = '\0';
> +        *value++ = 0;

That's quite inconsistent. IMHO just use always plain 0.

> +        } else if (!strcmp(key, "FPS")) {
> +            vivo->fps = atof(value);

Would be better to use a function with error checking (strtod).
Also, don't we have some float number parsing functions
somewhere already? That would be better because then
we would need to fix any fate failures due to precision issues
only in one place.
Lastly, as far as I can tell you don't actually use the fps for
anything except setting the time base, so I believe there is no
reason to have the fps value stored in the context?

> +    if ((ret = av_get_packet(pb, pkt, vivo->len)) < 0)
> +        goto fail;
> +
> +    // get next packet header
> +    if ((ret = vivo_get_packet_header(s)) < 0) {
> +        av_log(s, AV_LOG_ERROR, "couldn't get packet header\n");
> +        goto fail;
> +    }

I might have missed some logic behind this, but won't this always fail
for the last packed (because by definition the last packed won't have
a next packet header)?