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

[Paul B Mahol]

On 12/20/11, Reimar Doeffinger <Reimar.Doeffinger at gmx.de> wrote:
> 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))
Done.
>
>> +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?
Done.
>
>> +            av_dict_set(&s->metadata, "comment", comment,
>> +                        AV_DICT_DONT_STRDUP_VAL );
>
> Space before )
Fixed.
>
>> +        case MKTAG('_', 'S', 'N', 'D'):
>> +            if (ast)
>> +                return AVERROR_INVALIDDATA;
>
> A message might make sense, maybe there are files with
> multiple audio streams?
Fixed. I doubt that such files exist.

>
>> +            encoding                = avio_rl32(pb);
>
> I think you should set codec_tag to that value.
Done.
>
>> +            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.
Done.
>
>> +        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.
Done.
>
>> +            return 0;
>> +            break;
>
> Also both return and break is a bit overkill.
Done.
>
>> +    return AVERROR(EIO);
>
> Sure this shouldn't be EOF instead of EIO?
> At least somewhere there should be a return AVERROR_EOF
Fixed.
>
>> +    if (s->pb->eof_reached)
>> +        return AVERROR(EIO);
>
> Should be EOF I expect.
Fixed.
>
>> +        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?

There can be only audio and only video stream in file.
Also order of headers may be different and file still be playable.

> 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?
Yes, fixed.
>
>> +    default:
>> +        ret = AVERROR(EAGAIN);
>
> Why EAGAIN?? No normal demuxer should ever generate that one IMO.
Fixed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavf-Add-SMJPEG-demuxer.patch
Type: application/octet-stream
Size: 9881 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20111220/a13dd590/attachment.obj>