[FFmpeg-devel] [PATCH] lavf: Add SMJPEG demuxer.

[Reimar Döffinger]

On Tue, Dec 20, 2011 at 08:05:41PM +0000, Paul B Mahol wrote:
> +static const AVCodecTag ff_codec_smjpeg_tags[] = {
> +    { CODEC_ID_ADPCM_IMA_SMJPEG,  MKTAG('A', 'P', 'C', 'M') },
> +    { CODEC_ID_PCM_S16LE,         MKTAG('N', 'O', 'N', 'E') },
> +    { CODEC_ID_MJPEG,             MKTAG('J', 'F', 'I', 'F') },
> +    { CODEC_ID_NONE, 0 },
> +};

Local symbols don't need and should not have ff_ prefixes.

> +    if (!memcmp(p->buf, "\x0\xaSMJPEG", 8))
> +        return AVPROBE_SCORE_MAX;
> +    return 0;
> +}
> +
> +    version = avio_rb32(pb);
> +    if (version) {
> +        av_log_ask_for_sample(s, "unknown version %d\n", version);
> +        return AVERROR_PATCHWELCOME;
> +    }

I still think that returning full score and then failing is
questionable.
Personally I would prefer either a lower score or not exiting hard but
trying to continue.

> +            if (avio_read(pb, comment, hlength) != hlength) {
> +                av_freep(&comment);
> +                av_log(s, AV_LOG_ERROR, "error when parsing comment\n");

"parsing" seems not quite the right word, it only seems to try to read
it really.

> +        case MKTAG('H', 'E', 'N', 'D'):
> +            // Seek to the first frame
> +            avio_seek(pb, avio_tell(pb), SEEK_SET);

I'm afraid it does not make one bit of sense more to me.
It gets the current position and the seeks to that position, what is the
point of that? Seems like a NOP to me?!?

> +    if (s->pb->eof_reached)
> +        return AVERROR_EOF;
> +    dtype = avio_rl32(s->pb);
> +    switch (dtype) {
> +    case MKTAG('s', 'n', 'd', 'D'):
> +        timestamp = avio_rb32(s->pb);
> +        size = avio_rb32(s->pb);
> +        ret = av_get_packet(s->pb, pkt, size);
> +        pkt->stream_index = sc->audio_stream_index;
> +        pkt->pts = timestamp;
> +        break;
> +    case MKTAG('v', 'i', 'd', 'D'):
> +        timestamp = avio_rb32(s->pb);
> +        size = avio_rb32(s->pb);
> +        ret = av_get_packet(s->pb, pkt, size);
> +        pkt->stream_index = sc->video_stream_index;
> +        pkt->pts = timestamp;
> +        break;
> +    case MKTAG('D', 'O', 'N', 'E'):
> +        ret = AVERROR_EOF;
> +        break;
> +    default:
> +        ret = AVERROR(EIO);

Well, but this does not really indicate an IO error, or?
INVALIDATA maybe?

> +    .codec_tag      = (const AVCodecTag* const[]){ff_codec_smjpeg_tags, 0},

Hm, I am not sure how that is intended, but generally I think this isn't
set for demuxers, it only serves a purpose for muxers really.