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

[Reimar Döffinger]

On Tue, Dec 20, 2011 at 04:48:03AM +0000, Paul B Mahol wrote:
> +    if ((p->buf[0] == 0 && p->buf[1] == 0xa &&
> +         p->buf[2] == 'S' && p->buf[3] == 'M' &&
> +         p->buf[4] == 'J' && p->buf[5] == 'P' &&
> +         p->buf[6] == 'E' && p->buf[7] == 'G'))

I'd expect this could be simplified, maybe using memcmp
(either for all or just memcmp(p->buf + 2, "SMJPEG", 6))

> +static int smjpeg_read_header(AVFormatContext *s, AVFormatParameters *ap)
> +{
> +    SMJPEGContext *sc = s->priv_data;
> +    AVStream *ast = NULL, *vst = NULL;
> +    AVIOContext *pb = s->pb;
> +    uint32_t version, encoding, htype, hlength, duration;
> +    char *comment;
> +
> +    avio_skip(pb, 8); // magic
> +    version = avio_rb32(pb);
> +    if (version) {
> +        av_log(s, AV_LOG_ERROR, "unknown version %d\n", version);
> +        return AVERROR_PATCHWELCOME;

I you fail for something in the header IMO it should also be
in the probe function.
Though I admit this case might be a bit special.
Also the special "not supported feature, send sample" print function
might be better here?

> +            av_dict_set(&s->metadata, "comment", comment,
> +                        AV_DICT_DONT_STRDUP_VAL );

Space before )

> +        case MKTAG('_', 'S', 'N', 'D'):
> +            if (ast)
> +                return AVERROR_INVALIDDATA;

A message might make sense, maybe there are files with
multiple audio streams?

> +            encoding                = avio_rl32(pb);

I think you should set codec_tag to that value.

> +            if (encoding == MKTAG('A', 'P', 'C', 'M')) {
> +                ast->codec->codec_id = CODEC_ID_ADPCM_IMA_SMJPEG;
> +            } else if (encoding == MKTAG('N', 'O', 'N', 'E')) {
> +                ast->codec->codec_id = CODEC_ID_PCM_S16LE;
> +            } else {
> +                av_log(s, AV_LOG_ERROR, "unknown audio encoding %x\n", encoding);
> +            }

Maybe not worth it, but there are functions to use a table to do the
codec tag -> codec id translate.
Would make adding more variants a bit less messy.

> +        case MKTAG('_', 'V', 'I', 'D'):

Basically same comments as for audio.

> +        case MKTAG('H', 'E', 'N', 'D'):
> +            avio_seek(pb, avio_tell(pb), SEEK_SET);

That needs a comment because it sure makes no sense to me.

> +            return 0;
> +            break;

Also both return and break is a bit overkill.

> +    return AVERROR(EIO);

Sure this shouldn't be EOF instead of EIO?
At least somewhere there should be a return AVERROR_EOF

> +    if (s->pb->eof_reached)
> +        return AVERROR(EIO);

Should be EOF I expect.

> +        pkt->stream_index = sc->audio_stream_index;
> +        pkt->stream_index = sc->video_stream_index;

Uh, can sc->audio_stream_index/sc->video_stream_index ever have
other values than 0 or 1?
Not sure, but I think we had some demuxer that completely pointlessly
always stored the same value in some private struct.

> +    case MKTAG('D', 'O', 'N', 'E'):
> +        ret = AVERROR(EIO);

EOF too I guess?

> +    default:
> +        ret = AVERROR(EAGAIN);

Why EAGAIN?? No normal demuxer should ever generate that one IMO.