[FFmpeg-devel] lavc/vaapi_encode: grow packet if vaMapBuffer returns multiple buffers

Submitted by Linjie Fu on May 29, 2019, 8:57 p.m.

Details

Message ID 1559163423-22745-1-git-send-email-linjie.fu@intel.com
State New
Headers show

Commit Message

Linjie Fu May 29, 2019, 8:57 p.m.
It seems that VA_CODED_BUF_STATUS_SINGLE_NALU allows driver to map
buffer for each slice.

Currently, assigning new buffer for pkt when multiple buffer returns
from vaMapBuffer will cover the previous encoded pkt data and lead
to encode issues.

Using av_grow_packet to expand pkt if several buffers are returned.

Signed-off-by: Linjie Fu <linjie.fu@intel.com>
---
 libavcodec/vaapi_encode.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Mark Thompson May 29, 2019, 11:17 p.m.
On 29/05/2019 21:57, Linjie Fu wrote:
> It seems that VA_CODED_BUF_STATUS_SINGLE_NALU allows driver to map
> buffer for each slice.
> 
> Currently, assigning new buffer for pkt when multiple buffer returns
> from vaMapBuffer will cover the previous encoded pkt data and lead
> to encode issues.
> 
> Using av_grow_packet to expand pkt if several buffers are returned.
> 
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
>  libavcodec/vaapi_encode.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)

What driver uses this?

> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> index 2dda451..2812237 100644
> --- a/libavcodec/vaapi_encode.c
> +++ b/libavcodec/vaapi_encode.c
> @@ -490,6 +490,7 @@ static int vaapi_encode_output(AVCodecContext *avctx,
>      VACodedBufferSegment *buf_list, *buf;
>      VAStatus vas;
>      int err;
> +    uint8_t *ptr;
>  
>      err = vaapi_encode_wait(avctx, pic);
>      if (err < 0)
> @@ -509,11 +510,18 @@ static int vaapi_encode_output(AVCodecContext *avctx,
>          av_log(avctx, AV_LOG_DEBUG, "Output buffer: %u bytes "
>                 "(status %08x).\n", buf->size, buf->status);
>  
> -        err = av_new_packet(pkt, buf->size);
> +        if (pkt->size)
> +            err = av_grow_packet(pkt, buf->size);

av_grow_packet() can reallocate the buffer, which will change its address.

To avoid repeated new allocations and copies, perhaps it would be better to iterate through the list first to find out how large the single packet needs to be?

> +        else {
> +            err = av_new_packet(pkt, buf->size);
> +            ptr = pkt->data;
> +        }
> +
>          if (err < 0)
>              goto fail_mapped;
>  
> -        memcpy(pkt->data, buf->buf, buf->size);
> +        memcpy(ptr, buf->buf, buf->size);
> +        ptr += buf->size;
>      }
>  
>      if (pic->type == PICTURE_TYPE_IDR)
> 

- Mark
Linjie Fu May 30, 2019, 1:40 a.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Mark Thompson

> Sent: Thursday, May 30, 2019 07:17

> To: ffmpeg-devel@ffmpeg.org

> Subject: Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode: grow packet if

> vaMapBuffer returns multiple buffers

> 

> On 29/05/2019 21:57, Linjie Fu wrote:

> > It seems that VA_CODED_BUF_STATUS_SINGLE_NALU allows driver to

> map

> > buffer for each slice.

> >

> > Currently, assigning new buffer for pkt when multiple buffer returns

> > from vaMapBuffer will cover the previous encoded pkt data and lead

> > to encode issues.

> >

> > Using av_grow_packet to expand pkt if several buffers are returned.

> >

> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>

> > ---

> >  libavcodec/vaapi_encode.c | 12 ++++++++++--

> >  1 file changed, 10 insertions(+), 2 deletions(-)

> 

> What driver uses this?


It was found while enabling VP9 VDENC on ICL with iHD.
vaMapBuffer returns two bufs in the buf_list including a zero-sized buf.

IMHO, it should be fixed in driver level, but I guess ffmpeg should be able to 
cope with the multiple buf situation as well.

https://github.com/intel/media-driver/issues/624
> 

> > diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c

> > index 2dda451..2812237 100644

> > --- a/libavcodec/vaapi_encode.c

> > +++ b/libavcodec/vaapi_encode.c

> > @@ -490,6 +490,7 @@ static int vaapi_encode_output(AVCodecContext

> *avctx,

> >      VACodedBufferSegment *buf_list, *buf;

> >      VAStatus vas;

> >      int err;

> > +    uint8_t *ptr;

> >

> >      err = vaapi_encode_wait(avctx, pic);

> >      if (err < 0)

> > @@ -509,11 +510,18 @@ static int vaapi_encode_output(AVCodecContext

> *avctx,

> >          av_log(avctx, AV_LOG_DEBUG, "Output buffer: %u bytes "

> >                 "(status %08x).\n", buf->size, buf->status);

> >

> > -        err = av_new_packet(pkt, buf->size);

> > +        if (pkt->size)

> > +            err = av_grow_packet(pkt, buf->size);

> 

> av_grow_packet() can reallocate the buffer, which will change its address.

> 

> To avoid repeated new allocations and copies, perhaps it would be better to

> iterate through the list first to find out how large the single packet needs to

> be?


Avoiding repeated memcpy and reallocation sounds better to me.
Thanks Mark.

- Linjie

Patch hide | download patch | download mbox

diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
index 2dda451..2812237 100644
--- a/libavcodec/vaapi_encode.c
+++ b/libavcodec/vaapi_encode.c
@@ -490,6 +490,7 @@  static int vaapi_encode_output(AVCodecContext *avctx,
     VACodedBufferSegment *buf_list, *buf;
     VAStatus vas;
     int err;
+    uint8_t *ptr;
 
     err = vaapi_encode_wait(avctx, pic);
     if (err < 0)
@@ -509,11 +510,18 @@  static int vaapi_encode_output(AVCodecContext *avctx,
         av_log(avctx, AV_LOG_DEBUG, "Output buffer: %u bytes "
                "(status %08x).\n", buf->size, buf->status);
 
-        err = av_new_packet(pkt, buf->size);
+        if (pkt->size)
+            err = av_grow_packet(pkt, buf->size);
+        else {
+            err = av_new_packet(pkt, buf->size);
+            ptr = pkt->data;
+        }
+
         if (err < 0)
             goto fail_mapped;
 
-        memcpy(pkt->data, buf->buf, buf->size);
+        memcpy(ptr, buf->buf, buf->size);
+        ptr += buf->size;
     }
 
     if (pic->type == PICTURE_TYPE_IDR)