[FFmpeg-devel] [PATCH] Support playing SMV files.

[Reimar Döffinger]

On Tue, Oct 02, 2012 at 11:50:08AM +0100, Ash Hughes wrote:
> +            vst->codec->extradata_size = sizeof(SMVCodecExtra);
> +            vst->codec->extradata = av_malloc(sizeof(SMVCodecExtra) + FF_INPUT_BUFFER_PADDING_SIZE);

You can't store extradata directly like that, if you were to mux
it like that in a container on a little-endian machine the resulting
file would only be readable on a little-endian machine again.
And that is not even going into issues like struct alignment which
could break things even when just changing compilers.

> @@ -617,9 +625,14 @@
>              if (ret < 0)
>                  goto smv_out;
>              pkt->pos -= 3;
> -            pkt->pts = wav->smv_block * wav->smv_frames_per_jpeg;
> -            wav->smv_block++;
> +            pkt->pts = wav->smv_block * wav->smv_frames_per_jpeg + wav->smv_cur_pt;
> +            wav->smv_cur_pt++;
> +            wav->smv_cur_pt %= wav->smv_frames_per_jpeg;
> +            if (wav->smv_cur_pt == 0)
> +                wav->smv_block++;

Are you trying to create one packet per final frame here?
I don't think that should be done, IMHO it complicates things
without much benefit.

> +    s->picture[0].reference = s->picture[1].reference = 3;
> +    s->picture[2].reference = 3;

Why? SMV doesn't use reference frames to my knowledge.

> +    s->avctx[0] = avctx;
> +    s->avctx[1] = avcodec_alloc_context3(s->codec);

You never actually use s->avctx[0]

> +int ff_smvjpeg_decode_frame(AVCodecContext *avctx, void *data, int *data_size,
> +                          AVPacket *avpkt)

The indentation seems a bit strange.

> +{    

Trailing whitespace can't be committed into the repository.

> +    SMVJpegDecodeContext *s = avctx->priv_data;
> +    int ret = 0;
> +    AVFrame* buf_data = &s->picture[2];
> +    AVFrame* mjpeg_data = &s->picture[0];
> +    AVFrame* output = data;
> +    int u;
> +    int cur_frame = avpkt->pts % s->frames_per_jpeg;
> +    int* force_decode = (int*)avpkt->priv;

I don't think you're supposed to use "priv" that way, it won't
work in any external players like MPlayer or VLC.
You could use side data, but as said I think this approach is wrong.
There should be only one packet per frames_per_jpeg frames.
The decoder will then return 0 for all but the first frame to produce
multiple frames from one input packet.
I suspect this is likely to uncover some bugs though.

> +        /* we can't open or close the codec in init or end so we have
> +           to do it all here. */

Why?
Also opening/closing the codec is potentially slow, so it really should
not be done each time.

> +            av_picture_copy((AVPicture*)&s->picture[2], (AVPicture*)mjpeg_data,
> +                s->pix_fmt, buf_data->width, buf_data->height);
> +    av_image_copy_skip(&s->picture[1].data, &s->picture[1].linesize,
> +        buf_data->data, buf_data->linesize, avctx->pix_fmt, avctx->width,
> +        avctx->height, cur_frame);

This is 2 copies more than what should be necessary.
The first obviously can be avoided by fixing things so you can
open/close the JPEG decoder in the open/close decoder functions.
The second can be avoided by not setting CODEC_CAP_DR1 and just
exporting the appropriate part of the decoded frame.
The whole point of CODEC_CAP_DR1 is to avoid copying, if you
end up doing a copy in the codec because of it you do the opposite
of what its purpose it.
You are by far not the only one to misunderstand this (well, I assume
you misunderstood it), so suggestions for documenting this better
are welcome.

> +    if (s->picture[2].data[0]) {
> +        av_freep(&s->picture[2].data[0]);
> +    }

The if() is unnecessary.