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

[Paul B Mahol]

On 12/21/11, Reimar Doeffinger <Reimar.Doeffinger at gmx.de> wrote:
> 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.
Fixed.
>
>> +    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.

Fixed.
>
>> +            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.

Fixed.
>
>> +        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?!?

Indeed. Fixed.
>
>> +    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?

Fixed.
>
>> +    .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.
Removed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavf-Add-SMJPEG-demuxer.patch
Type: application/octet-stream
Size: 9533 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20111221/d4dff510/attachment.obj>