| Message ID | 1561640354-12545-1-git-send-email-zhong.li@intel.com |
|---|---|
| State | New |
| Headers | show |
> 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.
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
> 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
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 [...]
>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
> 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).
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,
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(-)