[FFmpeg-devel] [PATCH] ARMovie/RPL demuxer rev4

[Michael Niedermayer]

On Fri, Mar 28, 2008 at 04:13:02PM -0700, Eli Friedman wrote:
> Per subject, another revision of the ARMovie/RPL demuxer.  Addresses
> review comments.

Not all, you are still duplicating the
read_line();
read_int();
all over the place instead of reading ints directly from ByteIOContext.


[...]

> +static int32_t read_int(char* line, int* error) {
> +    unsigned long result = 0;
> +    for (; *line>='0' && *line<='9'; line++) {
> +        if (result > (0x7FFFFFFF - 9) / 10) {
> +            *error = -1;
> +            return result;

Is there any sense of returning here instead of just setting error but
returning normally? This question applies to the other similar looking spots
as well.


> +        }
> +        result = 10 * result + *line - '0';
> +    }
> +    return result;
> +}
> +
> +/** Parsing for fps, which can be a fraction. Unfortunately,
> +  * the spec for the header leaves out a lot of details,
> +  * so this is mostly guessing.
> +  */
> +static AVRational read_fps(char* line, int* error) {
> +    int64_t num = 0, den = 1;
> +    AVRational result = {-1, 1};

> +    for (; *line>='0' && *line<='9'; line++) {
> +        if (num > (INT64_MAX - 9) / 10) {
> +            *error = -1;
> +            return result;
> +        }
> +        num = 10 * num + *line - '0';
> +    }

duplicate of read_int()


[...]
> +        if (3 != sscanf(line, "%lu , %lu ; %lu",
> +                        &offset, &video_size, &audio_size))
> +            error |= -1;

You dont need a |= here, a simple = will do, this also applies to several
other places.


> +        if (offset >= 0xFFFFFFFFUL || video_size >= 0xFFFFFFFFUL ||
> +            audio_size >= 0xFFFFFFFFUL)
> +            error |= -1;

What exactly is this check good for?

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080329/e0930e86/attachment.pgp>