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

Maxym Dmytrychenko maxim.d33 at gmail.com
Thu Jul 26 09:44:45 CEST 2018


On Thu, Jul 26, 2018 at 7:55 AM Li, Zhong <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
> > Cc: Rogozhkin, Dmitry V <dmitry.v.rogozhkin at intel.com>; Maxym
> > Dmytrychenko <maxim.d33 at gmail.com>; Li, Zhong <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>
> > Cc: Maxym Dmytrychenko <maxim.d33 at gmail.com>
> > Cc: Zhong Li <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.


> >  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


>      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