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

[Ash Hughes]

> Date: Tue, 2 Oct 2012 21:17:35 +0200
> From: Reimar.Doeffinger at gmx.de
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] Support playing SMV files.
> 
> 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.

alright, I'll look more closely at how other codecs handle extradata

> 
> > @@ -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.

My intention was to have it reuse packets, like you mention below, but I got it wrong :S
> 
> > +    s->picture[0].reference = s->picture[1].reference = 3;
> > +    s->picture[2].reference = 3;
> 
> Why? SMV doesn't use reference frames to my knowledge.

ok!

> 
> > +    s->avctx[0] = avctx;
> > +    s->avctx[1] = avcodec_alloc_context3(s->codec);
> 
> You never actually use s->avctx[0]

other codecs (appear to me to) do it, e.g. kgv1dec.c so I assumed it was necessary.

> 
> > +int ff_smvjpeg_decode_frame(AVCodecContext *avctx, void *data, int *data_size,
> > +                          AVPacket *avpkt)
> 
> The indentation seems a bit strange.

missing two spaces? copied from mjpegdec and renamed...
> 
> > +{    
> 
> Trailing whitespace can't be committed into the repository.

oops!

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

ok, will rework.

> 
> > +        /* 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.

I know :( The reason for this is that if you do it in init or end, you get "insufficient thread locking around avcodec_open/close()". In libavcodec/utils.c,  in avcodec_open2, the line:

 if (entangled_thread_counter != 1)

is what triggers this. We probably don't want to call somebodies lock function twice if they supply one and I'm not sure exactly what it is protecting so I don't know how to get round this neatly. Any advice would be much appreciated!

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

Yes, I've misunderstood this, I'll have another read of documentation.
> 
> > +    if (s->picture[2].data[0]) {
> > +        av_freep(&s->picture[2].data[0]);
> > +    }
> 
> The if() is unnecessary.

ok, and thanks for the feedback :)

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel