| Message ID | d8f7ddc9-8e78-0963-ff33-f72aaf9dc496@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 20 Jan 2017 17:41:01 +0800 "Huang, Zhengxu" <zhengxu.maxwell@gmail.com> wrote: > From 2149f87637ab941be14828f7ae2c224908784c7d Mon Sep 17 00:00:00 2001 > From: Zhengxu <zhengxu.maxwell@gmail.com> > Date: Wed, 4 Jan 2017 16:43:43 +0800 > Subject: [PATCH] ffmpeg_qsv.c: Init an hwframes_context for decoder instead of > encoder. > > We consider that encoder is the last stage in the pipeline. Thinking > about filters, they get hwframes_context from their inputs. Likewise, > encoder should get hwframes_context from its input instead creating a > new faker one. Encoder can get acuurate parameters by doing so. the idea is right. What ffmpeg.c does is a gross hack that avconv doesn't have or need. Ideally, ffmpeg_qsv.c would look like avconv's equivalent file: https://git.libav.org/?p=libav.git;a=blob;f=avconv_qsv.c;hb=HEAD (It's quite possible that this still lacks avconv changes needed for this, and which were either skipped during merge, or not merged yet.) > Signed-off-by: ChaoX A Liu <chaox.a.liu@gmail.com> > Signed-off-by: Huang, Zhengxu <zhengxu.maxwell@gmail.com> > Signed-off-by: Andrew, Zhang <huazh407@gmail.com> > --- > ffmpeg_qsv.c | 95 +++++++++++++++++++++++++++--------------------------------- > 1 file changed, 43 insertions(+), 52 deletions(-) > > diff --git a/ffmpeg_qsv.c b/ffmpeg_qsv.c > index 86824b6..8cedb7e 100644 > --- a/ffmpeg_qsv.c > +++ b/ffmpeg_qsv.c > @@ -81,25 +81,26 @@ int qsv_init(AVCodecContext *s) > return ret; > } > > - av_buffer_unref(&ist->hw_frames_ctx); > - ist->hw_frames_ctx = av_hwframe_ctx_alloc(hw_device_ctx); > - if (!ist->hw_frames_ctx) > - return AVERROR(ENOMEM); > - > - frames_ctx = (AVHWFramesContext*)ist->hw_frames_ctx->data; > - frames_hwctx = frames_ctx->hwctx; > - > - frames_ctx->width = FFALIGN(s->coded_width, 32); > - frames_ctx->height = FFALIGN(s->coded_height, 32); > - frames_ctx->format = AV_PIX_FMT_QSV; > - frames_ctx->sw_format = s->sw_pix_fmt; > - frames_ctx->initial_pool_size = 64; > - frames_hwctx->frame_type = MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET; > - > - ret = av_hwframe_ctx_init(ist->hw_frames_ctx); > - if (ret < 0) { > - av_log(NULL, AV_LOG_ERROR, "Error initializing a QSV frame pool\n"); > - return ret; > + if(!ist->hw_frames_ctx) { Why this check? It's pointless - hw_frames_ctx is unset when get_format is called. It also changes indentation, which makes reviewing harder. > + ist->hw_frames_ctx = av_hwframe_ctx_alloc(hw_device_ctx); > + if (!ist->hw_frames_ctx) > + return AVERROR(ENOMEM); > + > + frames_ctx = (AVHWFramesContext*)ist->hw_frames_ctx->data; > + frames_hwctx = frames_ctx->hwctx; > + > + frames_ctx->width = FFALIGN(s->coded_width, 32); > + frames_ctx->height = FFALIGN(s->coded_height, 32); > + frames_ctx->format = AV_PIX_FMT_QSV; > + frames_ctx->sw_format = s->sw_pix_fmt; > + frames_ctx->initial_pool_size = 64; > + frames_hwctx->frame_type = MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET; > + > + ret = av_hwframe_ctx_init(ist->hw_frames_ctx); > + if (ret < 0) { > + av_log(NULL, AV_LOG_ERROR, "Error initializing a QSV frame pool\n"); > + return ret; > + } > } > > ist->hwaccel_get_buffer = qsv_get_buffer; > @@ -114,9 +115,8 @@ int qsv_transcode_init(OutputStream *ost) > const enum AVPixelFormat *pix_fmt; > > int err, i; > - AVBufferRef *encode_frames_ref = NULL; > - AVHWFramesContext *encode_frames; > - AVQSVFramesContext *qsv_frames; > + AVHWFramesContext *frames_ctx; > + AVQSVFramesContext *frames_hwctx; > > /* check if the encoder supports QSV */ > if (!ost->enc->pix_fmts) > @@ -150,42 +150,33 @@ int qsv_transcode_init(OutputStream *ost) > if (!hw_device_ctx) { > err = qsv_device_init(ist); > if (err < 0) > - goto fail; > - } > - > - // This creates a dummy hw_frames_ctx for the encoder to be > - // suitably initialised. It only contains one real frame, so > - // hopefully doesn't waste too much memory. > - > - encode_frames_ref = av_hwframe_ctx_alloc(hw_device_ctx); > - if (!encode_frames_ref) { > - err = AVERROR(ENOMEM); > - goto fail; > + return err; > } > - encode_frames = (AVHWFramesContext*)encode_frames_ref->data; > - qsv_frames = encode_frames->hwctx; > > - encode_frames->width = FFALIGN(ist->resample_width, 32); > - encode_frames->height = FFALIGN(ist->resample_height, 32); > - encode_frames->format = AV_PIX_FMT_QSV; > - encode_frames->sw_format = AV_PIX_FMT_NV12; > - encode_frames->initial_pool_size = 1; > + ist->dec_ctx->pix_fmt = AV_PIX_FMT_QSV; > + ist->resample_pix_fmt = AV_PIX_FMT_QSV; These lines should be removed. AFAIK they're hacks to make ffmpeg.c "behave". Please fix the root of the issue instead. > + ist->hw_frames_ctx = av_hwframe_ctx_alloc(hw_device_ctx); > + if (!ist->hw_frames_ctx) > + return AVERROR(ENOMEM); > > - qsv_frames->frame_type = MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET; > + frames_ctx = (AVHWFramesContext*)ist->hw_frames_ctx->data; > + frames_hwctx = frames_ctx->hwctx; > > - err = av_hwframe_ctx_init(encode_frames_ref); > - if (err < 0) > - goto fail; > + frames_ctx->width = FFALIGN(ist->resample_width, 32); > + frames_ctx->height = FFALIGN(ist->resample_height, 32); > + frames_ctx->format = AV_PIX_FMT_QSV; > + frames_ctx->sw_format = AV_PIX_FMT_NV12; > + frames_ctx->initial_pool_size = 64; > + frames_hwctx->frame_type = MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET; Why are you creating another frames_ctx here? > > - ist->dec_ctx->pix_fmt = AV_PIX_FMT_QSV; > - ist->resample_pix_fmt = AV_PIX_FMT_QSV; > + err = av_hwframe_ctx_init(ist->hw_frames_ctx); > + if (err < 0) { > + av_buffer_unref(&ist->hw_frames_ctx); > + av_log(NULL, AV_LOG_ERROR, "Error initializing a QSV frame pool\n"); > + return err; > + } > > - ost->enc_ctx->pix_fmt = AV_PIX_FMT_QSV; > - ost->enc_ctx->hw_frames_ctx = encode_frames_ref; > + ost->enc_ctx->pix_fmt = AV_PIX_FMT_QSV; enc_ctx should not be accessed. As you said yourself, filters decide the encoder input anyway. > > return 0; > - > -fail: > - av_buffer_unref(&encode_frames_ref); > - return err; > }
在 2017/1/20 18:05, wm4 写道: > On Fri, 20 Jan 2017 17:41:01 +0800 > "Huang, Zhengxu" <zhengxu.maxwell@gmail.com> wrote: > >> From 2149f87637ab941be14828f7ae2c224908784c7d Mon Sep 17 00:00:00 2001 >> From: Zhengxu <zhengxu.maxwell@gmail.com> >> Date: Wed, 4 Jan 2017 16:43:43 +0800 >> Subject: [PATCH] ffmpeg_qsv.c: Init an hwframes_context for decoder instead of >> encoder. >> >> We consider that encoder is the last stage in the pipeline. Thinking >> about filters, they get hwframes_context from their inputs. Likewise, >> encoder should get hwframes_context from its input instead creating a >> new faker one. Encoder can get acuurate parameters by doing so. > the idea is right. What ffmpeg.c does is a gross hack that avconv > doesn't have or need. Ideally, ffmpeg_qsv.c would look like avconv's > equivalent file: > > https://git.libav.org/?p=libav.git;a=blob;f=avconv_qsv.c;hb=HEAD This patch seem to be the right way to fix this issue. Maybe it should be merged ASAP after validation. > > (It's quite possible that this still lacks avconv changes needed for > this, and which were either skipped during merge, or not merged yet.) > >> Signed-off-by: ChaoX A Liu <chaox.a.liu@gmail.com> >> Signed-off-by: Huang, Zhengxu <zhengxu.maxwell@gmail.com> >> Signed-off-by: Andrew, Zhang <huazh407@gmail.com> >> --- >> ffmpeg_qsv.c | 95 +++++++++++++++++++++++++++--------------------------------- >> 1 file changed, 43 insertions(+), 52 deletions(-) >> >> diff --git a/ffmpeg_qsv.c b/ffmpeg_qsv.c >> index 86824b6..8cedb7e 100644 >> --- a/ffmpeg_qsv.c >> +++ b/ffmpeg_qsv.c >> @@ -81,25 +81,26 @@ int qsv_init(AVCodecContext *s) >> return ret; >> } >> >> - av_buffer_unref(&ist->hw_frames_ctx); >> - ist->hw_frames_ctx = av_hwframe_ctx_alloc(hw_device_ctx); >> - if (!ist->hw_frames_ctx) >> - return AVERROR(ENOMEM); >> - >> - frames_ctx = (AVHWFramesContext*)ist->hw_frames_ctx->data; >> - frames_hwctx = frames_ctx->hwctx; >> - >> - frames_ctx->width = FFALIGN(s->coded_width, 32); >> - frames_ctx->height = FFALIGN(s->coded_height, 32); >> - frames_ctx->format = AV_PIX_FMT_QSV; >> - frames_ctx->sw_format = s->sw_pix_fmt; >> - frames_ctx->initial_pool_size = 64; >> - frames_hwctx->frame_type = MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET; >> - >> - ret = av_hwframe_ctx_init(ist->hw_frames_ctx); >> - if (ret < 0) { >> - av_log(NULL, AV_LOG_ERROR, "Error initializing a QSV frame pool\n"); >> - return ret; >> + if(!ist->hw_frames_ctx) { > Why this check? It's pointless - hw_frames_ctx is unset when get_format > is called. > > It also changes indentation, which makes reviewing harder. > >> + ist->hw_frames_ctx = av_hwframe_ctx_alloc(hw_device_ctx); >> + if (!ist->hw_frames_ctx) >> + return AVERROR(ENOMEM); >> + >> + frames_ctx = (AVHWFramesContext*)ist->hw_frames_ctx->data; >> + frames_hwctx = frames_ctx->hwctx; >> + >> + frames_ctx->width = FFALIGN(s->coded_width, 32); >> + frames_ctx->height = FFALIGN(s->coded_height, 32); >> + frames_ctx->format = AV_PIX_FMT_QSV; >> + frames_ctx->sw_format = s->sw_pix_fmt; >> + frames_ctx->initial_pool_size = 64; >> + frames_hwctx->frame_type = MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET; >> + >> + ret = av_hwframe_ctx_init(ist->hw_frames_ctx); >> + if (ret < 0) { >> + av_log(NULL, AV_LOG_ERROR, "Error initializing a QSV frame pool\n"); >> + return ret; >> + } >> } >> >> ist->hwaccel_get_buffer = qsv_get_buffer; >> @@ -114,9 +115,8 @@ int qsv_transcode_init(OutputStream *ost) >> const enum AVPixelFormat *pix_fmt; >> >> int err, i; >> - AVBufferRef *encode_frames_ref = NULL; >> - AVHWFramesContext *encode_frames; >> - AVQSVFramesContext *qsv_frames; >> + AVHWFramesContext *frames_ctx; >> + AVQSVFramesContext *frames_hwctx; >> >> /* check if the encoder supports QSV */ >> if (!ost->enc->pix_fmts) >> @@ -150,42 +150,33 @@ int qsv_transcode_init(OutputStream *ost) >> if (!hw_device_ctx) { >> err = qsv_device_init(ist); >> if (err < 0) >> - goto fail; >> - } >> - >> - // This creates a dummy hw_frames_ctx for the encoder to be >> - // suitably initialised. It only contains one real frame, so >> - // hopefully doesn't waste too much memory. >> - >> - encode_frames_ref = av_hwframe_ctx_alloc(hw_device_ctx); >> - if (!encode_frames_ref) { >> - err = AVERROR(ENOMEM); >> - goto fail; >> + return err; >> } >> - encode_frames = (AVHWFramesContext*)encode_frames_ref->data; >> - qsv_frames = encode_frames->hwctx; >> >> - encode_frames->width = FFALIGN(ist->resample_width, 32); >> - encode_frames->height = FFALIGN(ist->resample_height, 32); >> - encode_frames->format = AV_PIX_FMT_QSV; >> - encode_frames->sw_format = AV_PIX_FMT_NV12; >> - encode_frames->initial_pool_size = 1; >> + ist->dec_ctx->pix_fmt = AV_PIX_FMT_QSV; >> + ist->resample_pix_fmt = AV_PIX_FMT_QSV; > These lines should be removed. AFAIK they're hacks to make ffmpeg.c > "behave". Please fix the root of the issue instead. > >> + ist->hw_frames_ctx = av_hwframe_ctx_alloc(hw_device_ctx); >> + if (!ist->hw_frames_ctx) >> + return AVERROR(ENOMEM); >> >> - qsv_frames->frame_type = MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET; >> + frames_ctx = (AVHWFramesContext*)ist->hw_frames_ctx->data; >> + frames_hwctx = frames_ctx->hwctx; >> >> - err = av_hwframe_ctx_init(encode_frames_ref); >> - if (err < 0) >> - goto fail; >> + frames_ctx->width = FFALIGN(ist->resample_width, 32); >> + frames_ctx->height = FFALIGN(ist->resample_height, 32); >> + frames_ctx->format = AV_PIX_FMT_QSV; >> + frames_ctx->sw_format = AV_PIX_FMT_NV12; >> + frames_ctx->initial_pool_size = 64; >> + frames_hwctx->frame_type = MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET; > Why are you creating another frames_ctx here? > >> >> - ist->dec_ctx->pix_fmt = AV_PIX_FMT_QSV; >> - ist->resample_pix_fmt = AV_PIX_FMT_QSV; >> + err = av_hwframe_ctx_init(ist->hw_frames_ctx); >> + if (err < 0) { >> + av_buffer_unref(&ist->hw_frames_ctx); >> + av_log(NULL, AV_LOG_ERROR, "Error initializing a QSV frame pool\n"); >> + return err; >> + } >> >> - ost->enc_ctx->pix_fmt = AV_PIX_FMT_QSV; >> - ost->enc_ctx->hw_frames_ctx = encode_frames_ref; >> + ost->enc_ctx->pix_fmt = AV_PIX_FMT_QSV; > enc_ctx should not be accessed. As you said yourself, filters decide > the encoder input anyway. > >> >> return 0; >> - >> -fail: >> - av_buffer_unref(&encode_frames_ref); >> - return err; >> } > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff --git a/ffmpeg_qsv.c b/ffmpeg_qsv.c index 86824b6..8cedb7e 100644 --- a/ffmpeg_qsv.c +++ b/ffmpeg_qsv.c @@ -81,25 +81,26 @@ int qsv_init(AVCodecContext *s) return ret; } - av_buffer_unref(&ist->hw_frames_ctx); - ist->hw_frames_ctx = av_hwframe_ctx_alloc(hw_device_ctx); - if (!ist->hw_frames_ctx) - return AVERROR(ENOMEM); - - frames_ctx = (AVHWFramesContext*)ist->hw_frames_ctx->data; - frames_hwctx = frames_ctx->hwctx; - - frames_ctx->width = FFALIGN(s->coded_width, 32); - frames_ctx->height = FFALIGN(s->coded_height, 32); - frames_ctx->format = AV_PIX_FMT_QSV; - frames_ctx->sw_format = s->sw_pix_fmt; - frames_ctx->initial_pool_size = 64; - frames_hwctx->frame_type = MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET; - - ret = av_hwframe_ctx_init(ist->hw_frames_ctx); - if (ret < 0) { - av_log(NULL, AV_LOG_ERROR, "Error initializing a QSV frame pool\n"); - return ret; + if(!ist->hw_frames_ctx) { + ist->hw_frames_ctx = av_hwframe_ctx_alloc(hw_device_ctx); + if (!ist->hw_frames_ctx) + return AVERROR(ENOMEM); + + frames_ctx = (AVHWFramesContext*)ist->hw_frames_ctx->data; + frames_hwctx = frames_ctx->hwctx; + + frames_ctx->width = FFALIGN(s->coded_width, 32); + frames_ctx->height = FFALIGN(s->coded_height, 32); + frames_ctx->format = AV_PIX_FMT_QSV; + frames_ctx->sw_format = s->sw_pix_fmt; + frames_ctx->initial_pool_size = 64; + frames_hwctx->frame_type = MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET; + + ret = av_hwframe_ctx_init(ist->hw_frames_ctx); + if (ret < 0) { + av_log(NULL, AV_LOG_ERROR, "Error initializing a QSV frame pool\n"); + return ret; + } } ist->hwaccel_get_buffer = qsv_get_buffer; @@ -114,9 +115,8 @@ int qsv_transcode_init(OutputStream *ost) const enum AVPixelFormat *pix_fmt; int err, i; - AVBufferRef *encode_frames_ref = NULL; - AVHWFramesContext *encode_frames; - AVQSVFramesContext *qsv_frames; + AVHWFramesContext *frames_ctx; + AVQSVFramesContext *frames_hwctx; /* check if the encoder supports QSV */ if (!ost->enc->pix_fmts) @@ -150,42 +150,33 @@ int qsv_transcode_init(OutputStream *ost) if (!hw_device_ctx) { err = qsv_device_init(ist); if (err < 0) - goto fail; - } - - // This creates a dummy hw_frames_ctx for the encoder to be - // suitably initialised. It only contains one real frame, so - // hopefully doesn't waste too much memory. - - encode_frames_ref = av_hwframe_ctx_alloc(hw_device_ctx); - if (!encode_frames_ref) { - err = AVERROR(ENOMEM); - goto fail; + return err; } - encode_frames = (AVHWFramesContext*)encode_frames_ref->data; - qsv_frames = encode_frames->hwctx; - encode_frames->width = FFALIGN(ist->resample_width, 32); - encode_frames->height = FFALIGN(ist->resample_height, 32); - encode_frames->format = AV_PIX_FMT_QSV; - encode_frames->sw_format = AV_PIX_FMT_NV12; - encode_frames->initial_pool_size = 1; + ist->dec_ctx->pix_fmt = AV_PIX_FMT_QSV; + ist->resample_pix_fmt = AV_PIX_FMT_QSV; + ist->hw_frames_ctx = av_hwframe_ctx_alloc(hw_device_ctx); + if (!ist->hw_frames_ctx) + return AVERROR(ENOMEM); - qsv_frames->frame_type = MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET; + frames_ctx = (AVHWFramesContext*)ist->hw_frames_ctx->data; + frames_hwctx = frames_ctx->hwctx; - err = av_hwframe_ctx_init(encode_frames_ref); - if (err < 0) - goto fail; + frames_ctx->width = FFALIGN(ist->resample_width, 32); + frames_ctx->height = FFALIGN(ist->resample_height, 32); + frames_ctx->format = AV_PIX_FMT_QSV; + frames_ctx->sw_format = AV_PIX_FMT_NV12; + frames_ctx->initial_pool_size = 64; + frames_hwctx->frame_type = MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET; - ist->dec_ctx->pix_fmt = AV_PIX_FMT_QSV; - ist->resample_pix_fmt = AV_PIX_FMT_QSV; + err = av_hwframe_ctx_init(ist->hw_frames_ctx); + if (err < 0) { + av_buffer_unref(&ist->hw_frames_ctx); + av_log(NULL, AV_LOG_ERROR, "Error initializing a QSV frame pool\n"); + return err; + } - ost->enc_ctx->pix_fmt = AV_PIX_FMT_QSV; - ost->enc_ctx->hw_frames_ctx = encode_frames_ref; + ost->enc_ctx->pix_fmt = AV_PIX_FMT_QSV; return 0; - -fail: - av_buffer_unref(&encode_frames_ref); - return err; }