[FFmpeg-devel] [PATCH 3/4] libavcodec/qsvdec.c: The ff_qsv_decode() now guarantees the consumption of whole packet.

[Michael Niedermayer]

On Thu, Jul 23, 2015 at 12:46:47PM +0300, Ivan Uskov wrote:
> Hello All ,
> 
> There is updated version of patch which should be applied without conflicts.
> Also here couple wrong places have been corrected into ff_qsv_decode().
> 
> Tuesday, July 21, 2015, 4:08:47 PM, you wrote:
> 
> IU> Hello all,
> 
> IU> Actual implementation of ff_qsv_decode() is not reliable, it may
> IU> return without consumption of 1..3 last bytes of packet which
> IU> initiates infinitive loop. New implementation guarantees that packet
> IU> will consumed, now internal fifo uses to join unconsumed previous packet
> IU> tail with new packet.
> IU> Please review.
> IU>   
> 
> 
> 
> 
> -- 
> Best regards,
>  Ivan                            mailto:ivan.uskov at nablet.com

>  qsvdec.c |  130 +++++++++++++++++++++++++++++++++++++++++++++++----------------
>  qsvdec.h |    1 
>  2 files changed, 100 insertions(+), 31 deletions(-)
> eb551cc0e8ef21a3557cebb850b5a15ce385e30e  0002-libavcodec-qsvdec.c-The-ff_qsv_decode-now-guarantees.patch
> From 1b1d3cae1910eb1a4dcea3ddee31a1b2a42292f5 Mon Sep 17 00:00:00 2001
> From: Ivan Uskov <ivan.uskov at nablet.com>
> Date: Tue, 21 Jul 2015 08:31:14 -0400
> Subject: [PATCH 2/2] libavcodec/qsvdec.c: The ff_qsv_decode() now guarantees
>  the consumption of whole packet.
> 
> ---
>  libavcodec/qsvdec.c | 130 +++++++++++++++++++++++++++++++++++++++-------------
>  libavcodec/qsvdec.h |   1 +
>  2 files changed, 100 insertions(+), 31 deletions(-)
> 
> diff --git a/libavcodec/qsvdec.c b/libavcodec/qsvdec.c
> index 4e7a0ac..b81781b 100644
> --- a/libavcodec/qsvdec.c
> +++ b/libavcodec/qsvdec.c
> @@ -126,6 +126,10 @@ int ff_qsv_decode_init(AVCodecContext *avctx, QSVContext *q, AVPacket *avpkt)
>      if (!q->async_fifo)
>          return AVERROR(ENOMEM);
>  
> +    q->input_fifo = av_fifo_alloc(1024*16);
> +    if (!q->input_fifo)
> +        return AVERROR(ENOMEM);
> +
>      q->engine_ready = 1;
>  
>      return 0;
> @@ -223,6 +227,31 @@ static QSVFrame *find_frame(QSVContext *q, mfxFrameSurface1 *surf)
>      return NULL;
>  }
>  
> +static void qsv_fifo_relocate(AVFifoBuffer *f, int bytes_to_free)
> +{
> +    int data_size;
> +    int data_rest = 0;
> +
> +    av_fifo_drain(f, bytes_to_free);
> +
> +    data_size = av_fifo_size(f);
> +    if (data_size > 0) {
> +        if (f->buffer!=f->rptr) {
> +            if ( (f->end - f->rptr) < data_size) {
> +                data_rest = data_size - (f->end - f->rptr);
> +                data_size-=data_rest;
> +                memmove(f->buffer+data_size, f->buffer, data_rest);
> +            }
> +            memmove(f->buffer, f->rptr, data_size);
> +            data_size+= data_rest;
> +        }
> +    }
> +    f->rptr = f->buffer;
> +    f->wptr = f->buffer + data_size;
> +    f->wndx = data_size;
> +    f->rndx = 0;
> +}
> +
>  int ff_qsv_decode(AVCodecContext *avctx, QSVContext *q,
>                    AVFrame *frame, int *got_frame,
>                    AVPacket *avpkt)

> @@ -233,55 +262,91 @@ int ff_qsv_decode(AVCodecContext *avctx, QSVContext *q,
>      mfxSyncPoint sync;
>      mfxBitstream bs = { { { 0 } } };
>      int ret;
> +    int n_out_frames;
> +    int buffered = 0;
>  
>      if (!q->engine_ready) {
>          ret = ff_qsv_decode_init(avctx, q, avpkt);
>          if (ret)
>              return ret;
>      }
> -    if (avpkt->size) {
> -        bs.Data       = avpkt->data;
> -        bs.DataLength = avpkt->size;
> +
> +    if (avpkt->size ) {
> +        if (av_fifo_size(q->input_fifo)) {
> +            /* we have got rest of previous packet into buffer */
> +            if (av_fifo_space(q->input_fifo) < avpkt->size) {
> +                ret = av_fifo_grow(q->input_fifo, avpkt->size);
> +                if (ret < 0)
> +                    return ret;
> +            }
> +            av_fifo_generic_write(q->input_fifo, avpkt->data, avpkt->size, NULL);
> +            bs.Data       = q->input_fifo->rptr;
> +            bs.DataLength = av_fifo_size(q->input_fifo);
> +            buffered = 1;
> +        } else {
> +            bs.Data       = avpkt->data;
> +            bs.DataLength = avpkt->size;
> +        }

Does this mean that each packet will be memcpy-ed ?
this would slow things down

also, is there a performance difference between AVParser and using
the external parser ?


>          bs.MaxLength  = bs.DataLength;
>          bs.TimeStamp  = avpkt->pts;
>      }
>  
> -    do {
> +    while (1) {
>          ret = get_surface(avctx, q, &insurf);
>          if (ret < 0)
>              return ret;
> +        do {
> +            ret = MFXVideoDECODE_DecodeFrameAsync(q->session, avpkt->size ? &bs : NULL,
> +                                                  insurf, &outsurf, &sync);
> +            if (ret != MFX_WRN_DEVICE_BUSY)
> +                break;
> -        ret = MFXVideoDECODE_DecodeFrameAsync(q->session, avpkt->size ? &bs : NULL,
> -                                              insurf, &outsurf, &sync);
> -        if (ret == MFX_WRN_DEVICE_BUSY)

> -            av_usleep(1);
> +            av_usleep(500);

looks like a unrelated change,
should be in a seperate patch with explanation

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150723/20ba701e/attachment.sig>