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

[Michael Niedermayer]

On Thu, Mar 27, 2008 at 04:04:56PM -0700, Eli Friedman wrote:
> This is the second revision of the patch I posted previously.  I
> believe I have addressed all the review comments.
> 

[...]
> +static int read_line(ByteIOContext * pb, char* line) {
> +    int i;

> +    int b;
> +    for (i = 0; i < RPL_LINE_LENGTH - 1; i++) {
> +        b = get_byte(pb);

declaration and initialization can be merged


[...]

> +
> +static int32_t read_int(char* line, char** endptr) {
> +    unsigned long result;

> +    errno = 0;
> +    result = strtoul(line, endptr, 10);
> +    if (*endptr == line || result > 0x7FFFFFFFU)
> +        return -1;

whats the errno=0 good for?


[...]
> +static int read_index_entry(char* line, int32_t* offset,
> +                            int32_t* video_size, int32_t* audio_size) {
> +    char* endptr;
> +
> +    *offset = read_int(line, &endptr);
> +    if (*offset < 0) return -1;
> +    line = endptr;
> +
> +    line = skip_char(',', line);
> +    if (!line) return -1;
> +
> +    *video_size = read_int(line, &endptr);
> +    if (*video_size < 0) return -1;
> +    line = endptr;
> +
> +    line = skip_char(';', line);
> +    if (!line) return -1;
> +
> +    *audio_size = read_int(line, &endptr);
> +    if (*audio_size < 0) return -1;
> +
> +    return 0;
> +}

sscanf()


[...]
> +    // Movie name
> +    if (read_line(pb, line)) return AVERROR(EIO);
> +    av_strlcpy(s->title, line, sizeof(s->title));

this should look like:

read_line(pb, s->title, sizeof(s->title));


or if you prefer to check for all errors

error |= read_line(pb, s->title, sizeof(s->title);


[...]
> +    // Video width
> +    if (read_line(pb, line)) return AVERROR(EIO);
> +    video_width = read_int(line, &endptr);
> +    if (video_width < 0) return AVERROR(EIO);
> +
> +    // Video height
> +    if (read_line(pb, line)) return AVERROR(EIO);
> +    video_height = read_int(line, &endptr);
> +    if (video_height < 0) return AVERROR(EIO);

This should look like:

video_st->codec->width = read_int(pb);
video_st->codec->height= read_int(pb);

or

vc->width = read_int(pb);
vc->height= read_int(pb);

(with vc being the AVCodecContext for the video codec)


or with some error checking if prefered, like

vc->width  = read_int(pb, &error);
vc->height = read_int(pb, &error);
...
if(error)
    ...


[...]

> +    // Video frames per second
> +    if (read_line(pb, line)) return AVERROR(EIO);
> +    // Should technically be a rational, but all the samples
> +    // I've seen have integer fps.
> +    fps = read_int(line, &endptr);
> +    if (fps < 0) return AVERROR(EIO);

please implement this correctly


[...]
> +    av_set_pts_info(vst, 32, fps, 1);
[...]
> +        av_set_pts_info(ast, 32, 1, ast->codec->bit_rate);

this looks strange


> +    }
> +
> +    url_fseek(pb, chunk_catalog_offset, SEEK_SET);
> +    if (url_feof(pb))
> +        return AVERROR(EIO);
> +

> +    total_audio_size = 0;
> +    for (i = 0; i < number_of_chunks; i++) {
> +        int32_t offset, video_size, audio_size;
> +        if (read_line(pb, line)) return AVERROR(EIO);
> +        if (read_index_entry(line, &offset, &video_size, &audio_size))
> +            return AVERROR(EIO);
> +        av_add_index_entry(vst, offset, i * rpl->frames_per_chunk,
> +                           video_size, rpl->frames_per_chunk, 0);

> +        av_add_index_entry(ast, offset + video_size, total_audio_size,
> +                           audio_size, audio_size * 8, 0);

your code will be segfaulting here


> +        total_audio_size += audio_size * 8;
> +    }

And this whole loop should be in a function not the reading of a single
entry 


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

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- 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/20080328/fcddff7d/attachment.pgp>