[FFmpeg-devel] ARMovie/RPL demuxer

[Michael Niedermayer]

On Sun, Mar 23, 2008 at 12:31:25AM -0700, Eli Friedman wrote:
> Per subject, a ARMovie/RPL demuxer; format documented at
> http://wiki.multimedia.cx/index.php?title=ARMovie, patch adapted from
> http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2007-November/037984.html.
> 
> This implementation can play the audio for all the samples at
> http://samples.mplayerhq.hu/game-formats/rpl/. The video for all those
> samples depends on the various Escape video codecs
> (http://wiki.multimedia.cx/index.php?title=Escape_122,
> http://wiki.multimedia.cx/index.php?title=Escape_124,
> http://wiki.multimedia.cx/index.php?title=Escape_130 /
> http://pirsoft-dsl-dropzone.de/dec130-spec.txt).
> 
> It would be nice if I had more samples to test with, but I'm having
> trouble finding files in this format.  Help here would be appreciated.
> 
> I think I've addressed all the comments from the original review.  The
> part I'm least sure about in this patch is the pts calculations; I
> think what I'm doing is sane, but I'm not entirely sure how pts is
> supposed to work.
[...]
> +// 256 is arbitrary, but should be big enough for any reasonable file.
> +#define RPL_LINE_LENGTH 256

not doxygen compatible comment


[...]
> +typedef struct RPLFrame {
> +    uint32_t offset;
> +    uint32_t video_size;
> +    uint32_t audio_size;
> +} RPLFrame;

instead of your own index system, using 
AStream.index_entries / av_add_index_entry() seems to make more sense.


[...]
> +static int read_line(ByteIOContext * pb, char* line) {
> +    int i;
> +    int b;
> +    for (i = 0; i < RPL_LINE_LENGTH; i++) {
> +        b = get_byte(pb);
> +        if (b == 0)
> +            break;
> +        if (b == '\n') {
> +            line[i] = '\0';
> +            return 0;
> +        }
> +        line[i] = b;
> +    }
> +    return 1;
> +}

add a comment that says that this does not gurantee line to be zero
terminated


> +
> +static int read_int(char* line, uint32_t* number) {
> +  unsigned long result;

inconsistent indention


> +  char* endptr;

> +  errno = 0;
> +  result = strtoul(line, &endptr, 10);
> +  if (endptr == line || errno || result > 0xFFFFFFFFU)
> +    return 1;

I do not like this code at all, the errno access is ugly.

Also error returns are consistently negative numbers in libav*
And i prefer to use return to return the value where possible.


> +  *number = result;
> +  return 0;
> +}
> +

> +static int read_float(char* line, float* number) {
> +  float result;
> +  char* endptr;
> +  errno = 0;
> +  result = strtod(line, &endptr);
> +  if (endptr == line || !isfinite(result))
> +    return 1;
> +  *number = result;
> +  return 0;
> +}

floats are not accurate things, this makes regression tests hard.
The string should be parsed into a AVRational with no intermediate
float/double.


> +
> +static int read_index_entry(char* line, uint32_t* offset, 
> +                            uint32_t* video_size, uint32_t* audio_size) {
> +  unsigned long result;
> +  char* endptr;
> +
> +  errno = 0;
> +  result = strtoul(line, &endptr, 10);
> +  if (endptr == line || errno || result > 0xFFFFFFFFU)
> +    return 1;
> +  *offset = result;
> +  line = endptr;
> +
> +  while (isspace(*line))
> +    line++;
> +  if (*line != ',')
> +    return 1;
> +  line++;
> +
> +  errno = 0;
> +  result = strtoul(line, &endptr, 10);
> +  if (endptr == line || errno || result > 0xFFFFFFFFU)
> +    return 1;
> +  *video_size = result;
> +  line = endptr;

code duplication


[...]
> +    // Movie name
> +    if (read_line(pb, line)) return AVERROR(EIO);
> +
> +    // Date/copyright
> +    if (read_line(pb, line)) return AVERROR(EIO);
> +
> +    // Author and other
> +    if (read_line(pb, line)) return AVERROR(EIO);

There are places for such metadata in AVFormatContext, this should be read
into that.


> +
> +    // Video format
> +    if (read_line(pb, line)) return AVERROR(EIO);
> +    if (read_int(line, &rpl->video_format)) return AVERROR(EIO);
> +
> +    // Video width
> +    if (read_line(pb, line)) return AVERROR(EIO);
> +    if (read_int(line, &rpl->video_width)) return AVERROR(EIO);
> +
> +    // Video height
> +    if (read_line(pb, line)) return AVERROR(EIO);
> +    if (read_int(line, &rpl->video_height)) return AVERROR(EIO);

These things should be directly read into AVCodecContext with no
intermediate if possible.


> +
> +    // Video bits per pixel
> +    if (read_line(pb, line)) return AVERROR(EIO);

Should be stored in bits_per_sample


[...]
> +    if (rpl->number_of_chunks >= SIZE_MAX / sizeof(RPLFrame))
> +        return AVERROR(ENOMEM);
> +    rpl->index = av_malloc(rpl->number_of_chunks * sizeof(RPLFrame));

av_malloc() takes a unsigned int so this check is not correct.


[...]
> +        default:
> +            av_log(NULL, AV_LOG_WARNING,

all new av_log() should have a context


> +                   "RPL video format %i not supported yet!\n",
> +                   rpl->video_format);
> +            vst->codec->codec_id = CODEC_ID_NONE;
> +    }
> +    vst->codec->width = rpl->video_width;
> +    vst->codec->height = rpl->video_height;
> +    r = av_d2q(rpl->fps, DEFAULT_FRAME_RATE_BASE);
> +    av_set_pts_info(vst, 32, r.num, r.den);
> +
> +    if (rpl->audio_format) {
> +        ast = av_new_stream(s, 0);
> +        if (!ast)
> +            return AVERROR(ENOMEM);
> +        ast->codec->codec_type = CODEC_TYPE_AUDIO;
> +        ast->codec->channels = rpl->audio_channels;
> +        ast->codec->sample_rate = rpl->audio_rate;
> +        ast->codec->bits_per_sample = rpl->audio_bps;
> +        ast->codec->bit_rate = rpl->audio_rate * rpl->audio_bps * rpl->audio_channels;
> +
> +        ast->codec->codec_id = CODEC_ID_NONE;
> +        switch (rpl->audio_format) {

codec_tag= rpl->audio_format
same for video


[...]
> +        ret = av_get_packet(pb, pkt, fr->video_size);
> +        if (ret != fr->video_size)
> +            return AVERROR(EIO);

memleak


> +
> +        // There should always be frames_per_chunk frames in a chunk.
> +        // (This should be checked once video decoding is implemented.)
> +        pkt->duration = rpl->frames_per_chunk;

1 AVPacket == 1 frame. Either demuxer or parser has to split them if there
are more.


> +        pkt->pts = rpl->chunk_number * rpl->frames_per_chunk;
> +        pkt->stream_index = 0;
> +    } else {
> +        url_fseek(pb, fr->offset + fr->video_size, SEEK_SET);
> +        ret = av_get_packet(pb, pkt, fr->audio_size);
> +        if (ret != fr->audio_size)
> +            return AVERROR(EIO);
> +
> +        // All the audio codecs supported in this container
> +        // (at least so far) are constant-bitrate.
> +        // The container apparently doesn't require a constant number
> +        // of samples per packet.
> +        pkt->duration = ret * 8;
> +        pkt->pts = rpl->audio_timestamp;
> +        rpl->audio_timestamp += pkt->duration;
> +        pkt->stream_index = 1;
> +    }
> +

> +    pkt->size = ret;

unneeded

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

I count him braver who overcomes his desires than him who conquers his
enemies for the hardest victory is over self. -- Aristotle
-------------- 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/20080324/9218e8c4/attachment.pgp>