[FFmpeg-devel] [PATCH] ffmpeg_qsv.c: Init an hwframes_context for decoder instead of encoder
Huang, Zhengxu
zhengxu.maxwell at gmail.com
Sun Jan 22 04:22:32 EET 2017
在 2017/1/20 18:05, wm4 写道:
> On Fri, 20 Jan 2017 17:41:01 +0800
> "Huang, Zhengxu" <zhengxu.maxwell at gmail.com> wrote:
>
>> From 2149f87637ab941be14828f7ae2c224908784c7d Mon Sep 17 00:00:00 2001
>> From: Zhengxu <zhengxu.maxwell at 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 at gmail.com>
>> Signed-off-by: Huang, Zhengxu <zhengxu.maxwell at gmail.com>
>> Signed-off-by: Andrew, Zhang <huazh407 at 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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
More information about the ffmpeg-devel
mailing list