| Message ID | 1570627691-9464-2-git-send-email-dmitry.v.rogozhkin@intel.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 2019-10-09 at 06:28 -0700, Dmitry Rogozhkin wrote: > From: Andrey Orlov <andrey.orlov@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);
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Dmitry Rogozhkin > Sent: Wednesday, October 9, 2019 21:28 > To: ffmpeg-devel@ffmpeg.org > Cc: Orlov, Andrey <andrey.orlov@intel.com> > Subject: [FFmpeg-devel] [PATCH] avcodec/qsv: polling free synchronization > > From: Andrey Orlov <andrey.orlov@intel.com> > > synchronization by sync point after DEVICE_BUSY > > Fixes: CPU usage on AVC decode cases (18% -> 9%) > --- > 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(-) > --- 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); Since av_usleep is removed, "libavutil/time.h" is redundant and should be removed as well. > + ff_qsv_handle_device_busy(&q->session, &q->last_dec_sync, &ret, > 500); If the value of sleep is hardcode to 500, is it still necessary to pass it as a parameter? > > } 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; > } > > 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); Same here. - linjie
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) +{ + int sync_ret; + + if (*sync) { + sync_ret = MFXVideoCORE_SyncOperation(*session, *sync, MFX_INFINITE); + if (sync_ret == MFX_ERR_NONE) { + *sync = NULL; + } else { + *ret = MFX_ERR_ABORTED; + } + } 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; } 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; 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);
From: Andrey Orlov <andrey.orlov@intel.com> synchronization by sync point after DEVICE_BUSY Fixes: CPU usage on AVC decode cases (18% -> 9%) --- 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(-)