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

Submitted by Zhong Li on June 27, 2019, 12:59 p.m.

Details

Message ID 1561640354-12545-1-git-send-email-zhong.li@intel.com
State New
Headers show

Commit Message

Zhong Li June 27, 2019, 12:59 p.m.
It will be reused in the following mjpeg_parser patch

Signed-off-by: Zhong Li <zhong.li@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(-)

Comments

Zhong Li June 27, 2019, 1:01 p.m.
> From: Li, Zhong
> Sent: Thursday, June 27, 2019 8:59 PM
> To: ffmpeg-devel@ffmpeg.org
> Cc: Li, Zhong <zhong.li@intel.com>
> Subject: [PATCH 1/3] lavc/mjpegdec: add function ff_mjpeg_decode_header
> 
> It will be reused in the following mjpeg_parser patch
> 
> Signed-off-by: Zhong Li <zhong.li@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.

Sorry, this annotation should be part of patch 3/3. Please ignore here.
Carl Eugen Hoyos June 27, 2019, 6:55 p.m.
Am Do., 27. Juni 2019 um 14:59 Uhr schrieb Zhong Li <zhong.li@intel.com>:

> -        if (s->avctx->pix_fmt == s->hwaccel_sw_pix_fmt && !size_change) {
> +    if (!(s->got_picture && s->interlaced && (s->bottom_field == !s->interlace_polarity))) {
> +        if (s->avctx->pix_fmt == s->hwaccel_sw_pix_fmt && !s->size_change) {

Is this an (unrelated) bug fix or only vaapi-related?
I wonder if it should be in this patch for both cases.

>              s->avctx->pix_fmt = s->hwaccel_pix_fmt;
> +            s->size_change = 0;

Thank you, Carl Eugen
Zhong Li June 28, 2019, 3:41 a.m.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Carl Eugen Hoyos

> Sent: Friday, June 28, 2019 2:56 AM

> To: FFmpeg development discussions and patches

> <ffmpeg-devel@ffmpeg.org>

> Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/mjpegdec: add function

> ff_mjpeg_decode_header

> 

> Am Do., 27. Juni 2019 um 14:59 Uhr schrieb Zhong Li <zhong.li@intel.com>:

> 

> > -        if (s->avctx->pix_fmt == s->hwaccel_sw_pix_fmt

> && !size_change) {

> > +    if (!(s->got_picture && s->interlaced && (s->bottom_field

> == !s->interlace_polarity))) {

> > +        if (s->avctx->pix_fmt == s->hwaccel_sw_pix_fmt &&

> > + !s->size_change) {

> 

> Is this an (unrelated) bug fix or only vaapi-related?

> I wonder if it should be in this patch for both cases.


Hi Carl:
  This is not to fix any issue, just a tailing after refact with ff_mjpeg_decode_header():

Original code: 
    if (s->got_picture && s->interlaced && (s->bottom_field == !s->interlace_polarity)) {
        if (s->progressive) {
            avpriv_request_sample(s->avctx, "progressively coded interlaced picture");
            return AVERROR_INVALIDDATA;
        }
    } else {
      ... 

        if (s->avctx->pix_fmt == s->hwaccel_sw_pix_fmt && !size_change) {
            s->avctx->pix_fmt = s->hwaccel_pix_fmt;
        } else {
            enum AVPixelFormat pix_fmts[] = {
#if CONFIG_MJPEG_NVDEC_HWACCEL
                AV_PIX_FMT_CUDA,
#endif
#if CONFIG_MJPEG_VAAPI_HWACCEL
                AV_PIX_FMT_VAAPI,
#endif
                s->avctx->pix_fmt,
                AV_PIX_FMT_NONE,
            };
            s->hwaccel_pix_fmt = ff_get_format(s->avctx, pix_fmts);

     ...
   }

Thanks
Zhong
Michael Niedermayer June 28, 2019, 12:52 p.m.
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@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

[...]
Zhong Li July 1, 2019, 2:47 a.m.
>Sorry for not realizing this earlier (I searched for "SOF0"):

>Why is this function duplicated?

>

>Carl Eugen


Hi Carl:
  You can find the difference: here I just find frame header markers (SOF0 ~ SOF 3), mjpeg decoder try to find all markers.
Thanks
Zhong
Zhong Li July 1, 2019, 4:26 a.m.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf
> Of Michael Niedermayer
> Sent: Friday, June 28, 2019 8:52 PM
> To: FFmpeg development discussions and patches
> <ffmpeg-devel@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@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).

Patch hide | download patch | download mbox

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;
     }
     if (s->pegasus_rct)
         bits = 9;
@@ -417,7 +419,7 @@  int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
     if (width != s->width || height != s->height || bits != s->bits ||
         memcmp(s->h_count, h_count, sizeof(h_count))                ||
         memcmp(s->v_count, v_count, sizeof(v_count))) {
-        size_change = 1;
+        s->size_change = 1;
 
         s->width      = width;
         s->height     = height;
@@ -444,8 +446,6 @@  int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
             return ret;
 
         s->first_picture = 0;
-    } else {
-        size_change = 0;
     }
 
     if (s->got_picture && s->interlaced && (s->bottom_field == !s->interlace_polarity)) {
@@ -673,9 +673,28 @@  int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
             av_log(s->avctx, AV_LOG_ERROR, "Could not get a pixel format descriptor.\n");
             return AVERROR_BUG;
         }
+    }
+
+    return 0;
+}
+
+int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
+{
+    int i, ret;
+
+    ret = ff_mjpeg_decode_header(s);
+    if (ret < 0)
+        return ret;
+
+    if (s->reinit_idct) {
+        init_idct(s->avctx);
+        s->reinit_idct = 0;
+    }
 
-        if (s->avctx->pix_fmt == s->hwaccel_sw_pix_fmt && !size_change) {
+    if (!(s->got_picture && s->interlaced && (s->bottom_field == !s->interlace_polarity))) {
+        if (s->avctx->pix_fmt == s->hwaccel_sw_pix_fmt && !s->size_change) {
             s->avctx->pix_fmt = s->hwaccel_pix_fmt;
+            s->size_change = 0;
         } else {
             enum AVPixelFormat pix_fmts[] = {
 #if CONFIG_MJPEG_NVDEC_HWACCEL
@@ -728,8 +747,8 @@  int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
 
     /* totally blank picture as progressive JPEG will only add details to it */
     if (s->progressive) {
-        int bw = (width  + s->h_max * 8 - 1) / (s->h_max * 8);
-        int bh = (height + s->v_max * 8 - 1) / (s->v_max * 8);
+        int bw = (s->width  + s->h_max * 8 - 1) / (s->h_max * 8);
+        int bh = (s->height + s->v_max * 8 - 1) / (s->v_max * 8);
         for (i = 0; i < s->nb_components; i++) {
             int size = bw * bh * s->h_count[i] * s->v_count[i];
             av_freep(&s->blocks[i]);
diff --git a/libavcodec/mjpegdec.h b/libavcodec/mjpegdec.h
index 653fe7c..8f38767 100644
--- a/libavcodec/mjpegdec.h
+++ b/libavcodec/mjpegdec.h
@@ -114,6 +114,9 @@  typedef struct MJpegDecodeContext {
     int restart_interval;
     int restart_count;
 
+    int reinit_idct;
+    int size_change;
+
     int buggy_avid;
     int cs_itu601;
     int interlace_polarity;
@@ -160,6 +163,7 @@  int ff_mjpeg_decode_frame(AVCodecContext *avctx,
                           AVPacket *avpkt);
 int ff_mjpeg_decode_dqt(MJpegDecodeContext *s);
 int ff_mjpeg_decode_dht(MJpegDecodeContext *s);
+int ff_mjpeg_decode_header(MJpegDecodeContext *s);
 int ff_mjpeg_decode_sof(MJpegDecodeContext *s);
 int ff_mjpeg_decode_sos(MJpegDecodeContext *s,
                         const uint8_t *mb_bitmask,int mb_bitmask_size,