[FFmpeg-devel] [PATCH] lavc/vaapi_encode: Async the encoding and output procedure of encoder

[Mark Thompson]

On 07/11/2019 16:32, Linjie Fu wrote:
> Currently, vaapi encodes a pic if all its references are ready,
> and then outputs it immediately by calling vaapi_encode_output.
> 
> However, while working on output procedure, hardware is be able to
> cope with encoding tasks in the meantime to have the better performance.
> 
> So a more efficient way is to send all the pics with available refs to
> hardware to allow encoding while output.
> 
> It's what vaapi originally did before the regression, and the performance
> could be improved for ~20%.
> 
> CMD:
> ffmpeg -hwaccel vaapi -vaapi_device /dev/dri/renderD128
> -hwaccel_output_format vaapi -i bbb_sunflower_1080p_30fps_normal.mp4
> -c:v h264_vaapi -f h264 -y /dev/null
> 
> Source:
> https://download.blender.org/demo/movies/BBB/
> 
> Before:
>     ~164 fps
> After:
>     ~198 fps
> 
> Fix #7706.
> 
> Signed-off-by: Linjie Fu <linjie.fu at intel.com>
> ---
>  libavcodec/vaapi_encode.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> index 3be9159d37..aceb268315 100644
> --- a/libavcodec/vaapi_encode.c
> +++ b/libavcodec/vaapi_encode.c
> @@ -1109,17 +1109,28 @@ int ff_vaapi_encode_receive_packet(AVCodecContext *avctx, AVPacket *pkt)
>              return AVERROR(EAGAIN);
>      }
>  
> +pick_next:
>      pic = NULL;
>      err = vaapi_encode_pick_next(avctx, &pic);
> -    if (err < 0)
> -        return err;
> -    av_assert0(pic);
> +    if (!err) {
> +        av_assert0(pic);
>  
> -    pic->encode_order = ctx->encode_order++;
> +        pic->encode_order = ctx->encode_order++;
>  
> -    err = vaapi_encode_issue(avctx, pic);
> -    if (err < 0) {
> -        av_log(avctx, AV_LOG_ERROR, "Encode failed: %d.\n", err);
> +        err = vaapi_encode_issue(avctx, pic);
> +        if (err < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Encode failed: %d.\n", err);
> +            return err;
> +        }
> +        goto pick_next;
> +    } else if (err == AVERROR(EAGAIN)) {
> +        for (pic = ctx->pic_start; pic; pic = pic->next)
> +            if (pic->encode_issued && !pic->encode_complete &&
> +                pic->encode_order == ctx->output_order)
> +                break;
> +        if (!pic)
> +            return err;
> +    } else {
>          return err;
>      }
>  
> @@ -1143,7 +1154,7 @@ int ff_vaapi_encode_receive_packet(AVCodecContext *avctx, AVPacket *pkt)
>      av_log(avctx, AV_LOG_DEBUG, "Output packet: pts %"PRId64" dts %"PRId64".\n",
>             pkt->pts, pkt->dts);
>  
> -    ctx->output_order = pic->encode_order;
> +    ctx->output_order++;
>      vaapi_encode_clear_old(avctx);
>  
>      return 0;
> 

The sync in the same receive call here is required for correctness because of the how VAAPI syncs to the input surface (consider the interactions of a split followed by two encoders using the same surfaces).  I didn't realise that for a long time, hence the error existing in earlier versions.

Relatedly, this change would significantly increase latency on Intel platforms because vaEndPicture() is mostly synchronous there, and you're now calling it multiple times before returning anything.

More generally, though, the API definition here is just really stupid.  If you're interested it would be much better to fix this in the API - sync-to-output of some kind would make far more sense, as would some kind of event-based system (extra points if it can interop via fds to normal poll() and Vulkan).

- Mark