[FFmpeg-devel] [PATCH 1/3] lavc/mjpegdec: add function ff_mjpeg_decode_header

[Li, Zhong]

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Michael Niedermayer
> Sent: Friday, June 28, 2019 8:52 PM
> To: FFmpeg development discussions and patches
> <ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/mjpegdec: add function
> ff_mjpeg_decode_header
> 
> On Thu, Jun 27, 2019 at 08:59:12PM +0800, Zhong Li wrote:
> > It will be reused in the following mjpeg_parser patch
> >
> > Signed-off-by: Zhong Li <zhong.li at intel.com>
> > ---
> > Mark Thompson: This seems suspicious - MJPEG is generally 4:2:2 (e.g.
> > UVC requires it), so I would expect a 4:2:2 format to be the default
> > here?  (Or maybe a longer list - VAAPI certainly supports 4:2:2, 4:2:0
> > and 4:4:4 on the same hardware.)
> > Zhong: libmfx can support jpeg baseline profile with more output formats,
> but current ffmpeg-qsv decoder/vpp can't. Will extend supported format list
> as separated patch.
> >
> >  libavcodec/mjpegdec.c | 37 ++++++++++++++++++++++++++++---------
> >  libavcodec/mjpegdec.h |  4 ++++
> >  2 files changed, 32 insertions(+), 9 deletions(-)
> >
> > diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c index
> > a65bc8d..5da66bb 100644
> > --- a/libavcodec/mjpegdec.c
> > +++ b/libavcodec/mjpegdec.c
> > @@ -157,6 +157,8 @@ av_cold int
> ff_mjpeg_decode_init(AVCodecContext *avctx)
> >      s->start_code    = -1;
> >      s->first_picture = 1;
> >      s->got_picture   = 0;
> > +    s->reinit_idct   = 0;
> > +    s->size_change   = 0;
> >      s->org_height    = avctx->coded_height;
> >      avctx->chroma_sample_location = AVCHROMA_LOC_CENTER;
> >      avctx->colorspace = AVCOL_SPC_BT470BG; @@ -302,9 +304,9 @@
> int
> > ff_mjpeg_decode_dht(MJpegDecodeContext *s)
> >      return 0;
> >  }
> >
> > -int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
> > +int ff_mjpeg_decode_header(MJpegDecodeContext *s)
> >  {
> > -    int len, nb_components, i, width, height, bits, ret, size_change;
> > +    int len, nb_components, i, width, height, bits, ret;
> >      unsigned pix_fmt_id;
> >      int h_count[MAX_COMPONENTS] = { 0 };
> >      int v_count[MAX_COMPONENTS] = { 0 }; @@ -324,7 +326,7 @@ int
> > ff_mjpeg_decode_sof(MJpegDecodeContext *s)
> >      if (s->avctx->bits_per_raw_sample != bits) {
> >          av_log(s->avctx, s->avctx->bits_per_raw_sample > 0 ?
> AV_LOG_INFO : AV_LOG_DEBUG, "Changing bps from %d to %d\n",
> s->avctx->bits_per_raw_sample, bits);
> >          s->avctx->bits_per_raw_sample = bits;
> > -        init_idct(s->avctx);
> > +        s->reinit_idct = 1;
> >      }
> 
> I think the avctx->bits_per_raw_sample value should stay in sync with the
> initialized idct
> 
> This patch would allow the bits_per_raw_sample to change and then a
> subsequent error to skip init_idct() maybe this is ok as it would be probably
> called later in a subsequent frame but i think its better if they stay closer
> together or there is nothing between them that can cause ine to exeucute
> without the other

Thanks for detail comment, actually this is an intended way to resolve a dependency:
Calling init_idct requires the decoder has been initialized. 

static void init_idct(AVCodecContext *avctx)
{
    MJpegDecodeContext *s = avctx->priv_data; 
    ...
}

But I hope ff_mjpeg_decode_header can be used for mjpeg_parser, if don't use current way, will cause segment fault when call init_idct() due to avctx->priv_data is NULL.

Probably we can change the definition of init_idct(AVCodecContext *avctx) to be init_idct(MJpegDecodeContext *s) ? (But init_idct is useless for parser).