[FFmpeg-devel,1/1] avcodec/vaapi_encode: add frame-skip func

frame-skip is required to implement network
bandwidth self-adaptive vaapi encoding.
To make a frame skipped, allocate its frame
side data of AV_FRAME_DATA_SKIP_FRAME type
and set its value to 1.

Signed-off-by: Jing SUN <jing.a.sun@intel.com>
---
 libavcodec/vaapi_encode.c | 142 ++++++++++++++++++++++++++++++++++++++++++++--
 libavcodec/vaapi_encode.h |   5 ++
 libavutil/frame.c         |   1 +
 libavutil/frame.h         |   5 ++
 4 files changed, 149 insertions(+), 4 deletions(-)

2018-11-19 10:04 GMT+01:00, Jing SUN <jing.a.sun@intel.com>:

> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 66f27f4..8ef6475 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -166,6 +166,11 @@ enum AVFrameSideDataType {
>       * function in libavutil/timecode.c.
>       */
>      AV_FRAME_DATA_S12M_TIMECODE,
> +
> +    /**
> +     * VAAPI Encode skip-frame indicator.
> +     */
> +    AV_FRAME_DATA_SKIP_FRAME,

I suspect this needs a  minor version bump and should be a
separate commit but please wait for an actual review.

Carl Eugen

On 19/11/18 09:04, Jing SUN wrote:
> frame-skip is required to implement network
> bandwidth self-adaptive vaapi encoding.
> To make a frame skipped, allocate its frame
> side data of AV_FRAME_DATA_SKIP_FRAME type
> and set its value to 1.

So if I'm reading this correctly the idea is to implement partial VFR by having a special new side-data type which indicates where frames would have been had the input actually matched the configured CFR behaviour?

Why is the user meant to create these special frames?  It seems to me that the existing method of looking at the timestamps would be a better way to find any gaps.

(Or, even better, add timestamps to VAAPI so that it can support VFR in a sensible way rather than adding hacks like this to allow partial VFR with weird constraints.)

> Signed-off-by: Jing SUN <jing.a.sun@intel.com>
> ---
>  libavcodec/vaapi_encode.c | 142 ++++++++++++++++++++++++++++++++++++++++++++--
>  libavcodec/vaapi_encode.h |   5 ++
>  libavutil/frame.c         |   1 +
>  libavutil/frame.h         |   5 ++
>  4 files changed, 149 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> index 2fe8501..a401d61 100644
> --- a/libavcodec/vaapi_encode.c
> +++ b/libavcodec/vaapi_encode.c
> @@ -23,6 +23,7 @@
>  #include "libavutil/common.h"
>  #include "libavutil/log.h"
>  #include "libavutil/pixdesc.h"
> +#include "libavutil/intreadwrite.h"
>  
>  #include "vaapi_encode.h"
>  #include "avcodec.h"
> @@ -103,6 +104,47 @@ static int vaapi_encode_make_param_buffer(AVCodecContext *avctx,
>      return 0;
>  }
>  
> +static int vaapi_encode_check_if_skip(AVCodecContext *avctx,
> +                                      VAAPIEncodePicture *pic)
> +{
> +    AVFrameSideData *fside = NULL;
> +    VAAPIEncodeContext *ctx = avctx->priv_data;
> +    VAAPIEncodePicture *cur = NULL;
> +    int i = 0;
> +
> +    if (!pic || !pic->input_image)
> +        return AVERROR(EINVAL);
> +
> +    fside = av_frame_get_side_data(pic->input_image, AV_FRAME_DATA_SKIP_FRAME);
> +    if (fside)
> +        pic->skipped_flag = AV_RL8(fside->data);
> +    else
> +        pic->skipped_flag = 0;
> +
> +    if (0 == pic->skipped_flag)
> +        return 0;
> +
> +    if ((pic->type == PICTURE_TYPE_IDR) || (pic->type == PICTURE_TYPE_I)) {
> +        av_log(avctx, AV_LOG_INFO, "Can't skip IDR/I pic %"PRId64"/%"PRId64".\n",
> +               pic->display_order, pic->encode_order);
> +        pic->skipped_flag = 0;
> +        return 0;
> +    }
> +
> +    for (cur = ctx->pic_start; cur; cur = cur->next) {
> +        for (i=0; i < cur->nb_refs; ++i) {
> +            if (cur->refs[i] == pic) {
> +                av_log(avctx, AV_LOG_INFO, "Can't skip ref pic %"PRId64"/%"PRId64".\n",
> +                       pic->display_order, pic->encode_order);
> +                pic->skipped_flag = 0;
> +                return 0;
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static int vaapi_encode_wait(AVCodecContext *avctx,
>                               VAAPIEncodePicture *pic)
>  {
> @@ -418,6 +460,69 @@ static int vaapi_encode_issue(AVCodecContext *avctx,
>          }
>      }
>  
> +    err = vaapi_encode_check_if_skip(avctx, pic);
> +    if (err != 0)
> +        av_log(avctx, AV_LOG_ERROR, "Fail to check if skip.\n");
> +
> +#ifdef VAEncMiscParameterSkipFrame
> +    if (pic->skipped_flag) {
> +        av_log(avctx, AV_LOG_INFO, "Skip pic %"PRId64"/%"PRId64" as requested.\n",
> +               pic->display_order, pic->encode_order);
> +
> +        ++ctx->skipped_pic_count;
> +        pic->encode_issued = 1;
> +
> +        return 0;
> +    } else if (ctx->skipped_pic_count > 0) {
> +        VABufferID skip_param_id;
> +        VAEncMiscParameterBuffer *misc_param;
> +        VAEncMiscParameterSkipFrame *skip_param;
> +
> +        err = vaapi_encode_make_param_buffer(avctx, pic,
> +                  VAEncMiscParameterBufferType, NULL,
> +                  (sizeof(VAEncMiscParameterBuffer) +
> +                  sizeof(VAEncMiscParameterSkipFrame)));
> +        if (err < 0)
> +            goto fail;
> +
> +        skip_param_id = pic->param_buffers[pic->nb_param_buffers-1];
> +
> +        vas = vaMapBuffer(ctx->hwctx->display,
> +                          skip_param_id,
> +                          (void **)&misc_param);
> +        if (vas != VA_STATUS_SUCCESS) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to map skip-frame buffer: "
> +                   "%d (%s).\n", vas, vaErrorStr(vas));
> +            err = AVERROR(EIO);
> +            goto fail;
> +        }
> +
> +        misc_param->type = (VAEncMiscParameterType)VAEncMiscParameterTypeSkipFrame;

You need to check VAConfigAttribEncSkipFrame to make sure this type is actually supported before using it.

> +        skip_param = (VAEncMiscParameterSkipFrame *)misc_param->data;
> +        skip_param->skip_frame_flag = 1;
> +        skip_param->num_skip_frames = ctx->skipped_pic_count;
> +        skip_param->size_skip_frames = 0;
> +
> +        vas = vaUnmapBuffer(ctx->hwctx->display, skip_param_id);
> +        if (vas != VA_STATUS_SUCCESS) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to unmap skip-frame buffer: "
> +                   "%d (%s).\n", vas, vaErrorStr(vas));
> +            err = AVERROR(EIO);
> +            goto fail;
> +        }

make_param_buffer can be called with the intended data directly - there is no need for this map/unmap.

> +
> +        ctx->skipped_pic_count = 0;
> +    }
> +#else
> +    if (pic->skipped_flag) {
> +        av_log(avctx, AV_LOG_INFO, "Skip-frame isn't supported and pic %"PRId64"/%"PRId64" isn't skipped.\n",
> +               pic->display_order, pic->encode_order);
> +
> +        pic->skipped_flag = 0;
> +        ctx->skipped_pic_count = 0;
> +    }
> +#endif
> +
>      vas = vaBeginPicture(ctx->hwctx->display, ctx->va_context,
>                           pic->input_surface);
>      if (vas != VA_STATUS_SUCCESS) {
> @@ -500,9 +605,28 @@ static int vaapi_encode_output(AVCodecContext *avctx,
>      VAStatus vas;
>      int err;
>  
> -    err = vaapi_encode_wait(avctx, pic);
> -    if (err < 0)
> -        return err;
> +    if (!pic->skipped_flag) {
> +        err = vaapi_encode_wait(avctx, pic);
> +        if (err < 0)
> +            return err;
> +    } else {
> +        av_frame_free(&pic->input_image);
> +        pic->encode_complete = 1;
> +
> +        err = av_new_packet(pkt, 0);
> +        if (err < 0)
> +            goto fail;
> +
> +        pkt->pts = pic->pts;
> +
> +        av_buffer_unref(&pic->output_buffer_ref);
> +        pic->output_buffer = VA_INVALID_ID;
> +
> +        av_log(avctx, AV_LOG_DEBUG, "Output 0 byte for pic %"PRId64"/%"PRId64".\n",
> +               pic->display_order, pic->encode_order);

Why is it helpful to create a zero-byte packet in this case?  Since no frame was encoded, I think no packet should be produced.

> +
> +        return 0;
> +    }
>  
>      buf_list = NULL;
>      vas = vaMapBuffer(ctx->hwctx->display, pic->output_buffer,
> @@ -523,6 +647,9 @@ static int vaapi_encode_output(AVCodecContext *avctx,
>              goto fail_mapped;
>  
>          memcpy(pkt->data, buf->buf, buf->size);
> +
> +        memset(buf->buf, 0, buf->size);
> +        buf->size = 0;

Seems unrelated?

>      }
>  
>      if (pic->type == PICTURE_TYPE_IDR)
> @@ -556,7 +683,12 @@ fail:
>  static int vaapi_encode_discard(AVCodecContext *avctx,
>                                  VAAPIEncodePicture *pic)
>  {
> -    vaapi_encode_wait(avctx, pic);
> +    if (!pic->skipped_flag) {
> +        vaapi_encode_wait(avctx, pic);
> +    } else {
> +        av_frame_free(&pic->input_image);
> +        pic->encode_complete = 1;
> +    }
>  
>      if (pic->output_buffer_ref) {
>          av_log(avctx, AV_LOG_DEBUG, "Discard output for pic "
> @@ -1994,6 +2126,8 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx)
>          }
>      }
>  
> +    ctx->skipped_pic_count = 0;
> +
>      return 0;
>  
>  fail:
> diff --git a/libavcodec/vaapi_encode.h b/libavcodec/vaapi_encode.h
> index 965fe65..bcee0b6 100644
> --- a/libavcodec/vaapi_encode.h
> +++ b/libavcodec/vaapi_encode.h
> @@ -92,6 +92,8 @@ typedef struct VAAPIEncodePicture {
>  
>      int          nb_slices;
>      VAAPIEncodeSlice *slices;
> +
> +    uint8_t skipped_flag;
>  } VAAPIEncodePicture;
>  
>  typedef struct VAAPIEncodeProfile {
> @@ -246,6 +248,9 @@ typedef struct VAAPIEncodeContext {
>      int gop_counter;
>      int p_counter;
>      int end_of_stream;
> +
> +    // Skipped frame info
> +    unsigned int skipped_pic_count;
>  } VAAPIEncodeContext;
>  
>  enum {
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 9b3fb13..c5fa205 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -840,6 +840,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)
>      case AV_FRAME_DATA_QP_TABLE_PROPERTIES:         return "QP table properties";
>      case AV_FRAME_DATA_QP_TABLE_DATA:               return "QP table data";
>  #endif
> +    case AV_FRAME_DATA_SKIP_FRAME:                  return "Skip frame";
>      }
>      return NULL;
>  }
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 66f27f4..8ef6475 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -166,6 +166,11 @@ enum AVFrameSideDataType {
>       * function in libavutil/timecode.c.
>       */
>      AV_FRAME_DATA_S12M_TIMECODE,
> +
> +    /**
> +     * VAAPI Encode skip-frame indicator.
> +     */
> +    AV_FRAME_DATA_SKIP_FRAME,
>  };
>  
>  enum AVActiveFormatDescription {
>

Hi Mark,

In some cases, that is useful. For example, an online content distributer, who keeps encoding the captured video frames by ffmpeg and sending them out. At times, there is no update of source, which makes one or several captured source frames are exactly the same as the last one, and the distributer wants to just skip such frames, without stopping the encoding process.

Regards,
SUN, Jing


-----Original Message-----
From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Mark Thompson

Sent: Tuesday, November 20, 2018 4:07 AM
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add frame-skip func

On 19/11/18 09:04, Jing SUN wrote:
> frame-skip is required to implement network bandwidth self-adaptive 

> vaapi encoding.

> To make a frame skipped, allocate its frame side data of 

> AV_FRAME_DATA_SKIP_FRAME type and set its value to 1.


So if I'm reading this correctly the idea is to implement partial VFR by having a special new side-data type which indicates where frames would have been had the input actually matched the configured CFR behaviour?

Why is the user meant to create these special frames?  It seems to me that the existing method of looking at the timestamps would be a better way to find any gaps.

(Or, even better, add timestamps to VAAPI so that it can support VFR in a sensible way rather than adding hacks like this to allow partial VFR with weird constraints.)

> Signed-off-by: Jing SUN <jing.a.sun@intel.com>

> ---

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

>  libavcodec/vaapi_encode.h |   5 ++

>  libavutil/frame.c         |   1 +

>  libavutil/frame.h         |   5 ++

>  4 files changed, 149 insertions(+), 4 deletions(-)

> 

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

> index 2fe8501..a401d61 100644

> --- a/libavcodec/vaapi_encode.c

> +++ b/libavcodec/vaapi_encode.c

> @@ -23,6 +23,7 @@

>  #include "libavutil/common.h"

>  #include "libavutil/log.h"

>  #include "libavutil/pixdesc.h"

> +#include "libavutil/intreadwrite.h"

>  

>  #include "vaapi_encode.h"

>  #include "avcodec.h"

> @@ -103,6 +104,47 @@ static int vaapi_encode_make_param_buffer(AVCodecContext *avctx,

>      return 0;

>  }

>  

> +static int vaapi_encode_check_if_skip(AVCodecContext *avctx,

> +                                      VAAPIEncodePicture *pic) {

> +    AVFrameSideData *fside = NULL;

> +    VAAPIEncodeContext *ctx = avctx->priv_data;

> +    VAAPIEncodePicture *cur = NULL;

> +    int i = 0;

> +

> +    if (!pic || !pic->input_image)

> +        return AVERROR(EINVAL);

> +

> +    fside = av_frame_get_side_data(pic->input_image, AV_FRAME_DATA_SKIP_FRAME);

> +    if (fside)

> +        pic->skipped_flag = AV_RL8(fside->data);

> +    else

> +        pic->skipped_flag = 0;

> +

> +    if (0 == pic->skipped_flag)

> +        return 0;

> +

> +    if ((pic->type == PICTURE_TYPE_IDR) || (pic->type == PICTURE_TYPE_I)) {

> +        av_log(avctx, AV_LOG_INFO, "Can't skip IDR/I pic %"PRId64"/%"PRId64".\n",

> +               pic->display_order, pic->encode_order);

> +        pic->skipped_flag = 0;

> +        return 0;

> +    }

> +

> +    for (cur = ctx->pic_start; cur; cur = cur->next) {

> +        for (i=0; i < cur->nb_refs; ++i) {

> +            if (cur->refs[i] == pic) {

> +                av_log(avctx, AV_LOG_INFO, "Can't skip ref pic %"PRId64"/%"PRId64".\n",

> +                       pic->display_order, pic->encode_order);

> +                pic->skipped_flag = 0;

> +                return 0;

> +            }

> +        }

> +    }

> +

> +    return 0;

> +}

> +

>  static int vaapi_encode_wait(AVCodecContext *avctx,

>                               VAAPIEncodePicture *pic)  { @@ -418,6 

> +460,69 @@ static int vaapi_encode_issue(AVCodecContext *avctx,

>          }

>      }

>  

> +    err = vaapi_encode_check_if_skip(avctx, pic);

> +    if (err != 0)

> +        av_log(avctx, AV_LOG_ERROR, "Fail to check if skip.\n");

> +

> +#ifdef VAEncMiscParameterSkipFrame

> +    if (pic->skipped_flag) {

> +        av_log(avctx, AV_LOG_INFO, "Skip pic %"PRId64"/%"PRId64" as requested.\n",

> +               pic->display_order, pic->encode_order);

> +

> +        ++ctx->skipped_pic_count;

> +        pic->encode_issued = 1;

> +

> +        return 0;

> +    } else if (ctx->skipped_pic_count > 0) {

> +        VABufferID skip_param_id;

> +        VAEncMiscParameterBuffer *misc_param;

> +        VAEncMiscParameterSkipFrame *skip_param;

> +

> +        err = vaapi_encode_make_param_buffer(avctx, pic,

> +                  VAEncMiscParameterBufferType, NULL,

> +                  (sizeof(VAEncMiscParameterBuffer) +

> +                  sizeof(VAEncMiscParameterSkipFrame)));

> +        if (err < 0)

> +            goto fail;

> +

> +        skip_param_id = pic->param_buffers[pic->nb_param_buffers-1];

> +

> +        vas = vaMapBuffer(ctx->hwctx->display,

> +                          skip_param_id,

> +                          (void **)&misc_param);

> +        if (vas != VA_STATUS_SUCCESS) {

> +            av_log(avctx, AV_LOG_ERROR, "Failed to map skip-frame buffer: "

> +                   "%d (%s).\n", vas, vaErrorStr(vas));

> +            err = AVERROR(EIO);

> +            goto fail;

> +        }

> +

> +        misc_param->type = 

> + (VAEncMiscParameterType)VAEncMiscParameterTypeSkipFrame;


You need to check VAConfigAttribEncSkipFrame to make sure this type is actually supported before using it.

> +        skip_param = (VAEncMiscParameterSkipFrame *)misc_param->data;

> +        skip_param->skip_frame_flag = 1;

> +        skip_param->num_skip_frames = ctx->skipped_pic_count;

> +        skip_param->size_skip_frames = 0;

> +

> +        vas = vaUnmapBuffer(ctx->hwctx->display, skip_param_id);

> +        if (vas != VA_STATUS_SUCCESS) {

> +            av_log(avctx, AV_LOG_ERROR, "Failed to unmap skip-frame buffer: "

> +                   "%d (%s).\n", vas, vaErrorStr(vas));

> +            err = AVERROR(EIO);

> +            goto fail;

> +        }


make_param_buffer can be called with the intended data directly - there is no need for this map/unmap.

> +

> +        ctx->skipped_pic_count = 0;

> +    }

> +#else

> +    if (pic->skipped_flag) {

> +        av_log(avctx, AV_LOG_INFO, "Skip-frame isn't supported and pic %"PRId64"/%"PRId64" isn't skipped.\n",

> +               pic->display_order, pic->encode_order);

> +

> +        pic->skipped_flag = 0;

> +        ctx->skipped_pic_count = 0;

> +    }

> +#endif

> +

>      vas = vaBeginPicture(ctx->hwctx->display, ctx->va_context,

>                           pic->input_surface);

>      if (vas != VA_STATUS_SUCCESS) {

> @@ -500,9 +605,28 @@ static int vaapi_encode_output(AVCodecContext *avctx,

>      VAStatus vas;

>      int err;

>  

> -    err = vaapi_encode_wait(avctx, pic);

> -    if (err < 0)

> -        return err;

> +    if (!pic->skipped_flag) {

> +        err = vaapi_encode_wait(avctx, pic);

> +        if (err < 0)

> +            return err;

> +    } else {

> +        av_frame_free(&pic->input_image);

> +        pic->encode_complete = 1;

> +

> +        err = av_new_packet(pkt, 0);

> +        if (err < 0)

> +            goto fail;

> +

> +        pkt->pts = pic->pts;

> +

> +        av_buffer_unref(&pic->output_buffer_ref);

> +        pic->output_buffer = VA_INVALID_ID;

> +

> +        av_log(avctx, AV_LOG_DEBUG, "Output 0 byte for pic %"PRId64"/%"PRId64".\n",

> +               pic->display_order, pic->encode_order);


Why is it helpful to create a zero-byte packet in this case?  Since no frame was encoded, I think no packet should be produced.

> +

> +        return 0;

> +    }

>  

>      buf_list = NULL;

>      vas = vaMapBuffer(ctx->hwctx->display, pic->output_buffer, @@ 

> -523,6 +647,9 @@ static int vaapi_encode_output(AVCodecContext *avctx,

>              goto fail_mapped;

>  

>          memcpy(pkt->data, buf->buf, buf->size);

> +

> +        memset(buf->buf, 0, buf->size);

> +        buf->size = 0;


Seems unrelated?

>      }

>  

>      if (pic->type == PICTURE_TYPE_IDR) @@ -556,7 +683,12 @@ fail:

>  static int vaapi_encode_discard(AVCodecContext *avctx,

>                                  VAAPIEncodePicture *pic)  {

> -    vaapi_encode_wait(avctx, pic);

> +    if (!pic->skipped_flag) {

> +        vaapi_encode_wait(avctx, pic);

> +    } else {

> +        av_frame_free(&pic->input_image);

> +        pic->encode_complete = 1;

> +    }

>  

>      if (pic->output_buffer_ref) {

>          av_log(avctx, AV_LOG_DEBUG, "Discard output for pic "

> @@ -1994,6 +2126,8 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx)

>          }

>      }

>  

> +    ctx->skipped_pic_count = 0;

> +

>      return 0;

>  

>  fail:

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

> index 965fe65..bcee0b6 100644

> --- a/libavcodec/vaapi_encode.h

> +++ b/libavcodec/vaapi_encode.h

> @@ -92,6 +92,8 @@ typedef struct VAAPIEncodePicture {

>  

>      int          nb_slices;

>      VAAPIEncodeSlice *slices;

> +

> +    uint8_t skipped_flag;

>  } VAAPIEncodePicture;

>  

>  typedef struct VAAPIEncodeProfile {

> @@ -246,6 +248,9 @@ typedef struct VAAPIEncodeContext {

>      int gop_counter;

>      int p_counter;

>      int end_of_stream;

> +

> +    // Skipped frame info

> +    unsigned int skipped_pic_count;

>  } VAAPIEncodeContext;

>  

>  enum {

> diff --git a/libavutil/frame.c b/libavutil/frame.c index 

> 9b3fb13..c5fa205 100644

> --- a/libavutil/frame.c

> +++ b/libavutil/frame.c

> @@ -840,6 +840,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)

>      case AV_FRAME_DATA_QP_TABLE_PROPERTIES:         return "QP table properties";

>      case AV_FRAME_DATA_QP_TABLE_DATA:               return "QP table data";

>  #endif

> +    case AV_FRAME_DATA_SKIP_FRAME:                  return "Skip frame";

>      }

>      return NULL;

>  }

> diff --git a/libavutil/frame.h b/libavutil/frame.h index 

> 66f27f4..8ef6475 100644

> --- a/libavutil/frame.h

> +++ b/libavutil/frame.h

> @@ -166,6 +166,11 @@ enum AVFrameSideDataType {

>       * function in libavutil/timecode.c.

>       */

>      AV_FRAME_DATA_S12M_TIMECODE,

> +

> +    /**

> +     * VAAPI Encode skip-frame indicator.

> +     */

> +    AV_FRAME_DATA_SKIP_FRAME,

>  };

>  

>  enum AVActiveFormatDescription {

> 

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Hi Mark,

Is there any issue that I need to fix for this patch please? 

Regards,
SUN, Jing


-----Original Message-----
From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Sun, Jing A

Sent: Friday, November 23, 2018 5:37 PM
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add frame-skip func

Hi Mark,

In some cases, that is useful. For example, an online content distributer, who keeps encoding the captured video frames by ffmpeg and sending them out. At times, there is no update of source, which makes one or several captured source frames are exactly the same as the last one, and the distributer wants to just skip such frames, without stopping the encoding process.

Regards,
SUN, Jing


-----Original Message-----
From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Mark Thompson

Sent: Tuesday, November 20, 2018 4:07 AM
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add frame-skip func

On 19/11/18 09:04, Jing SUN wrote:
> frame-skip is required to implement network bandwidth self-adaptive 

> vaapi encoding.

> To make a frame skipped, allocate its frame side data of 

> AV_FRAME_DATA_SKIP_FRAME type and set its value to 1.


So if I'm reading this correctly the idea is to implement partial VFR by having a special new side-data type which indicates where frames would have been had the input actually matched the configured CFR behaviour?

Why is the user meant to create these special frames?  It seems to me that the existing method of looking at the timestamps would be a better way to find any gaps.

(Or, even better, add timestamps to VAAPI so that it can support VFR in a sensible way rather than adding hacks like this to allow partial VFR with weird constraints.)

> Signed-off-by: Jing SUN <jing.a.sun@intel.com>

> ---

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

>  libavcodec/vaapi_encode.h |   5 ++

>  libavutil/frame.c         |   1 +

>  libavutil/frame.h         |   5 ++

>  4 files changed, 149 insertions(+), 4 deletions(-)

> 

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

> index 2fe8501..a401d61 100644

> --- a/libavcodec/vaapi_encode.c

> +++ b/libavcodec/vaapi_encode.c

> @@ -23,6 +23,7 @@

>  #include "libavutil/common.h"

>  #include "libavutil/log.h"

>  #include "libavutil/pixdesc.h"

> +#include "libavutil/intreadwrite.h"

>  

>  #include "vaapi_encode.h"

>  #include "avcodec.h"

> @@ -103,6 +104,47 @@ static int vaapi_encode_make_param_buffer(AVCodecContext *avctx,

>      return 0;

>  }

>  

> +static int vaapi_encode_check_if_skip(AVCodecContext *avctx,

> +                                      VAAPIEncodePicture *pic) {

> +    AVFrameSideData *fside = NULL;

> +    VAAPIEncodeContext *ctx = avctx->priv_data;

> +    VAAPIEncodePicture *cur = NULL;

> +    int i = 0;

> +

> +    if (!pic || !pic->input_image)

> +        return AVERROR(EINVAL);

> +

> +    fside = av_frame_get_side_data(pic->input_image, AV_FRAME_DATA_SKIP_FRAME);

> +    if (fside)

> +        pic->skipped_flag = AV_RL8(fside->data);

> +    else

> +        pic->skipped_flag = 0;

> +

> +    if (0 == pic->skipped_flag)

> +        return 0;

> +

> +    if ((pic->type == PICTURE_TYPE_IDR) || (pic->type == PICTURE_TYPE_I)) {

> +        av_log(avctx, AV_LOG_INFO, "Can't skip IDR/I pic %"PRId64"/%"PRId64".\n",

> +               pic->display_order, pic->encode_order);

> +        pic->skipped_flag = 0;

> +        return 0;

> +    }

> +

> +    for (cur = ctx->pic_start; cur; cur = cur->next) {

> +        for (i=0; i < cur->nb_refs; ++i) {

> +            if (cur->refs[i] == pic) {

> +                av_log(avctx, AV_LOG_INFO, "Can't skip ref pic %"PRId64"/%"PRId64".\n",

> +                       pic->display_order, pic->encode_order);

> +                pic->skipped_flag = 0;

> +                return 0;

> +            }

> +        }

> +    }

> +

> +    return 0;

> +}

> +

>  static int vaapi_encode_wait(AVCodecContext *avctx,

>                               VAAPIEncodePicture *pic)  { @@ -418,6

> +460,69 @@ static int vaapi_encode_issue(AVCodecContext *avctx,

>          }

>      }

>  

> +    err = vaapi_encode_check_if_skip(avctx, pic);

> +    if (err != 0)

> +        av_log(avctx, AV_LOG_ERROR, "Fail to check if skip.\n");

> +

> +#ifdef VAEncMiscParameterSkipFrame

> +    if (pic->skipped_flag) {

> +        av_log(avctx, AV_LOG_INFO, "Skip pic %"PRId64"/%"PRId64" as requested.\n",

> +               pic->display_order, pic->encode_order);

> +

> +        ++ctx->skipped_pic_count;

> +        pic->encode_issued = 1;

> +

> +        return 0;

> +    } else if (ctx->skipped_pic_count > 0) {

> +        VABufferID skip_param_id;

> +        VAEncMiscParameterBuffer *misc_param;

> +        VAEncMiscParameterSkipFrame *skip_param;

> +

> +        err = vaapi_encode_make_param_buffer(avctx, pic,

> +                  VAEncMiscParameterBufferType, NULL,

> +                  (sizeof(VAEncMiscParameterBuffer) +

> +                  sizeof(VAEncMiscParameterSkipFrame)));

> +        if (err < 0)

> +            goto fail;

> +

> +        skip_param_id = pic->param_buffers[pic->nb_param_buffers-1];

> +

> +        vas = vaMapBuffer(ctx->hwctx->display,

> +                          skip_param_id,

> +                          (void **)&misc_param);

> +        if (vas != VA_STATUS_SUCCESS) {

> +            av_log(avctx, AV_LOG_ERROR, "Failed to map skip-frame buffer: "

> +                   "%d (%s).\n", vas, vaErrorStr(vas));

> +            err = AVERROR(EIO);

> +            goto fail;

> +        }

> +

> +        misc_param->type =

> + (VAEncMiscParameterType)VAEncMiscParameterTypeSkipFrame;


You need to check VAConfigAttribEncSkipFrame to make sure this type is actually supported before using it.

> +        skip_param = (VAEncMiscParameterSkipFrame *)misc_param->data;

> +        skip_param->skip_frame_flag = 1;

> +        skip_param->num_skip_frames = ctx->skipped_pic_count;

> +        skip_param->size_skip_frames = 0;

> +

> +        vas = vaUnmapBuffer(ctx->hwctx->display, skip_param_id);

> +        if (vas != VA_STATUS_SUCCESS) {

> +            av_log(avctx, AV_LOG_ERROR, "Failed to unmap skip-frame buffer: "

> +                   "%d (%s).\n", vas, vaErrorStr(vas));

> +            err = AVERROR(EIO);

> +            goto fail;

> +        }


make_param_buffer can be called with the intended data directly - there is no need for this map/unmap.

> +

> +        ctx->skipped_pic_count = 0;

> +    }

> +#else

> +    if (pic->skipped_flag) {

> +        av_log(avctx, AV_LOG_INFO, "Skip-frame isn't supported and pic %"PRId64"/%"PRId64" isn't skipped.\n",

> +               pic->display_order, pic->encode_order);

> +

> +        pic->skipped_flag = 0;

> +        ctx->skipped_pic_count = 0;

> +    }

> +#endif

> +

>      vas = vaBeginPicture(ctx->hwctx->display, ctx->va_context,

>                           pic->input_surface);

>      if (vas != VA_STATUS_SUCCESS) {

> @@ -500,9 +605,28 @@ static int vaapi_encode_output(AVCodecContext *avctx,

>      VAStatus vas;

>      int err;

>  

> -    err = vaapi_encode_wait(avctx, pic);

> -    if (err < 0)

> -        return err;

> +    if (!pic->skipped_flag) {

> +        err = vaapi_encode_wait(avctx, pic);

> +        if (err < 0)

> +            return err;

> +    } else {

> +        av_frame_free(&pic->input_image);

> +        pic->encode_complete = 1;

> +

> +        err = av_new_packet(pkt, 0);

> +        if (err < 0)

> +            goto fail;

> +

> +        pkt->pts = pic->pts;

> +

> +        av_buffer_unref(&pic->output_buffer_ref);

> +        pic->output_buffer = VA_INVALID_ID;

> +

> +        av_log(avctx, AV_LOG_DEBUG, "Output 0 byte for pic %"PRId64"/%"PRId64".\n",

> +               pic->display_order, pic->encode_order);


Why is it helpful to create a zero-byte packet in this case?  Since no frame was encoded, I think no packet should be produced.

> +

> +        return 0;

> +    }

>  

>      buf_list = NULL;

>      vas = vaMapBuffer(ctx->hwctx->display, pic->output_buffer, @@

> -523,6 +647,9 @@ static int vaapi_encode_output(AVCodecContext *avctx,

>              goto fail_mapped;

>  

>          memcpy(pkt->data, buf->buf, buf->size);

> +

> +        memset(buf->buf, 0, buf->size);

> +        buf->size = 0;


Seems unrelated?

>      }

>  

>      if (pic->type == PICTURE_TYPE_IDR) @@ -556,7 +683,12 @@ fail:

>  static int vaapi_encode_discard(AVCodecContext *avctx,

>                                  VAAPIEncodePicture *pic)  {

> -    vaapi_encode_wait(avctx, pic);

> +    if (!pic->skipped_flag) {

> +        vaapi_encode_wait(avctx, pic);

> +    } else {

> +        av_frame_free(&pic->input_image);

> +        pic->encode_complete = 1;

> +    }

>  

>      if (pic->output_buffer_ref) {

>          av_log(avctx, AV_LOG_DEBUG, "Discard output for pic "

> @@ -1994,6 +2126,8 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx)

>          }

>      }

>  

> +    ctx->skipped_pic_count = 0;

> +

>      return 0;

>  

>  fail:

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

> index 965fe65..bcee0b6 100644

> --- a/libavcodec/vaapi_encode.h

> +++ b/libavcodec/vaapi_encode.h

> @@ -92,6 +92,8 @@ typedef struct VAAPIEncodePicture {

>  

>      int          nb_slices;

>      VAAPIEncodeSlice *slices;

> +

> +    uint8_t skipped_flag;

>  } VAAPIEncodePicture;

>  

>  typedef struct VAAPIEncodeProfile {

> @@ -246,6 +248,9 @@ typedef struct VAAPIEncodeContext {

>      int gop_counter;

>      int p_counter;

>      int end_of_stream;

> +

> +    // Skipped frame info

> +    unsigned int skipped_pic_count;

>  } VAAPIEncodeContext;

>  

>  enum {

> diff --git a/libavutil/frame.c b/libavutil/frame.c index

> 9b3fb13..c5fa205 100644

> --- a/libavutil/frame.c

> +++ b/libavutil/frame.c

> @@ -840,6 +840,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)

>      case AV_FRAME_DATA_QP_TABLE_PROPERTIES:         return "QP table properties";

>      case AV_FRAME_DATA_QP_TABLE_DATA:               return "QP table data";

>  #endif

> +    case AV_FRAME_DATA_SKIP_FRAME:                  return "Skip frame";

>      }

>      return NULL;

>  }

> diff --git a/libavutil/frame.h b/libavutil/frame.h index

> 66f27f4..8ef6475 100644

> --- a/libavutil/frame.h

> +++ b/libavutil/frame.h

> @@ -166,6 +166,11 @@ enum AVFrameSideDataType {

>       * function in libavutil/timecode.c.

>       */

>      AV_FRAME_DATA_S12M_TIMECODE,

> +

> +    /**

> +     * VAAPI Encode skip-frame indicator.

> +     */

> +    AV_FRAME_DATA_SKIP_FRAME,

>  };

>  

>  enum AVActiveFormatDescription {

> 

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

2018-12-04 2:46 GMT+01:00, Sun, Jing A <jing.a.sun@intel.com>:
> Hi Mark,
>
> Is there any issue that I need to fix for this patch please?

Did you already send a patch with the version bump?

Please avoid top-posting here, Carl Eugen

On 04/12/2018 01:46, Sun, Jing A wrote:
> Hi Mark,
> 
> Is there any issue that I need to fix for this patch please? 

See comments in the message you quoted below?  I think the most important point is that the existing timing information appears to contain all the information you want for VFR, so you probably shouldn't need the ad-hoc side-data type.

- Mark


> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Sun, Jing A
> Sent: Friday, November 23, 2018 5:37 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add frame-skip func
> 
> Hi Mark,
> 
> In some cases, that is useful. For example, an online content distributer, who keeps encoding the captured video frames by ffmpeg and sending them out. At times, there is no update of source, which makes one or several captured source frames are exactly the same as the last one, and the distributer wants to just skip such frames, without stopping the encoding process.
> 
> Regards,
> SUN, Jing
> 
> 
> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Mark Thompson
> Sent: Tuesday, November 20, 2018 4:07 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add frame-skip func
> 
> On 19/11/18 09:04, Jing SUN wrote:
>> frame-skip is required to implement network bandwidth self-adaptive 
>> vaapi encoding.
>> To make a frame skipped, allocate its frame side data of 
>> AV_FRAME_DATA_SKIP_FRAME type and set its value to 1.
> 
> So if I'm reading this correctly the idea is to implement partial VFR by having a special new side-data type which indicates where frames would have been had the input actually matched the configured CFR behaviour?
> 
> Why is the user meant to create these special frames?  It seems to me that the existing method of looking at the timestamps would be a better way to find any gaps.
> 
> (Or, even better, add timestamps to VAAPI so that it can support VFR in a sensible way rather than adding hacks like this to allow partial VFR with weird constraints.)
> 
>> Signed-off-by: Jing SUN <jing.a.sun@intel.com>
>> ---
>>  libavcodec/vaapi_encode.c | 142 ++++++++++++++++++++++++++++++++++++++++++++--
>>  libavcodec/vaapi_encode.h |   5 ++
>>  libavutil/frame.c         |   1 +
>>  libavutil/frame.h         |   5 ++
>>  4 files changed, 149 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c 
>> index 2fe8501..a401d61 100644
>> --- a/libavcodec/vaapi_encode.c
>> +++ b/libavcodec/vaapi_encode.c
>> @@ -23,6 +23,7 @@
>>  #include "libavutil/common.h"
>>  #include "libavutil/log.h"
>>  #include "libavutil/pixdesc.h"
>> +#include "libavutil/intreadwrite.h"
>>  
>>  #include "vaapi_encode.h"
>>  #include "avcodec.h"
>> @@ -103,6 +104,47 @@ static int vaapi_encode_make_param_buffer(AVCodecContext *avctx,
>>      return 0;
>>  }
>>  
>> +static int vaapi_encode_check_if_skip(AVCodecContext *avctx,
>> +                                      VAAPIEncodePicture *pic) {
>> +    AVFrameSideData *fside = NULL;
>> +    VAAPIEncodeContext *ctx = avctx->priv_data;
>> +    VAAPIEncodePicture *cur = NULL;
>> +    int i = 0;
>> +
>> +    if (!pic || !pic->input_image)
>> +        return AVERROR(EINVAL);
>> +
>> +    fside = av_frame_get_side_data(pic->input_image, AV_FRAME_DATA_SKIP_FRAME);
>> +    if (fside)
>> +        pic->skipped_flag = AV_RL8(fside->data);
>> +    else
>> +        pic->skipped_flag = 0;
>> +
>> +    if (0 == pic->skipped_flag)
>> +        return 0;
>> +
>> +    if ((pic->type == PICTURE_TYPE_IDR) || (pic->type == PICTURE_TYPE_I)) {
>> +        av_log(avctx, AV_LOG_INFO, "Can't skip IDR/I pic %"PRId64"/%"PRId64".\n",
>> +               pic->display_order, pic->encode_order);
>> +        pic->skipped_flag = 0;
>> +        return 0;
>> +    }
>> +
>> +    for (cur = ctx->pic_start; cur; cur = cur->next) {
>> +        for (i=0; i < cur->nb_refs; ++i) {
>> +            if (cur->refs[i] == pic) {
>> +                av_log(avctx, AV_LOG_INFO, "Can't skip ref pic %"PRId64"/%"PRId64".\n",
>> +                       pic->display_order, pic->encode_order);
>> +                pic->skipped_flag = 0;
>> +                return 0;
>> +            }
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static int vaapi_encode_wait(AVCodecContext *avctx,
>>                               VAAPIEncodePicture *pic)  { @@ -418,6
>> +460,69 @@ static int vaapi_encode_issue(AVCodecContext *avctx,
>>          }
>>      }
>>  
>> +    err = vaapi_encode_check_if_skip(avctx, pic);
>> +    if (err != 0)
>> +        av_log(avctx, AV_LOG_ERROR, "Fail to check if skip.\n");
>> +
>> +#ifdef VAEncMiscParameterSkipFrame
>> +    if (pic->skipped_flag) {
>> +        av_log(avctx, AV_LOG_INFO, "Skip pic %"PRId64"/%"PRId64" as requested.\n",
>> +               pic->display_order, pic->encode_order);
>> +
>> +        ++ctx->skipped_pic_count;
>> +        pic->encode_issued = 1;
>> +
>> +        return 0;
>> +    } else if (ctx->skipped_pic_count > 0) {
>> +        VABufferID skip_param_id;
>> +        VAEncMiscParameterBuffer *misc_param;
>> +        VAEncMiscParameterSkipFrame *skip_param;
>> +
>> +        err = vaapi_encode_make_param_buffer(avctx, pic,
>> +                  VAEncMiscParameterBufferType, NULL,
>> +                  (sizeof(VAEncMiscParameterBuffer) +
>> +                  sizeof(VAEncMiscParameterSkipFrame)));
>> +        if (err < 0)
>> +            goto fail;
>> +
>> +        skip_param_id = pic->param_buffers[pic->nb_param_buffers-1];
>> +
>> +        vas = vaMapBuffer(ctx->hwctx->display,
>> +                          skip_param_id,
>> +                          (void **)&misc_param);
>> +        if (vas != VA_STATUS_SUCCESS) {
>> +            av_log(avctx, AV_LOG_ERROR, "Failed to map skip-frame buffer: "
>> +                   "%d (%s).\n", vas, vaErrorStr(vas));
>> +            err = AVERROR(EIO);
>> +            goto fail;
>> +        }
>> +
>> +        misc_param->type =
>> + (VAEncMiscParameterType)VAEncMiscParameterTypeSkipFrame;
> 
> You need to check VAConfigAttribEncSkipFrame to make sure this type is actually supported before using it.
> 
>> +        skip_param = (VAEncMiscParameterSkipFrame *)misc_param->data;
>> +        skip_param->skip_frame_flag = 1;
>> +        skip_param->num_skip_frames = ctx->skipped_pic_count;
>> +        skip_param->size_skip_frames = 0;
>> +
>> +        vas = vaUnmapBuffer(ctx->hwctx->display, skip_param_id);
>> +        if (vas != VA_STATUS_SUCCESS) {
>> +            av_log(avctx, AV_LOG_ERROR, "Failed to unmap skip-frame buffer: "
>> +                   "%d (%s).\n", vas, vaErrorStr(vas));
>> +            err = AVERROR(EIO);
>> +            goto fail;
>> +        }
> 
> make_param_buffer can be called with the intended data directly - there is no need for this map/unmap.
> 
>> +
>> +        ctx->skipped_pic_count = 0;
>> +    }
>> +#else
>> +    if (pic->skipped_flag) {
>> +        av_log(avctx, AV_LOG_INFO, "Skip-frame isn't supported and pic %"PRId64"/%"PRId64" isn't skipped.\n",
>> +               pic->display_order, pic->encode_order);
>> +
>> +        pic->skipped_flag = 0;
>> +        ctx->skipped_pic_count = 0;
>> +    }
>> +#endif
>> +
>>      vas = vaBeginPicture(ctx->hwctx->display, ctx->va_context,
>>                           pic->input_surface);
>>      if (vas != VA_STATUS_SUCCESS) {
>> @@ -500,9 +605,28 @@ static int vaapi_encode_output(AVCodecContext *avctx,
>>      VAStatus vas;
>>      int err;
>>  
>> -    err = vaapi_encode_wait(avctx, pic);
>> -    if (err < 0)
>> -        return err;
>> +    if (!pic->skipped_flag) {
>> +        err = vaapi_encode_wait(avctx, pic);
>> +        if (err < 0)
>> +            return err;
>> +    } else {
>> +        av_frame_free(&pic->input_image);
>> +        pic->encode_complete = 1;
>> +
>> +        err = av_new_packet(pkt, 0);
>> +        if (err < 0)
>> +            goto fail;
>> +
>> +        pkt->pts = pic->pts;
>> +
>> +        av_buffer_unref(&pic->output_buffer_ref);
>> +        pic->output_buffer = VA_INVALID_ID;
>> +
>> +        av_log(avctx, AV_LOG_DEBUG, "Output 0 byte for pic %"PRId64"/%"PRId64".\n",
>> +               pic->display_order, pic->encode_order);
> 
> Why is it helpful to create a zero-byte packet in this case?  Since no frame was encoded, I think no packet should be produced.
> 
>> +
>> +        return 0;
>> +    }
>>  
>>      buf_list = NULL;
>>      vas = vaMapBuffer(ctx->hwctx->display, pic->output_buffer, @@
>> -523,6 +647,9 @@ static int vaapi_encode_output(AVCodecContext *avctx,
>>              goto fail_mapped;
>>  
>>          memcpy(pkt->data, buf->buf, buf->size);
>> +
>> +        memset(buf->buf, 0, buf->size);
>> +        buf->size = 0;
> 
> Seems unrelated?
> 
>>      }
>>  
>>      if (pic->type == PICTURE_TYPE_IDR) @@ -556,7 +683,12 @@ fail:
>>  static int vaapi_encode_discard(AVCodecContext *avctx,
>>                                  VAAPIEncodePicture *pic)  {
>> -    vaapi_encode_wait(avctx, pic);
>> +    if (!pic->skipped_flag) {
>> +        vaapi_encode_wait(avctx, pic);
>> +    } else {
>> +        av_frame_free(&pic->input_image);
>> +        pic->encode_complete = 1;
>> +    }
>>  
>>      if (pic->output_buffer_ref) {
>>          av_log(avctx, AV_LOG_DEBUG, "Discard output for pic "
>> @@ -1994,6 +2126,8 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx)
>>          }
>>      }
>>  
>> +    ctx->skipped_pic_count = 0;
>> +
>>      return 0;
>>  
>>  fail:
>> diff --git a/libavcodec/vaapi_encode.h b/libavcodec/vaapi_encode.h 
>> index 965fe65..bcee0b6 100644
>> --- a/libavcodec/vaapi_encode.h
>> +++ b/libavcodec/vaapi_encode.h
>> @@ -92,6 +92,8 @@ typedef struct VAAPIEncodePicture {
>>  
>>      int          nb_slices;
>>      VAAPIEncodeSlice *slices;
>> +
>> +    uint8_t skipped_flag;
>>  } VAAPIEncodePicture;
>>  
>>  typedef struct VAAPIEncodeProfile {
>> @@ -246,6 +248,9 @@ typedef struct VAAPIEncodeContext {
>>      int gop_counter;
>>      int p_counter;
>>      int end_of_stream;
>> +
>> +    // Skipped frame info
>> +    unsigned int skipped_pic_count;
>>  } VAAPIEncodeContext;
>>  
>>  enum {
>> diff --git a/libavutil/frame.c b/libavutil/frame.c index
>> 9b3fb13..c5fa205 100644
>> --- a/libavutil/frame.c
>> +++ b/libavutil/frame.c
>> @@ -840,6 +840,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)
>>      case AV_FRAME_DATA_QP_TABLE_PROPERTIES:         return "QP table properties";
>>      case AV_FRAME_DATA_QP_TABLE_DATA:               return "QP table data";
>>  #endif
>> +    case AV_FRAME_DATA_SKIP_FRAME:                  return "Skip frame";
>>      }
>>      return NULL;
>>  }
>> diff --git a/libavutil/frame.h b/libavutil/frame.h index
>> 66f27f4..8ef6475 100644
>> --- a/libavutil/frame.h
>> +++ b/libavutil/frame.h
>> @@ -166,6 +166,11 @@ enum AVFrameSideDataType {
>>       * function in libavutil/timecode.c.
>>       */
>>      AV_FRAME_DATA_S12M_TIMECODE,
>> +
>> +    /**
>> +     * VAAPI Encode skip-frame indicator.
>> +     */
>> +    AV_FRAME_DATA_SKIP_FRAME,
>>  };
>>  
>>  enum AVActiveFormatDescription {
>>

Hi Mark,

This patch is not trying to support VFR. Some frames, after which are just produced, could be considered as not needed by theirs producer and will get skipped in the encoding process. And in my opinion the existing timing information is not sufficient to support the case.

Regards,
SUN, Jing


-----Original Message-----
From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Mark Thompson

Sent: Wednesday, December 5, 2018 7:50 AM
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add frame-skip func

On 04/12/2018 01:46, Sun, Jing A wrote:
> Hi Mark,

> 

> Is there any issue that I need to fix for this patch please? 


See comments in the message you quoted below?  I think the most important point is that the existing timing information appears to contain all the information you want for VFR, so you probably shouldn't need the ad-hoc side-data type.

- Mark


> -----Original Message-----

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

> Of Sun, Jing A

> Sent: Friday, November 23, 2018 5:37 PM

> To: FFmpeg development discussions and patches 

> <ffmpeg-devel@ffmpeg.org>

> Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add 

> frame-skip func

> 

> Hi Mark,

> 

> In some cases, that is useful. For example, an online content distributer, who keeps encoding the captured video frames by ffmpeg and sending them out. At times, there is no update of source, which makes one or several captured source frames are exactly the same as the last one, and the distributer wants to just skip such frames, without stopping the encoding process.

> 

> Regards,

> SUN, Jing

> 

> 

> -----Original Message-----

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

> Of Mark Thompson

> Sent: Tuesday, November 20, 2018 4:07 AM

> To: ffmpeg-devel@ffmpeg.org

> Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add 

> frame-skip func

> 

> On 19/11/18 09:04, Jing SUN wrote:

>> frame-skip is required to implement network bandwidth self-adaptive 

>> vaapi encoding.

>> To make a frame skipped, allocate its frame side data of 

>> AV_FRAME_DATA_SKIP_FRAME type and set its value to 1.

> 

> So if I'm reading this correctly the idea is to implement partial VFR by having a special new side-data type which indicates where frames would have been had the input actually matched the configured CFR behaviour?

> 

> Why is the user meant to create these special frames?  It seems to me that the existing method of looking at the timestamps would be a better way to find any gaps.

> 

> (Or, even better, add timestamps to VAAPI so that it can support VFR 

> in a sensible way rather than adding hacks like this to allow partial 

> VFR with weird constraints.)

> 

>> Signed-off-by: Jing SUN <jing.a.sun@intel.com>

>> ---

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

>>  libavcodec/vaapi_encode.h |   5 ++

>>  libavutil/frame.c         |   1 +

>>  libavutil/frame.h         |   5 ++

>>  4 files changed, 149 insertions(+), 4 deletions(-)

>>

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

>> index 2fe8501..a401d61 100644

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

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

>> @@ -23,6 +23,7 @@

>>  #include "libavutil/common.h"

>>  #include "libavutil/log.h"

>>  #include "libavutil/pixdesc.h"

>> +#include "libavutil/intreadwrite.h"

>>  

>>  #include "vaapi_encode.h"

>>  #include "avcodec.h"

>> @@ -103,6 +104,47 @@ static int vaapi_encode_make_param_buffer(AVCodecContext *avctx,

>>      return 0;

>>  }

>>  

>> +static int vaapi_encode_check_if_skip(AVCodecContext *avctx,

>> +                                      VAAPIEncodePicture *pic) {

>> +    AVFrameSideData *fside = NULL;

>> +    VAAPIEncodeContext *ctx = avctx->priv_data;

>> +    VAAPIEncodePicture *cur = NULL;

>> +    int i = 0;

>> +

>> +    if (!pic || !pic->input_image)

>> +        return AVERROR(EINVAL);

>> +

>> +    fside = av_frame_get_side_data(pic->input_image, AV_FRAME_DATA_SKIP_FRAME);

>> +    if (fside)

>> +        pic->skipped_flag = AV_RL8(fside->data);

>> +    else

>> +        pic->skipped_flag = 0;

>> +

>> +    if (0 == pic->skipped_flag)

>> +        return 0;

>> +

>> +    if ((pic->type == PICTURE_TYPE_IDR) || (pic->type == PICTURE_TYPE_I)) {

>> +        av_log(avctx, AV_LOG_INFO, "Can't skip IDR/I pic %"PRId64"/%"PRId64".\n",

>> +               pic->display_order, pic->encode_order);

>> +        pic->skipped_flag = 0;

>> +        return 0;

>> +    }

>> +

>> +    for (cur = ctx->pic_start; cur; cur = cur->next) {

>> +        for (i=0; i < cur->nb_refs; ++i) {

>> +            if (cur->refs[i] == pic) {

>> +                av_log(avctx, AV_LOG_INFO, "Can't skip ref pic %"PRId64"/%"PRId64".\n",

>> +                       pic->display_order, pic->encode_order);

>> +                pic->skipped_flag = 0;

>> +                return 0;

>> +            }

>> +        }

>> +    }

>> +

>> +    return 0;

>> +}

>> +

>>  static int vaapi_encode_wait(AVCodecContext *avctx,

>>                               VAAPIEncodePicture *pic)  { @@ -418,6

>> +460,69 @@ static int vaapi_encode_issue(AVCodecContext *avctx,

>>          }

>>      }

>>  

>> +    err = vaapi_encode_check_if_skip(avctx, pic);

>> +    if (err != 0)

>> +        av_log(avctx, AV_LOG_ERROR, "Fail to check if skip.\n");

>> +

>> +#ifdef VAEncMiscParameterSkipFrame

>> +    if (pic->skipped_flag) {

>> +        av_log(avctx, AV_LOG_INFO, "Skip pic %"PRId64"/%"PRId64" as requested.\n",

>> +               pic->display_order, pic->encode_order);

>> +

>> +        ++ctx->skipped_pic_count;

>> +        pic->encode_issued = 1;

>> +

>> +        return 0;

>> +    } else if (ctx->skipped_pic_count > 0) {

>> +        VABufferID skip_param_id;

>> +        VAEncMiscParameterBuffer *misc_param;

>> +        VAEncMiscParameterSkipFrame *skip_param;

>> +

>> +        err = vaapi_encode_make_param_buffer(avctx, pic,

>> +                  VAEncMiscParameterBufferType, NULL,

>> +                  (sizeof(VAEncMiscParameterBuffer) +

>> +                  sizeof(VAEncMiscParameterSkipFrame)));

>> +        if (err < 0)

>> +            goto fail;

>> +

>> +        skip_param_id = pic->param_buffers[pic->nb_param_buffers-1];

>> +

>> +        vas = vaMapBuffer(ctx->hwctx->display,

>> +                          skip_param_id,

>> +                          (void **)&misc_param);

>> +        if (vas != VA_STATUS_SUCCESS) {

>> +            av_log(avctx, AV_LOG_ERROR, "Failed to map skip-frame buffer: "

>> +                   "%d (%s).\n", vas, vaErrorStr(vas));

>> +            err = AVERROR(EIO);

>> +            goto fail;

>> +        }

>> +

>> +        misc_param->type =

>> + (VAEncMiscParameterType)VAEncMiscParameterTypeSkipFrame;

> 

> You need to check VAConfigAttribEncSkipFrame to make sure this type is actually supported before using it.

> 

>> +        skip_param = (VAEncMiscParameterSkipFrame *)misc_param->data;

>> +        skip_param->skip_frame_flag = 1;

>> +        skip_param->num_skip_frames = ctx->skipped_pic_count;

>> +        skip_param->size_skip_frames = 0;

>> +

>> +        vas = vaUnmapBuffer(ctx->hwctx->display, skip_param_id);

>> +        if (vas != VA_STATUS_SUCCESS) {

>> +            av_log(avctx, AV_LOG_ERROR, "Failed to unmap skip-frame buffer: "

>> +                   "%d (%s).\n", vas, vaErrorStr(vas));

>> +            err = AVERROR(EIO);

>> +            goto fail;

>> +        }

> 

> make_param_buffer can be called with the intended data directly - there is no need for this map/unmap.

> 

>> +

>> +        ctx->skipped_pic_count = 0;

>> +    }

>> +#else

>> +    if (pic->skipped_flag) {

>> +        av_log(avctx, AV_LOG_INFO, "Skip-frame isn't supported and pic %"PRId64"/%"PRId64" isn't skipped.\n",

>> +               pic->display_order, pic->encode_order);

>> +

>> +        pic->skipped_flag = 0;

>> +        ctx->skipped_pic_count = 0;

>> +    }

>> +#endif

>> +

>>      vas = vaBeginPicture(ctx->hwctx->display, ctx->va_context,

>>                           pic->input_surface);

>>      if (vas != VA_STATUS_SUCCESS) {

>> @@ -500,9 +605,28 @@ static int vaapi_encode_output(AVCodecContext *avctx,

>>      VAStatus vas;

>>      int err;

>>  

>> -    err = vaapi_encode_wait(avctx, pic);

>> -    if (err < 0)

>> -        return err;

>> +    if (!pic->skipped_flag) {

>> +        err = vaapi_encode_wait(avctx, pic);

>> +        if (err < 0)

>> +            return err;

>> +    } else {

>> +        av_frame_free(&pic->input_image);

>> +        pic->encode_complete = 1;

>> +

>> +        err = av_new_packet(pkt, 0);

>> +        if (err < 0)

>> +            goto fail;

>> +

>> +        pkt->pts = pic->pts;

>> +

>> +        av_buffer_unref(&pic->output_buffer_ref);

>> +        pic->output_buffer = VA_INVALID_ID;

>> +

>> +        av_log(avctx, AV_LOG_DEBUG, "Output 0 byte for pic %"PRId64"/%"PRId64".\n",

>> +               pic->display_order, pic->encode_order);

> 

> Why is it helpful to create a zero-byte packet in this case?  Since no frame was encoded, I think no packet should be produced.

> 

>> +

>> +        return 0;

>> +    }

>>  

>>      buf_list = NULL;

>>      vas = vaMapBuffer(ctx->hwctx->display, pic->output_buffer, @@

>> -523,6 +647,9 @@ static int vaapi_encode_output(AVCodecContext *avctx,

>>              goto fail_mapped;

>>  

>>          memcpy(pkt->data, buf->buf, buf->size);

>> +

>> +        memset(buf->buf, 0, buf->size);

>> +        buf->size = 0;

> 

> Seems unrelated?

> 

>>      }

>>  

>>      if (pic->type == PICTURE_TYPE_IDR) @@ -556,7 +683,12 @@ fail:

>>  static int vaapi_encode_discard(AVCodecContext *avctx,

>>                                  VAAPIEncodePicture *pic)  {

>> -    vaapi_encode_wait(avctx, pic);

>> +    if (!pic->skipped_flag) {

>> +        vaapi_encode_wait(avctx, pic);

>> +    } else {

>> +        av_frame_free(&pic->input_image);

>> +        pic->encode_complete = 1;

>> +    }

>>  

>>      if (pic->output_buffer_ref) {

>>          av_log(avctx, AV_LOG_DEBUG, "Discard output for pic "

>> @@ -1994,6 +2126,8 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx)

>>          }

>>      }

>>  

>> +    ctx->skipped_pic_count = 0;

>> +

>>      return 0;

>>  

>>  fail:

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

>> index 965fe65..bcee0b6 100644

>> --- a/libavcodec/vaapi_encode.h

>> +++ b/libavcodec/vaapi_encode.h

>> @@ -92,6 +92,8 @@ typedef struct VAAPIEncodePicture {

>>  

>>      int          nb_slices;

>>      VAAPIEncodeSlice *slices;

>> +

>> +    uint8_t skipped_flag;

>>  } VAAPIEncodePicture;

>>  

>>  typedef struct VAAPIEncodeProfile {

>> @@ -246,6 +248,9 @@ typedef struct VAAPIEncodeContext {

>>      int gop_counter;

>>      int p_counter;

>>      int end_of_stream;

>> +

>> +    // Skipped frame info

>> +    unsigned int skipped_pic_count;

>>  } VAAPIEncodeContext;

>>  

>>  enum {

>> diff --git a/libavutil/frame.c b/libavutil/frame.c index

>> 9b3fb13..c5fa205 100644

>> --- a/libavutil/frame.c

>> +++ b/libavutil/frame.c

>> @@ -840,6 +840,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)

>>      case AV_FRAME_DATA_QP_TABLE_PROPERTIES:         return "QP table properties";

>>      case AV_FRAME_DATA_QP_TABLE_DATA:               return "QP table data";

>>  #endif

>> +    case AV_FRAME_DATA_SKIP_FRAME:                  return "Skip frame";

>>      }

>>      return NULL;

>>  }

>> diff --git a/libavutil/frame.h b/libavutil/frame.h index

>> 66f27f4..8ef6475 100644

>> --- a/libavutil/frame.h

>> +++ b/libavutil/frame.h

>> @@ -166,6 +166,11 @@ enum AVFrameSideDataType {

>>       * function in libavutil/timecode.c.

>>       */

>>      AV_FRAME_DATA_S12M_TIMECODE,

>> +

>> +    /**

>> +     * VAAPI Encode skip-frame indicator.

>> +     */

>> +    AV_FRAME_DATA_SKIP_FRAME,

>>  };

>>  

>>  enum AVActiveFormatDescription {

>>

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

On 06/12/2018 10:04, Sun, Jing A wrote:
> Hi Mark,
> 
> This patch is not trying to support VFR. Some frames, after which are just produced, could be considered as not needed by theirs producer and will get skipped in the encoding process. And in my opinion the existing timing information is not sufficient to support the case.

A skipped frame will still have a timestamp, so if a frame is skipped before the encoder then no frame with that timestamp will be given to the encoder.  For CFR content this can be detected in the encoder to reconstruct your skip-frames information exactly - a skip has occurred if the gap between two frames does not match the framerate, and the size of the gap tells you how many frames were skipped.  Avoiding a requirement that the gap sizes exactly match makes it implement a simple VFR scheme too, since skips can be inserted to keep the rate controller correct as long as you never have frames closer together than the configured framerate.

(Please avoid top-posting on this list.)

- Mark


> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Mark Thompson
> Sent: Wednesday, December 5, 2018 7:50 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add frame-skip func
> 
> On 04/12/2018 01:46, Sun, Jing A wrote:
>> Hi Mark,
>>
>> Is there any issue that I need to fix for this patch please? 
> 
> See comments in the message you quoted below?  I think the most important point is that the existing timing information appears to contain all the information you want for VFR, so you probably shouldn't need the ad-hoc side-data type.
> 
> - Mark
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf 
>> Of Sun, Jing A
>> Sent: Friday, November 23, 2018 5:37 PM
>> To: FFmpeg development discussions and patches 
>> <ffmpeg-devel@ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add 
>> frame-skip func
>>
>> Hi Mark,
>>
>> In some cases, that is useful. For example, an online content distributer, who keeps encoding the captured video frames by ffmpeg and sending them out. At times, there is no update of source, which makes one or several captured source frames are exactly the same as the last one, and the distributer wants to just skip such frames, without stopping the encoding process.
>>
>> Regards,
>> SUN, Jing
>>
>>
>> -----Original Message-----
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf 
>> Of Mark Thompson
>> Sent: Tuesday, November 20, 2018 4:07 AM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add 
>> frame-skip func
>>
>> On 19/11/18 09:04, Jing SUN wrote:
>>> frame-skip is required to implement network bandwidth self-adaptive 
>>> vaapi encoding.
>>> To make a frame skipped, allocate its frame side data of 
>>> AV_FRAME_DATA_SKIP_FRAME type and set its value to 1.
>>
>> So if I'm reading this correctly the idea is to implement partial VFR by having a special new side-data type which indicates where frames would have been had the input actually matched the configured CFR behaviour?
>>
>> Why is the user meant to create these special frames?  It seems to me that the existing method of looking at the timestamps would be a better way to find any gaps.
>>
>> (Or, even better, add timestamps to VAAPI so that it can support VFR 
>> in a sensible way rather than adding hacks like this to allow partial 
>> VFR with weird constraints.)
>>
>>> Signed-off-by: Jing SUN <jing.a.sun@intel.com>
>>> ---
>>>  libavcodec/vaapi_encode.c | 142 ++++++++++++++++++++++++++++++++++++++++++++--
>>>  libavcodec/vaapi_encode.h |   5 ++
>>>  libavutil/frame.c         |   1 +
>>>  libavutil/frame.h         |   5 ++
>>>  4 files changed, 149 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c 
>>> index 2fe8501..a401d61 100644
>>> --- a/libavcodec/vaapi_encode.c
>>> +++ b/libavcodec/vaapi_encode.c
>>> @@ -23,6 +23,7 @@
>>>  #include "libavutil/common.h"
>>>  #include "libavutil/log.h"
>>>  #include "libavutil/pixdesc.h"
>>> +#include "libavutil/intreadwrite.h"
>>>  
>>>  #include "vaapi_encode.h"
>>>  #include "avcodec.h"
>>> @@ -103,6 +104,47 @@ static int vaapi_encode_make_param_buffer(AVCodecContext *avctx,
>>>      return 0;
>>>  }
>>>  
>>> +static int vaapi_encode_check_if_skip(AVCodecContext *avctx,
>>> +                                      VAAPIEncodePicture *pic) {
>>> +    AVFrameSideData *fside = NULL;
>>> +    VAAPIEncodeContext *ctx = avctx->priv_data;
>>> +    VAAPIEncodePicture *cur = NULL;
>>> +    int i = 0;
>>> +
>>> +    if (!pic || !pic->input_image)
>>> +        return AVERROR(EINVAL);
>>> +
>>> +    fside = av_frame_get_side_data(pic->input_image, AV_FRAME_DATA_SKIP_FRAME);
>>> +    if (fside)
>>> +        pic->skipped_flag = AV_RL8(fside->data);
>>> +    else
>>> +        pic->skipped_flag = 0;
>>> +
>>> +    if (0 == pic->skipped_flag)
>>> +        return 0;
>>> +
>>> +    if ((pic->type == PICTURE_TYPE_IDR) || (pic->type == PICTURE_TYPE_I)) {
>>> +        av_log(avctx, AV_LOG_INFO, "Can't skip IDR/I pic %"PRId64"/%"PRId64".\n",
>>> +               pic->display_order, pic->encode_order);
>>> +        pic->skipped_flag = 0;
>>> +        return 0;
>>> +    }
>>> +
>>> +    for (cur = ctx->pic_start; cur; cur = cur->next) {
>>> +        for (i=0; i < cur->nb_refs; ++i) {
>>> +            if (cur->refs[i] == pic) {
>>> +                av_log(avctx, AV_LOG_INFO, "Can't skip ref pic %"PRId64"/%"PRId64".\n",
>>> +                       pic->display_order, pic->encode_order);
>>> +                pic->skipped_flag = 0;
>>> +                return 0;
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>  static int vaapi_encode_wait(AVCodecContext *avctx,
>>>                               VAAPIEncodePicture *pic)  { @@ -418,6
>>> +460,69 @@ static int vaapi_encode_issue(AVCodecContext *avctx,
>>>          }
>>>      }
>>>  
>>> +    err = vaapi_encode_check_if_skip(avctx, pic);
>>> +    if (err != 0)
>>> +        av_log(avctx, AV_LOG_ERROR, "Fail to check if skip.\n");
>>> +
>>> +#ifdef VAEncMiscParameterSkipFrame
>>> +    if (pic->skipped_flag) {
>>> +        av_log(avctx, AV_LOG_INFO, "Skip pic %"PRId64"/%"PRId64" as requested.\n",
>>> +               pic->display_order, pic->encode_order);
>>> +
>>> +        ++ctx->skipped_pic_count;
>>> +        pic->encode_issued = 1;
>>> +
>>> +        return 0;
>>> +    } else if (ctx->skipped_pic_count > 0) {
>>> +        VABufferID skip_param_id;
>>> +        VAEncMiscParameterBuffer *misc_param;
>>> +        VAEncMiscParameterSkipFrame *skip_param;
>>> +
>>> +        err = vaapi_encode_make_param_buffer(avctx, pic,
>>> +                  VAEncMiscParameterBufferType, NULL,
>>> +                  (sizeof(VAEncMiscParameterBuffer) +
>>> +                  sizeof(VAEncMiscParameterSkipFrame)));
>>> +        if (err < 0)
>>> +            goto fail;
>>> +
>>> +        skip_param_id = pic->param_buffers[pic->nb_param_buffers-1];
>>> +
>>> +        vas = vaMapBuffer(ctx->hwctx->display,
>>> +                          skip_param_id,
>>> +                          (void **)&misc_param);
>>> +        if (vas != VA_STATUS_SUCCESS) {
>>> +            av_log(avctx, AV_LOG_ERROR, "Failed to map skip-frame buffer: "
>>> +                   "%d (%s).\n", vas, vaErrorStr(vas));
>>> +            err = AVERROR(EIO);
>>> +            goto fail;
>>> +        }
>>> +
>>> +        misc_param->type =
>>> + (VAEncMiscParameterType)VAEncMiscParameterTypeSkipFrame;
>>
>> You need to check VAConfigAttribEncSkipFrame to make sure this type is actually supported before using it.
>>
>>> +        skip_param = (VAEncMiscParameterSkipFrame *)misc_param->data;
>>> +        skip_param->skip_frame_flag = 1;
>>> +        skip_param->num_skip_frames = ctx->skipped_pic_count;
>>> +        skip_param->size_skip_frames = 0;
>>> +
>>> +        vas = vaUnmapBuffer(ctx->hwctx->display, skip_param_id);
>>> +        if (vas != VA_STATUS_SUCCESS) {
>>> +            av_log(avctx, AV_LOG_ERROR, "Failed to unmap skip-frame buffer: "
>>> +                   "%d (%s).\n", vas, vaErrorStr(vas));
>>> +            err = AVERROR(EIO);
>>> +            goto fail;
>>> +        }
>>
>> make_param_buffer can be called with the intended data directly - there is no need for this map/unmap.
>>
>>> +
>>> +        ctx->skipped_pic_count = 0;
>>> +    }
>>> +#else
>>> +    if (pic->skipped_flag) {
>>> +        av_log(avctx, AV_LOG_INFO, "Skip-frame isn't supported and pic %"PRId64"/%"PRId64" isn't skipped.\n",
>>> +               pic->display_order, pic->encode_order);
>>> +
>>> +        pic->skipped_flag = 0;
>>> +        ctx->skipped_pic_count = 0;
>>> +    }
>>> +#endif
>>> +
>>>      vas = vaBeginPicture(ctx->hwctx->display, ctx->va_context,
>>>                           pic->input_surface);
>>>      if (vas != VA_STATUS_SUCCESS) {
>>> @@ -500,9 +605,28 @@ static int vaapi_encode_output(AVCodecContext *avctx,
>>>      VAStatus vas;
>>>      int err;
>>>  
>>> -    err = vaapi_encode_wait(avctx, pic);
>>> -    if (err < 0)
>>> -        return err;
>>> +    if (!pic->skipped_flag) {
>>> +        err = vaapi_encode_wait(avctx, pic);
>>> +        if (err < 0)
>>> +            return err;
>>> +    } else {
>>> +        av_frame_free(&pic->input_image);
>>> +        pic->encode_complete = 1;
>>> +
>>> +        err = av_new_packet(pkt, 0);
>>> +        if (err < 0)
>>> +            goto fail;
>>> +
>>> +        pkt->pts = pic->pts;
>>> +
>>> +        av_buffer_unref(&pic->output_buffer_ref);
>>> +        pic->output_buffer = VA_INVALID_ID;
>>> +
>>> +        av_log(avctx, AV_LOG_DEBUG, "Output 0 byte for pic %"PRId64"/%"PRId64".\n",
>>> +               pic->display_order, pic->encode_order);
>>
>> Why is it helpful to create a zero-byte packet in this case?  Since no frame was encoded, I think no packet should be produced.
>>
>>> +
>>> +        return 0;
>>> +    }
>>>  
>>>      buf_list = NULL;
>>>      vas = vaMapBuffer(ctx->hwctx->display, pic->output_buffer, @@
>>> -523,6 +647,9 @@ static int vaapi_encode_output(AVCodecContext *avctx,
>>>              goto fail_mapped;
>>>  
>>>          memcpy(pkt->data, buf->buf, buf->size);
>>> +
>>> +        memset(buf->buf, 0, buf->size);
>>> +        buf->size = 0;
>>
>> Seems unrelated?
>>
>>>      }
>>>  
>>>      if (pic->type == PICTURE_TYPE_IDR) @@ -556,7 +683,12 @@ fail:
>>>  static int vaapi_encode_discard(AVCodecContext *avctx,
>>>                                  VAAPIEncodePicture *pic)  {
>>> -    vaapi_encode_wait(avctx, pic);
>>> +    if (!pic->skipped_flag) {
>>> +        vaapi_encode_wait(avctx, pic);
>>> +    } else {
>>> +        av_frame_free(&pic->input_image);
>>> +        pic->encode_complete = 1;
>>> +    }
>>>  
>>>      if (pic->output_buffer_ref) {
>>>          av_log(avctx, AV_LOG_DEBUG, "Discard output for pic "
>>> @@ -1994,6 +2126,8 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx)
>>>          }
>>>      }
>>>  
>>> +    ctx->skipped_pic_count = 0;
>>> +
>>>      return 0;
>>>  
>>>  fail:
>>> diff --git a/libavcodec/vaapi_encode.h b/libavcodec/vaapi_encode.h 
>>> index 965fe65..bcee0b6 100644
>>> --- a/libavcodec/vaapi_encode.h
>>> +++ b/libavcodec/vaapi_encode.h
>>> @@ -92,6 +92,8 @@ typedef struct VAAPIEncodePicture {
>>>  
>>>      int          nb_slices;
>>>      VAAPIEncodeSlice *slices;
>>> +
>>> +    uint8_t skipped_flag;
>>>  } VAAPIEncodePicture;
>>>  
>>>  typedef struct VAAPIEncodeProfile {
>>> @@ -246,6 +248,9 @@ typedef struct VAAPIEncodeContext {
>>>      int gop_counter;
>>>      int p_counter;
>>>      int end_of_stream;
>>> +
>>> +    // Skipped frame info
>>> +    unsigned int skipped_pic_count;
>>>  } VAAPIEncodeContext;
>>>  
>>>  enum {
>>> diff --git a/libavutil/frame.c b/libavutil/frame.c index
>>> 9b3fb13..c5fa205 100644
>>> --- a/libavutil/frame.c
>>> +++ b/libavutil/frame.c
>>> @@ -840,6 +840,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)
>>>      case AV_FRAME_DATA_QP_TABLE_PROPERTIES:         return "QP table properties";
>>>      case AV_FRAME_DATA_QP_TABLE_DATA:               return "QP table data";
>>>  #endif
>>> +    case AV_FRAME_DATA_SKIP_FRAME:                  return "Skip frame";
>>>      }
>>>      return NULL;
>>>  }
>>> diff --git a/libavutil/frame.h b/libavutil/frame.h index
>>> 66f27f4..8ef6475 100644
>>> --- a/libavutil/frame.h
>>> +++ b/libavutil/frame.h
>>> @@ -166,6 +166,11 @@ enum AVFrameSideDataType {
>>>       * function in libavutil/timecode.c.
>>>       */
>>>      AV_FRAME_DATA_S12M_TIMECODE,
>>> +
>>> +    /**
>>> +     * VAAPI Encode skip-frame indicator.
>>> +     */
>>> +    AV_FRAME_DATA_SKIP_FRAME,
>>>  };
>>>  
>>>  enum AVActiveFormatDescription {
>>>

Hi Mark,

Thanks for your kind guidance, but the problem is we are not trying to control the output framerate by this skip-frame feature. Our purpose is to just skip some frames, which are being requested by the content producer. And to implement that, we need an interface between the app and the vaapi lib, which informs the latter that a frame shall be skipped. 

Regards,
SUN, Jing


-----Original Message-----
From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Mark Thompson

Sent: Monday, December 10, 2018 2:40 AM
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add frame-skip func

On 06/12/2018 10:04, Sun, Jing A wrote:
> Hi Mark,

> 

> This patch is not trying to support VFR. Some frames, after which are just produced, could be considered as not needed by theirs producer and will get skipped in the encoding process. And in my opinion the existing timing information is not sufficient to support the case.


A skipped frame will still have a timestamp, so if a frame is skipped before the encoder then no frame with that timestamp will be given to the encoder.  For CFR content this can be detected in the encoder to reconstruct your skip-frames information exactly - a skip has occurred if the gap between two frames does not match the framerate, and the size of the gap tells you how many frames were skipped.  Avoiding a requirement that the gap sizes exactly match makes it implement a simple VFR scheme too, since skips can be inserted to keep the rate controller correct as long as you never have frames closer together than the configured framerate.

(Please avoid top-posting on this list.)

- Mark


> -----Original Message-----

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

> Of Mark Thompson

> Sent: Wednesday, December 5, 2018 7:50 AM

> To: ffmpeg-devel@ffmpeg.org

> Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add 

> frame-skip func

> 

> On 04/12/2018 01:46, Sun, Jing A wrote:

>> Hi Mark,

>>

>> Is there any issue that I need to fix for this patch please? 

> 

> See comments in the message you quoted below?  I think the most important point is that the existing timing information appears to contain all the information you want for VFR, so you probably shouldn't need the ad-hoc side-data type.

> 

> - Mark

> 

> 

>> -----Original Message-----

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

>> Of Sun, Jing A

>> Sent: Friday, November 23, 2018 5:37 PM

>> To: FFmpeg development discussions and patches 

>> <ffmpeg-devel@ffmpeg.org>

>> Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add 

>> frame-skip func

>>

>> Hi Mark,

>>

>> In some cases, that is useful. For example, an online content distributer, who keeps encoding the captured video frames by ffmpeg and sending them out. At times, there is no update of source, which makes one or several captured source frames are exactly the same as the last one, and the distributer wants to just skip such frames, without stopping the encoding process.

>>

>> Regards,

>> SUN, Jing

>>

>>

>> -----Original Message-----

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

>> Of Mark Thompson

>> Sent: Tuesday, November 20, 2018 4:07 AM

>> To: ffmpeg-devel@ffmpeg.org

>> Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add 

>> frame-skip func

>>

>> On 19/11/18 09:04, Jing SUN wrote:

>>> frame-skip is required to implement network bandwidth self-adaptive 

>>> vaapi encoding.

>>> To make a frame skipped, allocate its frame side data of 

>>> AV_FRAME_DATA_SKIP_FRAME type and set its value to 1.

>>

>> So if I'm reading this correctly the idea is to implement partial VFR by having a special new side-data type which indicates where frames would have been had the input actually matched the configured CFR behaviour?

>>

>> Why is the user meant to create these special frames?  It seems to me that the existing method of looking at the timestamps would be a better way to find any gaps.

>>

>> (Or, even better, add timestamps to VAAPI so that it can support VFR 

>> in a sensible way rather than adding hacks like this to allow partial 

>> VFR with weird constraints.)

>>

>>> Signed-off-by: Jing SUN <jing.a.sun@intel.com>

>>> ---

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

>>>  libavcodec/vaapi_encode.h |   5 ++

>>>  libavutil/frame.c         |   1 +

>>>  libavutil/frame.h         |   5 ++

>>>  4 files changed, 149 insertions(+), 4 deletions(-)

>>>

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

>>> index 2fe8501..a401d61 100644

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

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

>>> @@ -23,6 +23,7 @@

>>>  #include "libavutil/common.h"

>>>  #include "libavutil/log.h"

>>>  #include "libavutil/pixdesc.h"

>>> +#include "libavutil/intreadwrite.h"

>>>  

>>>  #include "vaapi_encode.h"

>>>  #include "avcodec.h"

>>> @@ -103,6 +104,47 @@ static int vaapi_encode_make_param_buffer(AVCodecContext *avctx,

>>>      return 0;

>>>  }

>>>  

>>> +static int vaapi_encode_check_if_skip(AVCodecContext *avctx,

>>> +                                      VAAPIEncodePicture *pic) {

>>> +    AVFrameSideData *fside = NULL;

>>> +    VAAPIEncodeContext *ctx = avctx->priv_data;

>>> +    VAAPIEncodePicture *cur = NULL;

>>> +    int i = 0;

>>> +

>>> +    if (!pic || !pic->input_image)

>>> +        return AVERROR(EINVAL);

>>> +

>>> +    fside = av_frame_get_side_data(pic->input_image, AV_FRAME_DATA_SKIP_FRAME);

>>> +    if (fside)

>>> +        pic->skipped_flag = AV_RL8(fside->data);

>>> +    else

>>> +        pic->skipped_flag = 0;

>>> +

>>> +    if (0 == pic->skipped_flag)

>>> +        return 0;

>>> +

>>> +    if ((pic->type == PICTURE_TYPE_IDR) || (pic->type == PICTURE_TYPE_I)) {

>>> +        av_log(avctx, AV_LOG_INFO, "Can't skip IDR/I pic %"PRId64"/%"PRId64".\n",

>>> +               pic->display_order, pic->encode_order);

>>> +        pic->skipped_flag = 0;

>>> +        return 0;

>>> +    }

>>> +

>>> +    for (cur = ctx->pic_start; cur; cur = cur->next) {

>>> +        for (i=0; i < cur->nb_refs; ++i) {

>>> +            if (cur->refs[i] == pic) {

>>> +                av_log(avctx, AV_LOG_INFO, "Can't skip ref pic %"PRId64"/%"PRId64".\n",

>>> +                       pic->display_order, pic->encode_order);

>>> +                pic->skipped_flag = 0;

>>> +                return 0;

>>> +            }

>>> +        }

>>> +    }

>>> +

>>> +    return 0;

>>> +}

>>> +

>>>  static int vaapi_encode_wait(AVCodecContext *avctx,

>>>                               VAAPIEncodePicture *pic)  { @@ -418,6

>>> +460,69 @@ static int vaapi_encode_issue(AVCodecContext *avctx,

>>>          }

>>>      }

>>>  

>>> +    err = vaapi_encode_check_if_skip(avctx, pic);

>>> +    if (err != 0)

>>> +        av_log(avctx, AV_LOG_ERROR, "Fail to check if skip.\n");

>>> +

>>> +#ifdef VAEncMiscParameterSkipFrame

>>> +    if (pic->skipped_flag) {

>>> +        av_log(avctx, AV_LOG_INFO, "Skip pic %"PRId64"/%"PRId64" as requested.\n",

>>> +               pic->display_order, pic->encode_order);

>>> +

>>> +        ++ctx->skipped_pic_count;

>>> +        pic->encode_issued = 1;

>>> +

>>> +        return 0;

>>> +    } else if (ctx->skipped_pic_count > 0) {

>>> +        VABufferID skip_param_id;

>>> +        VAEncMiscParameterBuffer *misc_param;

>>> +        VAEncMiscParameterSkipFrame *skip_param;

>>> +

>>> +        err = vaapi_encode_make_param_buffer(avctx, pic,

>>> +                  VAEncMiscParameterBufferType, NULL,

>>> +                  (sizeof(VAEncMiscParameterBuffer) +

>>> +                  sizeof(VAEncMiscParameterSkipFrame)));

>>> +        if (err < 0)

>>> +            goto fail;

>>> +

>>> +        skip_param_id = 

>>> + pic->param_buffers[pic->nb_param_buffers-1];

>>> +

>>> +        vas = vaMapBuffer(ctx->hwctx->display,

>>> +                          skip_param_id,

>>> +                          (void **)&misc_param);

>>> +        if (vas != VA_STATUS_SUCCESS) {

>>> +            av_log(avctx, AV_LOG_ERROR, "Failed to map skip-frame buffer: "

>>> +                   "%d (%s).\n", vas, vaErrorStr(vas));

>>> +            err = AVERROR(EIO);

>>> +            goto fail;

>>> +        }

>>> +

>>> +        misc_param->type =

>>> + (VAEncMiscParameterType)VAEncMiscParameterTypeSkipFrame;

>>

>> You need to check VAConfigAttribEncSkipFrame to make sure this type is actually supported before using it.

>>

>>> +        skip_param = (VAEncMiscParameterSkipFrame *)misc_param->data;

>>> +        skip_param->skip_frame_flag = 1;

>>> +        skip_param->num_skip_frames = ctx->skipped_pic_count;

>>> +        skip_param->size_skip_frames = 0;

>>> +

>>> +        vas = vaUnmapBuffer(ctx->hwctx->display, skip_param_id);

>>> +        if (vas != VA_STATUS_SUCCESS) {

>>> +            av_log(avctx, AV_LOG_ERROR, "Failed to unmap skip-frame buffer: "

>>> +                   "%d (%s).\n", vas, vaErrorStr(vas));

>>> +            err = AVERROR(EIO);

>>> +            goto fail;

>>> +        }

>>

>> make_param_buffer can be called with the intended data directly - there is no need for this map/unmap.

>>

>>> +

>>> +        ctx->skipped_pic_count = 0;

>>> +    }

>>> +#else

>>> +    if (pic->skipped_flag) {

>>> +        av_log(avctx, AV_LOG_INFO, "Skip-frame isn't supported and pic %"PRId64"/%"PRId64" isn't skipped.\n",

>>> +               pic->display_order, pic->encode_order);

>>> +

>>> +        pic->skipped_flag = 0;

>>> +        ctx->skipped_pic_count = 0;

>>> +    }

>>> +#endif

>>> +

>>>      vas = vaBeginPicture(ctx->hwctx->display, ctx->va_context,

>>>                           pic->input_surface);

>>>      if (vas != VA_STATUS_SUCCESS) { @@ -500,9 +605,28 @@ static int 

>>> vaapi_encode_output(AVCodecContext *avctx,

>>>      VAStatus vas;

>>>      int err;

>>>  

>>> -    err = vaapi_encode_wait(avctx, pic);

>>> -    if (err < 0)

>>> -        return err;

>>> +    if (!pic->skipped_flag) {

>>> +        err = vaapi_encode_wait(avctx, pic);

>>> +        if (err < 0)

>>> +            return err;

>>> +    } else {

>>> +        av_frame_free(&pic->input_image);

>>> +        pic->encode_complete = 1;

>>> +

>>> +        err = av_new_packet(pkt, 0);

>>> +        if (err < 0)

>>> +            goto fail;

>>> +

>>> +        pkt->pts = pic->pts;

>>> +

>>> +        av_buffer_unref(&pic->output_buffer_ref);

>>> +        pic->output_buffer = VA_INVALID_ID;

>>> +

>>> +        av_log(avctx, AV_LOG_DEBUG, "Output 0 byte for pic %"PRId64"/%"PRId64".\n",

>>> +               pic->display_order, pic->encode_order);

>>

>> Why is it helpful to create a zero-byte packet in this case?  Since no frame was encoded, I think no packet should be produced.

>>

>>> +

>>> +        return 0;

>>> +    }

>>>  

>>>      buf_list = NULL;

>>>      vas = vaMapBuffer(ctx->hwctx->display, pic->output_buffer, @@

>>> -523,6 +647,9 @@ static int vaapi_encode_output(AVCodecContext *avctx,

>>>              goto fail_mapped;

>>>  

>>>          memcpy(pkt->data, buf->buf, buf->size);

>>> +

>>> +        memset(buf->buf, 0, buf->size);

>>> +        buf->size = 0;

>>

>> Seems unrelated?

>>

>>>      }

>>>  

>>>      if (pic->type == PICTURE_TYPE_IDR) @@ -556,7 +683,12 @@ fail:

>>>  static int vaapi_encode_discard(AVCodecContext *avctx,

>>>                                  VAAPIEncodePicture *pic)  {

>>> -    vaapi_encode_wait(avctx, pic);

>>> +    if (!pic->skipped_flag) {

>>> +        vaapi_encode_wait(avctx, pic);

>>> +    } else {

>>> +        av_frame_free(&pic->input_image);

>>> +        pic->encode_complete = 1;

>>> +    }

>>>  

>>>      if (pic->output_buffer_ref) {

>>>          av_log(avctx, AV_LOG_DEBUG, "Discard output for pic "

>>> @@ -1994,6 +2126,8 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx)

>>>          }

>>>      }

>>>  

>>> +    ctx->skipped_pic_count = 0;

>>> +

>>>      return 0;

>>>  

>>>  fail:

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

>>> index 965fe65..bcee0b6 100644

>>> --- a/libavcodec/vaapi_encode.h

>>> +++ b/libavcodec/vaapi_encode.h

>>> @@ -92,6 +92,8 @@ typedef struct VAAPIEncodePicture {

>>>  

>>>      int          nb_slices;

>>>      VAAPIEncodeSlice *slices;

>>> +

>>> +    uint8_t skipped_flag;

>>>  } VAAPIEncodePicture;

>>>  

>>>  typedef struct VAAPIEncodeProfile { @@ -246,6 +248,9 @@ typedef 

>>> struct VAAPIEncodeContext {

>>>      int gop_counter;

>>>      int p_counter;

>>>      int end_of_stream;

>>> +

>>> +    // Skipped frame info

>>> +    unsigned int skipped_pic_count;

>>>  } VAAPIEncodeContext;

>>>  

>>>  enum {

>>> diff --git a/libavutil/frame.c b/libavutil/frame.c index

>>> 9b3fb13..c5fa205 100644

>>> --- a/libavutil/frame.c

>>> +++ b/libavutil/frame.c

>>> @@ -840,6 +840,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)

>>>      case AV_FRAME_DATA_QP_TABLE_PROPERTIES:         return "QP table properties";

>>>      case AV_FRAME_DATA_QP_TABLE_DATA:               return "QP table data";

>>>  #endif

>>> +    case AV_FRAME_DATA_SKIP_FRAME:                  return "Skip frame";

>>>      }

>>>      return NULL;

>>>  }

>>> diff --git a/libavutil/frame.h b/libavutil/frame.h index

>>> 66f27f4..8ef6475 100644

>>> --- a/libavutil/frame.h

>>> +++ b/libavutil/frame.h

>>> @@ -166,6 +166,11 @@ enum AVFrameSideDataType {

>>>       * function in libavutil/timecode.c.

>>>       */

>>>      AV_FRAME_DATA_S12M_TIMECODE,

>>> +

>>> +    /**

>>> +     * VAAPI Encode skip-frame indicator.

>>> +     */

>>> +    AV_FRAME_DATA_SKIP_FRAME,

>>>  };

>>>  

>>>  enum AVActiveFormatDescription {

>>>

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Hi Mark,

Such APP controlled frame-skipping feature is required by our vaapi lib users. Could we get it merged please?

Regards,
SUN, Jing

-----Original Message-----
From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Sun, Jing A

Sent: Monday, December 10, 2018 3:19 PM
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add frame-skip func

Hi Mark,

Thanks for your kind guidance, but the problem is we are not trying to control the output framerate by this skip-frame feature. Our purpose is to just skip some frames, which are being requested by the content producer. And to implement that, we need an interface between the app and the vaapi lib, which informs the latter that a frame shall be skipped. 

Regards,
SUN, Jing


-----Original Message-----
From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Mark Thompson

Sent: Monday, December 10, 2018 2:40 AM
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add frame-skip func

On 06/12/2018 10:04, Sun, Jing A wrote:
> Hi Mark,

> 

> This patch is not trying to support VFR. Some frames, after which are just produced, could be considered as not needed by theirs producer and will get skipped in the encoding process. And in my opinion the existing timing information is not sufficient to support the case.


A skipped frame will still have a timestamp, so if a frame is skipped before the encoder then no frame with that timestamp will be given to the encoder.  For CFR content this can be detected in the encoder to reconstruct your skip-frames information exactly - a skip has occurred if the gap between two frames does not match the framerate, and the size of the gap tells you how many frames were skipped.  Avoiding a requirement that the gap sizes exactly match makes it implement a simple VFR scheme too, since skips can be inserted to keep the rate controller correct as long as you never have frames closer together than the configured framerate.

(Please avoid top-posting on this list.)

- Mark


> -----Original Message-----

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

> Of Mark Thompson

> Sent: Wednesday, December 5, 2018 7:50 AM

> To: ffmpeg-devel@ffmpeg.org

> Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add 

> frame-skip func

> 

> On 04/12/2018 01:46, Sun, Jing A wrote:

>> Hi Mark,

>>

>> Is there any issue that I need to fix for this patch please? 

> 

> See comments in the message you quoted below?  I think the most important point is that the existing timing information appears to contain all the information you want for VFR, so you probably shouldn't need the ad-hoc side-data type.

> 

> - Mark

> 

> 

>> -----Original Message-----

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

>> Of Sun, Jing A

>> Sent: Friday, November 23, 2018 5:37 PM

>> To: FFmpeg development discussions and patches 

>> <ffmpeg-devel@ffmpeg.org>

>> Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add 

>> frame-skip func

>>

>> Hi Mark,

>>

>> In some cases, that is useful. For example, an online content distributer, who keeps encoding the captured video frames by ffmpeg and sending them out. At times, there is no update of source, which makes one or several captured source frames are exactly the same as the last one, and the distributer wants to just skip such frames, without stopping the encoding process.

>>

>> Regards,

>> SUN, Jing

>>

>>

>> -----Original Message-----

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

>> Of Mark Thompson

>> Sent: Tuesday, November 20, 2018 4:07 AM

>> To: ffmpeg-devel@ffmpeg.org

>> Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add 

>> frame-skip func

>>

>> On 19/11/18 09:04, Jing SUN wrote:

>>> frame-skip is required to implement network bandwidth self-adaptive 

>>> vaapi encoding.

>>> To make a frame skipped, allocate its frame side data of 

>>> AV_FRAME_DATA_SKIP_FRAME type and set its value to 1.

>>

>> So if I'm reading this correctly the idea is to implement partial VFR by having a special new side-data type which indicates where frames would have been had the input actually matched the configured CFR behaviour?

>>

>> Why is the user meant to create these special frames?  It seems to me that the existing method of looking at the timestamps would be a better way to find any gaps.

>>

>> (Or, even better, add timestamps to VAAPI so that it can support VFR 

>> in a sensible way rather than adding hacks like this to allow partial 

>> VFR with weird constraints.)

>>

>>> Signed-off-by: Jing SUN <jing.a.sun@intel.com>

>>> ---

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

>>>  libavcodec/vaapi_encode.h |   5 ++

>>>  libavutil/frame.c         |   1 +

>>>  libavutil/frame.h         |   5 ++

>>>  4 files changed, 149 insertions(+), 4 deletions(-)

>>>

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

>>> index 2fe8501..a401d61 100644

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

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

>>> @@ -23,6 +23,7 @@

>>>  #include "libavutil/common.h"

>>>  #include "libavutil/log.h"

>>>  #include "libavutil/pixdesc.h"

>>> +#include "libavutil/intreadwrite.h"

>>>  

>>>  #include "vaapi_encode.h"

>>>  #include "avcodec.h"

>>> @@ -103,6 +104,47 @@ static int vaapi_encode_make_param_buffer(AVCodecContext *avctx,

>>>      return 0;

>>>  }

>>>  

>>> +static int vaapi_encode_check_if_skip(AVCodecContext *avctx,

>>> +                                      VAAPIEncodePicture *pic) {

>>> +    AVFrameSideData *fside = NULL;

>>> +    VAAPIEncodeContext *ctx = avctx->priv_data;

>>> +    VAAPIEncodePicture *cur = NULL;

>>> +    int i = 0;

>>> +

>>> +    if (!pic || !pic->input_image)

>>> +        return AVERROR(EINVAL);

>>> +

>>> +    fside = av_frame_get_side_data(pic->input_image, AV_FRAME_DATA_SKIP_FRAME);

>>> +    if (fside)

>>> +        pic->skipped_flag = AV_RL8(fside->data);

>>> +    else

>>> +        pic->skipped_flag = 0;

>>> +

>>> +    if (0 == pic->skipped_flag)

>>> +        return 0;

>>> +

>>> +    if ((pic->type == PICTURE_TYPE_IDR) || (pic->type == PICTURE_TYPE_I)) {

>>> +        av_log(avctx, AV_LOG_INFO, "Can't skip IDR/I pic %"PRId64"/%"PRId64".\n",

>>> +               pic->display_order, pic->encode_order);

>>> +        pic->skipped_flag = 0;

>>> +        return 0;

>>> +    }

>>> +

>>> +    for (cur = ctx->pic_start; cur; cur = cur->next) {

>>> +        for (i=0; i < cur->nb_refs; ++i) {

>>> +            if (cur->refs[i] == pic) {

>>> +                av_log(avctx, AV_LOG_INFO, "Can't skip ref pic %"PRId64"/%"PRId64".\n",

>>> +                       pic->display_order, pic->encode_order);

>>> +                pic->skipped_flag = 0;

>>> +                return 0;

>>> +            }

>>> +        }

>>> +    }

>>> +

>>> +    return 0;

>>> +}

>>> +

>>>  static int vaapi_encode_wait(AVCodecContext *avctx,

>>>                               VAAPIEncodePicture *pic)  { @@ -418,6

>>> +460,69 @@ static int vaapi_encode_issue(AVCodecContext *avctx,

>>>          }

>>>      }

>>>  

>>> +    err = vaapi_encode_check_if_skip(avctx, pic);

>>> +    if (err != 0)

>>> +        av_log(avctx, AV_LOG_ERROR, "Fail to check if skip.\n");

>>> +

>>> +#ifdef VAEncMiscParameterSkipFrame

>>> +    if (pic->skipped_flag) {

>>> +        av_log(avctx, AV_LOG_INFO, "Skip pic %"PRId64"/%"PRId64" as requested.\n",

>>> +               pic->display_order, pic->encode_order);

>>> +

>>> +        ++ctx->skipped_pic_count;

>>> +        pic->encode_issued = 1;

>>> +

>>> +        return 0;

>>> +    } else if (ctx->skipped_pic_count > 0) {

>>> +        VABufferID skip_param_id;

>>> +        VAEncMiscParameterBuffer *misc_param;

>>> +        VAEncMiscParameterSkipFrame *skip_param;

>>> +

>>> +        err = vaapi_encode_make_param_buffer(avctx, pic,

>>> +                  VAEncMiscParameterBufferType, NULL,

>>> +                  (sizeof(VAEncMiscParameterBuffer) +

>>> +                  sizeof(VAEncMiscParameterSkipFrame)));

>>> +        if (err < 0)

>>> +            goto fail;

>>> +

>>> +        skip_param_id =

>>> + pic->param_buffers[pic->nb_param_buffers-1];

>>> +

>>> +        vas = vaMapBuffer(ctx->hwctx->display,

>>> +                          skip_param_id,

>>> +                          (void **)&misc_param);

>>> +        if (vas != VA_STATUS_SUCCESS) {

>>> +            av_log(avctx, AV_LOG_ERROR, "Failed to map skip-frame buffer: "

>>> +                   "%d (%s).\n", vas, vaErrorStr(vas));

>>> +            err = AVERROR(EIO);

>>> +            goto fail;

>>> +        }

>>> +

>>> +        misc_param->type =

>>> + (VAEncMiscParameterType)VAEncMiscParameterTypeSkipFrame;

>>

>> You need to check VAConfigAttribEncSkipFrame to make sure this type is actually supported before using it.

>>

>>> +        skip_param = (VAEncMiscParameterSkipFrame *)misc_param->data;

>>> +        skip_param->skip_frame_flag = 1;

>>> +        skip_param->num_skip_frames = ctx->skipped_pic_count;

>>> +        skip_param->size_skip_frames = 0;

>>> +

>>> +        vas = vaUnmapBuffer(ctx->hwctx->display, skip_param_id);

>>> +        if (vas != VA_STATUS_SUCCESS) {

>>> +            av_log(avctx, AV_LOG_ERROR, "Failed to unmap skip-frame buffer: "

>>> +                   "%d (%s).\n", vas, vaErrorStr(vas));

>>> +            err = AVERROR(EIO);

>>> +            goto fail;

>>> +        }

>>

>> make_param_buffer can be called with the intended data directly - there is no need for this map/unmap.

>>

>>> +

>>> +        ctx->skipped_pic_count = 0;

>>> +    }

>>> +#else

>>> +    if (pic->skipped_flag) {

>>> +        av_log(avctx, AV_LOG_INFO, "Skip-frame isn't supported and pic %"PRId64"/%"PRId64" isn't skipped.\n",

>>> +               pic->display_order, pic->encode_order);

>>> +

>>> +        pic->skipped_flag = 0;

>>> +        ctx->skipped_pic_count = 0;

>>> +    }

>>> +#endif

>>> +

>>>      vas = vaBeginPicture(ctx->hwctx->display, ctx->va_context,

>>>                           pic->input_surface);

>>>      if (vas != VA_STATUS_SUCCESS) { @@ -500,9 +605,28 @@ static int 

>>> vaapi_encode_output(AVCodecContext *avctx,

>>>      VAStatus vas;

>>>      int err;

>>>  

>>> -    err = vaapi_encode_wait(avctx, pic);

>>> -    if (err < 0)

>>> -        return err;

>>> +    if (!pic->skipped_flag) {

>>> +        err = vaapi_encode_wait(avctx, pic);

>>> +        if (err < 0)

>>> +            return err;

>>> +    } else {

>>> +        av_frame_free(&pic->input_image);

>>> +        pic->encode_complete = 1;

>>> +

>>> +        err = av_new_packet(pkt, 0);

>>> +        if (err < 0)

>>> +            goto fail;

>>> +

>>> +        pkt->pts = pic->pts;

>>> +

>>> +        av_buffer_unref(&pic->output_buffer_ref);

>>> +        pic->output_buffer = VA_INVALID_ID;

>>> +

>>> +        av_log(avctx, AV_LOG_DEBUG, "Output 0 byte for pic %"PRId64"/%"PRId64".\n",

>>> +               pic->display_order, pic->encode_order);

>>

>> Why is it helpful to create a zero-byte packet in this case?  Since no frame was encoded, I think no packet should be produced.

>>

>>> +

>>> +        return 0;

>>> +    }

>>>  

>>>      buf_list = NULL;

>>>      vas = vaMapBuffer(ctx->hwctx->display, pic->output_buffer, @@

>>> -523,6 +647,9 @@ static int vaapi_encode_output(AVCodecContext *avctx,

>>>              goto fail_mapped;

>>>  

>>>          memcpy(pkt->data, buf->buf, buf->size);

>>> +

>>> +        memset(buf->buf, 0, buf->size);

>>> +        buf->size = 0;

>>

>> Seems unrelated?

>>

>>>      }

>>>  

>>>      if (pic->type == PICTURE_TYPE_IDR) @@ -556,7 +683,12 @@ fail:

>>>  static int vaapi_encode_discard(AVCodecContext *avctx,

>>>                                  VAAPIEncodePicture *pic)  {

>>> -    vaapi_encode_wait(avctx, pic);

>>> +    if (!pic->skipped_flag) {

>>> +        vaapi_encode_wait(avctx, pic);

>>> +    } else {

>>> +        av_frame_free(&pic->input_image);

>>> +        pic->encode_complete = 1;

>>> +    }

>>>  

>>>      if (pic->output_buffer_ref) {

>>>          av_log(avctx, AV_LOG_DEBUG, "Discard output for pic "

>>> @@ -1994,6 +2126,8 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx)

>>>          }

>>>      }

>>>  

>>> +    ctx->skipped_pic_count = 0;

>>> +

>>>      return 0;

>>>  

>>>  fail:

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

>>> index 965fe65..bcee0b6 100644

>>> --- a/libavcodec/vaapi_encode.h

>>> +++ b/libavcodec/vaapi_encode.h

>>> @@ -92,6 +92,8 @@ typedef struct VAAPIEncodePicture {

>>>  

>>>      int          nb_slices;

>>>      VAAPIEncodeSlice *slices;

>>> +

>>> +    uint8_t skipped_flag;

>>>  } VAAPIEncodePicture;

>>>  

>>>  typedef struct VAAPIEncodeProfile { @@ -246,6 +248,9 @@ typedef 

>>> struct VAAPIEncodeContext {

>>>      int gop_counter;

>>>      int p_counter;

>>>      int end_of_stream;

>>> +

>>> +    // Skipped frame info

>>> +    unsigned int skipped_pic_count;

>>>  } VAAPIEncodeContext;

>>>  

>>>  enum {

>>> diff --git a/libavutil/frame.c b/libavutil/frame.c index

>>> 9b3fb13..c5fa205 100644

>>> --- a/libavutil/frame.c

>>> +++ b/libavutil/frame.c

>>> @@ -840,6 +840,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)

>>>      case AV_FRAME_DATA_QP_TABLE_PROPERTIES:         return "QP table properties";

>>>      case AV_FRAME_DATA_QP_TABLE_DATA:               return "QP table data";

>>>  #endif

>>> +    case AV_FRAME_DATA_SKIP_FRAME:                  return "Skip frame";

>>>      }

>>>      return NULL;

>>>  }

>>> diff --git a/libavutil/frame.h b/libavutil/frame.h index

>>> 66f27f4..8ef6475 100644

>>> --- a/libavutil/frame.h

>>> +++ b/libavutil/frame.h

>>> @@ -166,6 +166,11 @@ enum AVFrameSideDataType {

>>>       * function in libavutil/timecode.c.

>>>       */

>>>      AV_FRAME_DATA_S12M_TIMECODE,

>>> +

>>> +    /**

>>> +     * VAAPI Encode skip-frame indicator.

>>> +     */

>>> +    AV_FRAME_DATA_SKIP_FRAME,

>>>  };

>>>  

>>>  enum AVActiveFormatDescription {

>>>

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Hi Mark,

May I have your comments on this please?

Regards,
SUN, Jing

-----Original Message-----
From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Sun, Jing A

Sent: Tuesday, December 18, 2018 4:23 PM
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add frame-skip func

Hi Mark,

Such APP controlled frame-skipping feature is required by our vaapi lib users. Could we get it merged please?

Regards,
SUN, Jing

-----Original Message-----
From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Sun, Jing A

Sent: Monday, December 10, 2018 3:19 PM
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add frame-skip func

Hi Mark,

Thanks for your kind guidance, but the problem is we are not trying to control the output framerate by this skip-frame feature. Our purpose is to just skip some frames, which are being requested by the content producer. And to implement that, we need an interface between the app and the vaapi lib, which informs the latter that a frame shall be skipped. 

Regards,
SUN, Jing


-----Original Message-----
From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Mark Thompson

Sent: Monday, December 10, 2018 2:40 AM
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add frame-skip func

On 06/12/2018 10:04, Sun, Jing A wrote:
> Hi Mark,

> 

> This patch is not trying to support VFR. Some frames, after which are just produced, could be considered as not needed by theirs producer and will get skipped in the encoding process. And in my opinion the existing timing information is not sufficient to support the case.


A skipped frame will still have a timestamp, so if a frame is skipped before the encoder then no frame with that timestamp will be given to the encoder.  For CFR content this can be detected in the encoder to reconstruct your skip-frames information exactly - a skip has occurred if the gap between two frames does not match the framerate, and the size of the gap tells you how many frames were skipped.  Avoiding a requirement that the gap sizes exactly match makes it implement a simple VFR scheme too, since skips can be inserted to keep the rate controller correct as long as you never have frames closer together than the configured framerate.

(Please avoid top-posting on this list.)

- Mark


> -----Original Message-----

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

> Of Mark Thompson

> Sent: Wednesday, December 5, 2018 7:50 AM

> To: ffmpeg-devel@ffmpeg.org

> Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add 

> frame-skip func

> 

> On 04/12/2018 01:46, Sun, Jing A wrote:

>> Hi Mark,

>>

>> Is there any issue that I need to fix for this patch please? 

> 

> See comments in the message you quoted below?  I think the most important point is that the existing timing information appears to contain all the information you want for VFR, so you probably shouldn't need the ad-hoc side-data type.

> 

> - Mark

> 

> 

>> -----Original Message-----

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

>> Of Sun, Jing A

>> Sent: Friday, November 23, 2018 5:37 PM

>> To: FFmpeg development discussions and patches 

>> <ffmpeg-devel@ffmpeg.org>

>> Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add 

>> frame-skip func

>>

>> Hi Mark,

>>

>> In some cases, that is useful. For example, an online content distributer, who keeps encoding the captured video frames by ffmpeg and sending them out. At times, there is no update of source, which makes one or several captured source frames are exactly the same as the last one, and the distributer wants to just skip such frames, without stopping the encoding process.

>>

>> Regards,

>> SUN, Jing

>>

>>

>> -----Original Message-----

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

>> Of Mark Thompson

>> Sent: Tuesday, November 20, 2018 4:07 AM

>> To: ffmpeg-devel@ffmpeg.org

>> Subject: Re: [FFmpeg-devel] [PATCH 1/1] avcodec/vaapi_encode: add 

>> frame-skip func

>>

>> On 19/11/18 09:04, Jing SUN wrote:

>>> frame-skip is required to implement network bandwidth self-adaptive 

>>> vaapi encoding.

>>> To make a frame skipped, allocate its frame side data of 

>>> AV_FRAME_DATA_SKIP_FRAME type and set its value to 1.

>>

>> So if I'm reading this correctly the idea is to implement partial VFR by having a special new side-data type which indicates where frames would have been had the input actually matched the configured CFR behaviour?

>>

>> Why is the user meant to create these special frames?  It seems to me that the existing method of looking at the timestamps would be a better way to find any gaps.

>>

>> (Or, even better, add timestamps to VAAPI so that it can support VFR 

>> in a sensible way rather than adding hacks like this to allow partial 

>> VFR with weird constraints.)

>>

>>> Signed-off-by: Jing SUN <jing.a.sun@intel.com>

>>> ---

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

>>>  libavcodec/vaapi_encode.h |   5 ++

>>>  libavutil/frame.c         |   1 +

>>>  libavutil/frame.h         |   5 ++

>>>  4 files changed, 149 insertions(+), 4 deletions(-)

>>>

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

>>> index 2fe8501..a401d61 100644

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

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

>>> @@ -23,6 +23,7 @@

>>>  #include "libavutil/common.h"

>>>  #include "libavutil/log.h"

>>>  #include "libavutil/pixdesc.h"

>>> +#include "libavutil/intreadwrite.h"

>>>  

>>>  #include "vaapi_encode.h"

>>>  #include "avcodec.h"

>>> @@ -103,6 +104,47 @@ static int vaapi_encode_make_param_buffer(AVCodecContext *avctx,

>>>      return 0;

>>>  }

>>>  

>>> +static int vaapi_encode_check_if_skip(AVCodecContext *avctx,

>>> +                                      VAAPIEncodePicture *pic) {

>>> +    AVFrameSideData *fside = NULL;

>>> +    VAAPIEncodeContext *ctx = avctx->priv_data;

>>> +    VAAPIEncodePicture *cur = NULL;

>>> +    int i = 0;

>>> +

>>> +    if (!pic || !pic->input_image)

>>> +        return AVERROR(EINVAL);

>>> +

>>> +    fside = av_frame_get_side_data(pic->input_image, AV_FRAME_DATA_SKIP_FRAME);

>>> +    if (fside)

>>> +        pic->skipped_flag = AV_RL8(fside->data);

>>> +    else

>>> +        pic->skipped_flag = 0;

>>> +

>>> +    if (0 == pic->skipped_flag)

>>> +        return 0;

>>> +

>>> +    if ((pic->type == PICTURE_TYPE_IDR) || (pic->type == PICTURE_TYPE_I)) {

>>> +        av_log(avctx, AV_LOG_INFO, "Can't skip IDR/I pic %"PRId64"/%"PRId64".\n",

>>> +               pic->display_order, pic->encode_order);

>>> +        pic->skipped_flag = 0;

>>> +        return 0;

>>> +    }

>>> +

>>> +    for (cur = ctx->pic_start; cur; cur = cur->next) {

>>> +        for (i=0; i < cur->nb_refs; ++i) {

>>> +            if (cur->refs[i] == pic) {

>>> +                av_log(avctx, AV_LOG_INFO, "Can't skip ref pic %"PRId64"/%"PRId64".\n",

>>> +                       pic->display_order, pic->encode_order);

>>> +                pic->skipped_flag = 0;

>>> +                return 0;

>>> +            }

>>> +        }

>>> +    }

>>> +

>>> +    return 0;

>>> +}

>>> +

>>>  static int vaapi_encode_wait(AVCodecContext *avctx,

>>>                               VAAPIEncodePicture *pic)  { @@ -418,6

>>> +460,69 @@ static int vaapi_encode_issue(AVCodecContext *avctx,

>>>          }

>>>      }

>>>  

>>> +    err = vaapi_encode_check_if_skip(avctx, pic);

>>> +    if (err != 0)

>>> +        av_log(avctx, AV_LOG_ERROR, "Fail to check if skip.\n");

>>> +

>>> +#ifdef VAEncMiscParameterSkipFrame

>>> +    if (pic->skipped_flag) {

>>> +        av_log(avctx, AV_LOG_INFO, "Skip pic %"PRId64"/%"PRId64" as requested.\n",

>>> +               pic->display_order, pic->encode_order);

>>> +

>>> +        ++ctx->skipped_pic_count;

>>> +        pic->encode_issued = 1;

>>> +

>>> +        return 0;

>>> +    } else if (ctx->skipped_pic_count > 0) {

>>> +        VABufferID skip_param_id;

>>> +        VAEncMiscParameterBuffer *misc_param;

>>> +        VAEncMiscParameterSkipFrame *skip_param;

>>> +

>>> +        err = vaapi_encode_make_param_buffer(avctx, pic,

>>> +                  VAEncMiscParameterBufferType, NULL,

>>> +                  (sizeof(VAEncMiscParameterBuffer) +

>>> +                  sizeof(VAEncMiscParameterSkipFrame)));

>>> +        if (err < 0)

>>> +            goto fail;

>>> +

>>> +        skip_param_id =

>>> + pic->param_buffers[pic->nb_param_buffers-1];

>>> +

>>> +        vas = vaMapBuffer(ctx->hwctx->display,

>>> +                          skip_param_id,

>>> +                          (void **)&misc_param);

>>> +        if (vas != VA_STATUS_SUCCESS) {

>>> +            av_log(avctx, AV_LOG_ERROR, "Failed to map skip-frame buffer: "

>>> +                   "%d (%s).\n", vas, vaErrorStr(vas));

>>> +            err = AVERROR(EIO);

>>> +            goto fail;

>>> +        }

>>> +

>>> +        misc_param->type =

>>> + (VAEncMiscParameterType)VAEncMiscParameterTypeSkipFrame;

>>

>> You need to check VAConfigAttribEncSkipFrame to make sure this type is actually supported before using it.

>>

>>> +        skip_param = (VAEncMiscParameterSkipFrame *)misc_param->data;

>>> +        skip_param->skip_frame_flag = 1;

>>> +        skip_param->num_skip_frames = ctx->skipped_pic_count;

>>> +        skip_param->size_skip_frames = 0;

>>> +

>>> +        vas = vaUnmapBuffer(ctx->hwctx->display, skip_param_id);

>>> +        if (vas != VA_STATUS_SUCCESS) {

>>> +            av_log(avctx, AV_LOG_ERROR, "Failed to unmap skip-frame buffer: "

>>> +                   "%d (%s).\n", vas, vaErrorStr(vas));

>>> +            err = AVERROR(EIO);

>>> +            goto fail;

>>> +        }

>>

>> make_param_buffer can be called with the intended data directly - there is no need for this map/unmap.

>>

>>> +

>>> +        ctx->skipped_pic_count = 0;

>>> +    }

>>> +#else

>>> +    if (pic->skipped_flag) {

>>> +        av_log(avctx, AV_LOG_INFO, "Skip-frame isn't supported and pic %"PRId64"/%"PRId64" isn't skipped.\n",

>>> +               pic->display_order, pic->encode_order);

>>> +

>>> +        pic->skipped_flag = 0;

>>> +        ctx->skipped_pic_count = 0;

>>> +    }

>>> +#endif

>>> +

>>>      vas = vaBeginPicture(ctx->hwctx->display, ctx->va_context,

>>>                           pic->input_surface);

>>>      if (vas != VA_STATUS_SUCCESS) { @@ -500,9 +605,28 @@ static int 

>>> vaapi_encode_output(AVCodecContext *avctx,

>>>      VAStatus vas;

>>>      int err;

>>>  

>>> -    err = vaapi_encode_wait(avctx, pic);

>>> -    if (err < 0)

>>> -        return err;

>>> +    if (!pic->skipped_flag) {

>>> +        err = vaapi_encode_wait(avctx, pic);

>>> +        if (err < 0)

>>> +            return err;

>>> +    } else {

>>> +        av_frame_free(&pic->input_image);

>>> +        pic->encode_complete = 1;

>>> +

>>> +        err = av_new_packet(pkt, 0);

>>> +        if (err < 0)

>>> +            goto fail;

>>> +

>>> +        pkt->pts = pic->pts;

>>> +

>>> +        av_buffer_unref(&pic->output_buffer_ref);

>>> +        pic->output_buffer = VA_INVALID_ID;

>>> +

>>> +        av_log(avctx, AV_LOG_DEBUG, "Output 0 byte for pic %"PRId64"/%"PRId64".\n",

>>> +               pic->display_order, pic->encode_order);

>>

>> Why is it helpful to create a zero-byte packet in this case?  Since no frame was encoded, I think no packet should be produced.

>>

>>> +

>>> +        return 0;

>>> +    }

>>>  

>>>      buf_list = NULL;

>>>      vas = vaMapBuffer(ctx->hwctx->display, pic->output_buffer, @@

>>> -523,6 +647,9 @@ static int vaapi_encode_output(AVCodecContext *avctx,

>>>              goto fail_mapped;

>>>  

>>>          memcpy(pkt->data, buf->buf, buf->size);

>>> +

>>> +        memset(buf->buf, 0, buf->size);

>>> +        buf->size = 0;

>>

>> Seems unrelated?

>>

>>>      }

>>>  

>>>      if (pic->type == PICTURE_TYPE_IDR) @@ -556,7 +683,12 @@ fail:

>>>  static int vaapi_encode_discard(AVCodecContext *avctx,

>>>                                  VAAPIEncodePicture *pic)  {

>>> -    vaapi_encode_wait(avctx, pic);

>>> +    if (!pic->skipped_flag) {

>>> +        vaapi_encode_wait(avctx, pic);

>>> +    } else {

>>> +        av_frame_free(&pic->input_image);

>>> +        pic->encode_complete = 1;

>>> +    }

>>>  

>>>      if (pic->output_buffer_ref) {

>>>          av_log(avctx, AV_LOG_DEBUG, "Discard output for pic "

>>> @@ -1994,6 +2126,8 @@ av_cold int ff_vaapi_encode_init(AVCodecContext *avctx)

>>>          }

>>>      }

>>>  

>>> +    ctx->skipped_pic_count = 0;

>>> +

>>>      return 0;

>>>  

>>>  fail:

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

>>> index 965fe65..bcee0b6 100644

>>> --- a/libavcodec/vaapi_encode.h

>>> +++ b/libavcodec/vaapi_encode.h

>>> @@ -92,6 +92,8 @@ typedef struct VAAPIEncodePicture {

>>>  

>>>      int          nb_slices;

>>>      VAAPIEncodeSlice *slices;

>>> +

>>> +    uint8_t skipped_flag;

>>>  } VAAPIEncodePicture;

>>>  

>>>  typedef struct VAAPIEncodeProfile { @@ -246,6 +248,9 @@ typedef 

>>> struct VAAPIEncodeContext {

>>>      int gop_counter;

>>>      int p_counter;

>>>      int end_of_stream;

>>> +

>>> +    // Skipped frame info

>>> +    unsigned int skipped_pic_count;

>>>  } VAAPIEncodeContext;

>>>  

>>>  enum {

>>> diff --git a/libavutil/frame.c b/libavutil/frame.c index

>>> 9b3fb13..c5fa205 100644

>>> --- a/libavutil/frame.c

>>> +++ b/libavutil/frame.c

>>> @@ -840,6 +840,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)

>>>      case AV_FRAME_DATA_QP_TABLE_PROPERTIES:         return "QP table properties";

>>>      case AV_FRAME_DATA_QP_TABLE_DATA:               return "QP table data";

>>>  #endif

>>> +    case AV_FRAME_DATA_SKIP_FRAME:                  return "Skip frame";

>>>      }

>>>      return NULL;

>>>  }

>>> diff --git a/libavutil/frame.h b/libavutil/frame.h index

>>> 66f27f4..8ef6475 100644

>>> --- a/libavutil/frame.h

>>> +++ b/libavutil/frame.h

>>> @@ -166,6 +166,11 @@ enum AVFrameSideDataType {

>>>       * function in libavutil/timecode.c.

>>>       */

>>>      AV_FRAME_DATA_S12M_TIMECODE,

>>> +

>>> +    /**

>>> +     * VAAPI Encode skip-frame indicator.

>>> +     */

>>> +    AV_FRAME_DATA_SKIP_FRAME,

>>>  };

>>>  

>>>  enum AVActiveFormatDescription {

>>>

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel