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-boun...@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-boun...@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....@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
