[libav-devel] [PATCH] avcodec/qsv: fix async support

Rogozhkin, Dmitry V dmitry.v.rogozhkin at intel.com
Thu Jul 26 21:13:31 CEST 2018


On Thu, 2018-07-26 at 09:44 +0200, Maxym Dmytrychenko wrote:


On Thu, Jul 26, 2018 at 7:55 AM Li, Zhong <zhong.li at intel.com<mailto:zhong.li at intel.com>> wrote:
> -----Original Message-----
> From: Rogozhkin, Dmitry V
> Sent: Wednesday, July 25, 2018 1:36 AM
> To: libav-devel at libav.org<mailto:libav-devel at libav.org>
> Cc: Rogozhkin, Dmitry V <dmitry.v.rogozhkin at intel.com<mailto:dmitry.v.rogozhkin at intel.com>>; Maxym
> Dmytrychenko <maxim.d33 at gmail.com<mailto:maxim.d33 at gmail.com>>; Li, Zhong <zhong.li at intel.com<mailto:zhong.li at intel.com>>
> Subject: [PATCH] avcodec/qsv: fix async support
>
> Current implementations of qsv components incorrectly work with async
> level, they actually try to work in async+1 level stepping into
> MFX_WRN_DEVICE_BUSY and polling loop. This change address this
> misbehaviour.
>
> Signed-off-by: Dmitry Rogozhkin <dmitry.v.rogozhkin at intel.com<mailto:dmitry.v.rogozhkin at intel.com>>
> Cc: Maxym Dmytrychenko <maxim.d33 at gmail.com<mailto:maxim.d33 at gmail.com>>
> Cc: Zhong Li <zhong.li at intel.com<mailto:zhong.li at intel.com>>
> ---
>  libavcodec/qsvdec.c | 15 ++++++++++++---  libavcodec/qsvenc.c | 17
> +++++++++++++----
>  2 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/libavcodec/qsvdec.c b/libavcodec/qsvdec.c index
> 32f1fe7..b9707f7 100644
> --- a/libavcodec/qsvdec.c
> +++ b/libavcodec/qsvdec.c
> @@ -110,6 +110,16 @@ static int qsv_init_session(AVCodecContext *avctx,
> QSVContext *q, mfxSession ses
>      return 0;
>  }
>
> +static inline unsigned int qsv_fifo_item_size(void) {
> +    return sizeof(mfxSyncPoint*) + sizeof(QSVFrame*); }
> +
> +static inline unsigned int qsv_fifo_size(const AVFifoBuffer* fifo) {
> +    return av_fifo_size(fifo)/qsv_fifo_item_size();
> +}
> +
>  static int qsv_decode_init(AVCodecContext *avctx, QSVContext *q)  {
>      const AVPixFmtDescriptor *desc;
> @@ -125,8 +135,7 @@ static int qsv_decode_init(AVCodecContext *avctx,
> QSVContext *q)
>          return AVERROR_BUG;
>
>      if (!q->async_fifo) {
> -        q->async_fifo = av_fifo_alloc((1 + q->async_depth) *
> -                                      (sizeof(mfxSyncPoint*) +
> sizeof(QSVFrame*)));
> +        q->async_fifo = av_fifo_alloc(q->async_depth *
> + qsv_fifo_item_size());
>          if (!q->async_fifo)
>              return AVERROR(ENOMEM);
>      }
> @@ -384,7 +393,7 @@ static int qsv_decode(AVCodecContext *avctx,
> QSVContext *q,
>          av_freep(&sync);
>      }
>
> -    if (!av_fifo_space(q->async_fifo) ||
> +    if ((qsv_fifo_size(q->async_fifo) >= q->async_depth) ||
>          (!avpkt->size && av_fifo_size(q->async_fifo))) {
>          AVFrame *src_frame;
>
> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index
> 3ce5ffe..40ddb34 100644
> --- a/libavcodec/qsvenc.c
> +++ b/libavcodec/qsvenc.c
> @@ -777,7 +777,7 @@ static int qsv_init_opaque_alloc(AVCodecContext
> *avctx, QSVEncContext *q)
>      mfxFrameSurface1 *surfaces;
>      int nb_surfaces, i;
>
> -    nb_surfaces = qsv->nb_opaque_surfaces +
> q->req.NumFrameSuggested + q->async_depth;
> +    nb_surfaces = qsv->nb_opaque_surfaces +
> q->req.NumFrameSuggested;
>
>      q->opaque_alloc_buf = av_buffer_allocz(sizeof(*surfaces) *
> nb_surfaces);
>      if (!q->opaque_alloc_buf)
> @@ -848,6 +848,16 @@ static int qsvenc_init_session(AVCodecContext
> *avctx, QSVEncContext *q)
>      return 0;
>  }
>
> +static inline unsigned int qsv_fifo_item_size(void) {
> +    return sizeof(AVPacket) + sizeof(mfxSyncPoint*) +
> +sizeof(mfxBitstream*); }
> +
> +static inline unsigned int qsv_fifo_size(const AVFifoBuffer* fifo) {
> +    return av_fifo_size(fifo)/qsv_fifo_item_size();
> +}
> +
Add a blank before and after "/" is a unified coding style.
Maybe better to move it to qsv.c since it is common for qsvdec/enc.


good point - agree.


I uploaded v2 of the patch. I added spaces, but I did not move the qsv_fifo_size to the common place. Indeed it is the same between dec/enc, but qsv_fifo_item_size is not the same (sizeof queue objects are different). Thus, I can move only qsv_fifo_size and rely on the symbol resolution that qsv_fifo_item_size will be found. I don't think this is a good idea. So, I left as is, but fixed white space.

>  int ff_qsv_enc_init(AVCodecContext *avctx, QSVEncContext *q)  {
>      int iopattern = 0;
> @@ -856,8 +866,7 @@ int ff_qsv_enc_init(AVCodecContext *avctx,
> QSVEncContext *q)
>
>      q->param.AsyncDepth = q->async_depth;
>
> -    q->async_fifo = av_fifo_alloc((1 + q->async_depth) *
> -                                  (sizeof(AVPacket) +
> sizeof(mfxSyncPoint*) + sizeof(mfxBitstream*)));
> +    q->async_fifo = av_fifo_alloc(q->async_depth *
> + qsv_fifo_item_size());

I agree with remove the "+1". And noted someone was also confused too: http://ffmpeg.org/pipermail/ffmpeg-devel/2017-November/219643.html
Any historic reason to add "+1" in the initial implementation? If no, I would like to see this patch applied.


just a minor reason.

BTW, currently the option "async_depth" range is 0 ~ INT_MAX. Need to change it to 1 ~ INT_MAX now, no?


yes, will adjust this as well

I adjusted the range. Actually this can be handled other way round. If you pass AsyncDepth=0 to the mediasdk it will correct this parameter and use an internal default (I think =5). Application can query this value with either Query or GetVideoParam after component Init. However, I don't think this is a good idea to have 2 level of defaults, one on ffmpeg level, another on MediaSDK level. Thus, I suggest to consider async_depth>=1 and have ffmpeg level default (=4 at the moment).



>      if (!q->async_fifo)
>          return AVERROR(ENOMEM);
>
> @@ -1214,7 +1223,7 @@ int ff_qsv_encode(AVCodecContext *avctx,
> QSVEncContext *q,
>      if (ret < 0)
>          return ret;
>
> -    if (!av_fifo_space(q->async_fifo) ||
> +    if ((qsv_fifo_size(q->async_fifo) >= q->async_depth) ||
>          (!frame && av_fifo_size(q->async_fifo))) {
>          AVPacket new_pkt;
>          mfxBitstream *bs;
> --
> 1.8.3.1




More information about the libav-devel mailing list