[FFmpeg-devel] [PATCH] lavf: Add SMJPEG demuxer.
Paul B Mahol
onemda at gmail.com
Tue Dec 20 21:05:41 CET 2011
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>
More information about the ffmpeg-devel
mailing list