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

[Michael Niedermayer]

On Sat, Mar 29, 2008 at 12:55:20AM -0700, Eli Friedman wrote:
> Okay, here's the next revision of the ARMovie/RPL demuxer.

[...]
> +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;
> +        result = 10 * result + *line - '0';
> +    }
> +    return result;
> +}
> +
> +static int32_t read_line_and_int(ByteIOContext * pb, int* error)
> +{
> +    char line[RPL_LINE_LENGTH];
> +    *error |= read_line(pb, line, sizeof(line));
> +    return read_int(line, error);
> +}
> +
> +/** 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;
> +        num = 10 * num + *line - '0';
> +    }

This is still duplicate of read_int()


[...]
> +static int rpl_read_header(AVFormatContext *s, AVFormatParameters *ap)
> +{
> +    ByteIOContext *pb = s->pb;
> +    RPLContext *rpl = s->priv_data;
> +    AVStream *vst = NULL, *ast = NULL;
> +    int total_audio_size;
> +    int error = 0;
> +
> +    uint32_t i;
> +
> +    int32_t audio_format, chunk_catalog_offset, number_of_chunks;
> +    AVRational fps;
> +
> +    char line[RPL_LINE_LENGTH];
> +
> +    // The header for RPL/ARMovie files is 21 lines of text
> +    // containing the various header fields.  The fields are always
> +    // in the same order, and other text besides the first
> +    // number usually isn't important.
> +    // (The spec says that there exists some significance
> +    // for the text in a few cases; samples needed.)

> +    error |= read_line(pb, line, sizeof(line));  // ARMovie
> +    error |= read_line(pb, s->title, sizeof(s->title));  // movie name
> +    error |= read_line(pb, s->copyright, sizeof(s->copyright));  // date/copyright
> +    error |= read_line(pb, s->author, sizeof(s->author));  // author and other

vertical align


> +
> +    // video headers
> +    vst = av_new_stream(s, 0);
> +    if (!vst)
> +        return AVERROR(ENOMEM);
> +    vst->codec->codec_type = CODEC_TYPE_VIDEO;

> +    vst->codec->codec_tag = read_line_and_int(pb, &error);  // video format
> +    vst->codec->width = read_line_and_int(pb, &error);  // video width
> +    vst->codec->height = read_line_and_int(pb, &error);  // video height

vertical align


[...]
> +    // Audio headers    

trailing whitespace, and i suggest you remove all of that, and not let me
point to each one by one (will take less time for you for certain ...)


[...]
> +    error |= read_line(pb, line, sizeof(line));  // number of chunks in the file
> +    number_of_chunks = read_int(line, &error);

hmm


[...]
> +    if (stream->codec->codec_type == CODEC_TYPE_VIDEO &&
> +        stream->codec->codec_tag == 124) {
> +        // We have to split Escape 124 frames because there are
> +        // multiple frames per chunk in Escape 124 samples.
> +        uint32_t frame_size, frame_flags;
> +        if (rpl->frame_in_part == 0)
> +            if (url_fseek(pb, index_entry->pos, SEEK_SET) < 0)
> +                return AVERROR(EIO);
> +
> +        frame_flags = get_le32(pb);
> +        frame_size = get_le32(pb);
> +        if (url_fseek(pb, -8, SEEK_CUR) < 0)
> +            return AVERROR(EIO);
> +
> +        ret = av_get_packet(pb, pkt, frame_size);
> +        if (ret != frame_size) {
> +            av_free_packet(pkt);
> +            return AVERROR(EIO);
> +        }
> +        pkt->duration = 1;
> +        pkt->pts = index_entry->timestamp + rpl->frame_in_part;
> +        pkt->stream_index = rpl->chunk_part;
> +
> +        rpl->frame_in_part++;
> +        if (rpl->frame_in_part == rpl->frames_per_chunk) {
> +            rpl->frame_in_part = 0;
> +            rpl->chunk_part++;
> +        }
> +    } else {

> +        url_fseek(pb, index_entry->pos, SEEK_SET);

This is the same on both sides of the if/else and can be factored


> +        ret = av_get_packet(pb, pkt, index_entry->size);
> +        if (ret != index_entry->size) {
> +            av_free_packet(pkt);
> +            return AVERROR(EIO);
> +        }
> +
> +        if (stream->codec->codec_type == CODEC_TYPE_VIDEO) {
> +            // frames_per_chunk should always be one here; the header
> +            // parsing will warn if it isn't.
> +            pkt->duration = rpl->frames_per_chunk;
> +        } else {
> +            // All the audio codecs supported in this container
> +            // (at least so far) are constant-bitrate.
> +            pkt->duration = ret * 8;
> +        }
> +        pkt->pts = index_entry->timestamp;
> +        pkt->stream_index = rpl->chunk_part;
> +        rpl->chunk_part++;
> +    }

At least the first packet of each stream should have PKT_FLAG_KEY set 


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

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/c495493d/attachment.pgp>