[FFmpeg-devel] [PATCH] avcodec/qsv: polling free synchronization
Rogozhkin, Dmitry V
dmitry.v.rogozhkin at intel.com
Thu Oct 10 00:45:13 EEST 2019
On Wed, 2019-10-09 at 06:28 -0700, Dmitry Rogozhkin wrote:
> From: Andrey Orlov <andrey.orlov at intel.com>
>
> synchronization by sync point after DEVICE_BUSY
>
> Fixes: CPU usage on AVC decode cases (18% -> 9%)
I would expect a link or reference to the bug following "Fixes:". If
you don't have it - just a text what is getting fixed. As Zhong pointed
out it would be nice to clarify which codecs other than AVC are
actually affected.
In the code below I see your change affecting decoding and encoding.
What about VP? Should similar change be done there (if yes - in
separate patch)?
> ---
> libavcodec/qsv.c | 17 +++++++++++++++++
> libavcodec/qsv_internal.h | 2 ++
> libavcodec/qsvdec.c | 12 ++++++++----
> libavcodec/qsvdec.h | 2 ++
> libavcodec/qsvenc.c | 13 +++++++++----
> libavcodec/qsvenc.h | 2 ++
> 6 files changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c
> index b00e427..4b5018b 100644
> --- a/libavcodec/qsv.c
> +++ b/libavcodec/qsv.c
> @@ -32,6 +32,7 @@
> #include "libavutil/hwcontext_qsv.h"
> #include "libavutil/imgutils.h"
> #include "libavutil/avassert.h"
> +#include "libavutil/time.h"
>
> #include "avcodec.h"
> #include "qsv_internal.h"
> @@ -852,3 +853,19 @@ int ff_qsv_close_internal_session(QSVSession
> *qs)
> #endif
> return 0;
> }
> +
> +void ff_qsv_handle_device_busy(mfxSession *session, mfxSyncPoint
> *sync, mfxStatus *ret, unsigned sleep)
Why pass return value in parameter when you have a legacy way? I think
function prototype should be:
int ff_qsv_handle_device_busy(mfxSession *session, mfxSyncPoint *sync,
unsigned int timeout)
> +{
> + int sync_ret;
Just 'ret', I think.
> +
> + if (*sync) {
> + sync_ret = MFXVideoCORE_SyncOperation(*session, *sync,
> MFX_INFINITE);
> + if (sync_ret == MFX_ERR_NONE) {
> + *sync = NULL;
> + } else {
> + *ret = MFX_ERR_ABORTED;
This masks real error which you get from the call above - just return
it directly.
> + }
> + } else {
> + av_usleep(sleep);
> + }
> +}
> diff --git a/libavcodec/qsv_internal.h b/libavcodec/qsv_internal.h
> index 3755927..01c98cc 100644
> --- a/libavcodec/qsv_internal.h
> +++ b/libavcodec/qsv_internal.h
> @@ -141,4 +141,6 @@ int ff_qsv_init_session_frames(AVCodecContext
> *avctx, mfxSession *session,
>
> int ff_qsv_find_surface_idx(QSVFramesContext *ctx, QSVFrame *frame);
>
> +void ff_qsv_handle_device_busy(mfxSession *session, mfxSyncPoint
> *sync, mfxStatus *ret, unsigned sleep);
> +
> #endif /* AVCODEC_QSV_INTERNAL_H */
> diff --git a/libavcodec/qsvdec.c b/libavcodec/qsvdec.c
> index ae50239..aa83272 100644
> --- a/libavcodec/qsvdec.c
> +++ b/libavcodec/qsvdec.c
> @@ -457,8 +457,12 @@ static int qsv_decode(AVCodecContext *avctx,
> QSVContext *q,
>
> ret = MFXVideoDECODE_DecodeFrameAsync(q->session, avpkt-
> >size ? &bs : NULL,
> insurf, &outsurf,
> sync);
> +
> + if (ret == MFX_ERR_NONE)
> + q->last_dec_sync = *sync;
> +
> if (ret == MFX_WRN_DEVICE_BUSY)
> - av_usleep(500);
> + ff_qsv_handle_device_busy(&q->session, &q-
> >last_dec_sync, &ret, 500);
>
> } while (ret == MFX_WRN_DEVICE_BUSY || ret ==
> MFX_ERR_MORE_SURFACE);
>
> @@ -510,9 +514,9 @@ static int qsv_decode(AVCodecContext *avctx,
> QSVContext *q,
> out_frame->queued = 0;
>
> if (avctx->pix_fmt != AV_PIX_FMT_QSV) {
> - do {
> - ret = MFXVideoCORE_SyncOperation(q->session, *sync,
> 1000);
> - } while (ret == MFX_WRN_IN_EXECUTION);
> + ret = MFXVideoCORE_SyncOperation(q->session, *sync,
> MFX_INFINITE);
> + if (ret < 0)
> + return ret;
This change is not the primary goal of the patch, so I suggest to put
it in a separate patch if you believe it is needed. From my perspective
you change is LGTM.
> }
>
> av_freep(&sync);
> diff --git a/libavcodec/qsvdec.h b/libavcodec/qsvdec.h
> index dec1f61..d27ea68 100644
> --- a/libavcodec/qsvdec.h
> +++ b/libavcodec/qsvdec.h
> @@ -72,6 +72,8 @@ typedef struct QSVContext {
>
> mfxExtBuffer **ext_buffers;
> int nb_ext_buffers;
> +
> + mfxSyncPoint last_dec_sync;
> } QSVContext;
>
> extern const AVCodecHWConfigInternal *ff_qsv_hw_configs[];
> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> index ba85d64..bd5dd75 100644
> --- a/libavcodec/qsvenc.c
> +++ b/libavcodec/qsvenc.c
> @@ -1368,8 +1368,13 @@ static int encode_frame(AVCodecContext *avctx,
> QSVEncContext *q,
>
> do {
> ret = MFXVideoENCODE_EncodeFrameAsync(q->session, enc_ctrl,
> surf, bs, sync);
> +
> + if (ret == MFX_ERR_NONE)
> + q->last_enc_sync = *sync;
> +
> if (ret == MFX_WRN_DEVICE_BUSY)
> - av_usleep(500);
> + ff_qsv_handle_device_busy(&q->session, &q-
> >last_enc_sync, &ret, 500);
> +
> } while (ret == MFX_WRN_DEVICE_BUSY || ret ==
> MFX_WRN_IN_EXECUTION);
>
> if (ret > 0)
> @@ -1435,9 +1440,9 @@ int ff_qsv_encode(AVCodecContext *avctx,
> QSVEncContext *q,
> av_fifo_generic_read(q->async_fifo,
> &sync, sizeof(sync), NULL);
> av_fifo_generic_read(q->async_fifo,
> &bs, sizeof(bs), NULL);
>
> - do {
> - ret = MFXVideoCORE_SyncOperation(q->session, *sync,
> 1000);
> - } while (ret == MFX_WRN_IN_EXECUTION);
> + ret = MFXVideoCORE_SyncOperation(q->session, *sync,
> MFX_INFINITE);
> + if (ret < 0)
> + return ret;
Same as above - separate patch.
>
> new_pkt.dts = av_rescale_q(bs->DecodeTimeStamp,
> (AVRational){1, 90000}, avctx->time_base);
> new_pkt.pts = av_rescale_q(bs-
> >TimeStamp, (AVRational){1, 90000}, avctx->time_base);
> diff --git a/libavcodec/qsvenc.h b/libavcodec/qsvenc.h
> index ec8b541..f1c22d7 100644
> --- a/libavcodec/qsvenc.h
> +++ b/libavcodec/qsvenc.h
> @@ -185,6 +185,8 @@ typedef struct QSVEncContext {
> char *load_plugins;
> SetEncodeCtrlCB *set_encode_ctrl_cb;
> int forced_idr;
> +
> + mfxSyncPoint last_enc_sync;
> } QSVEncContext;
>
> int ff_qsv_enc_init(AVCodecContext *avctx, QSVEncContext *q);
More information about the ffmpeg-devel
mailing list