[FFmpeg-devel] [PATCH v2 5/6] vaapi_encode: Add ROI support
Guo, Yejun
yejun.guo at intel.com
Thu Mar 14 07:57:21 EET 2019
> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Mark Thompson
> Sent: Wednesday, March 13, 2019 5:44 PM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v2 5/6] vaapi_encode: Add ROI support
>
> On 13/03/2019 02:46, Guo, Yejun wrote:
> >> -----Original Message-----
> >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
> Behalf
> >> Of Mark Thompson
> >> Sent: Wednesday, March 13, 2019 8:18 AM
> >> To: ffmpeg-devel at ffmpeg.org
> >> Subject: [FFmpeg-devel] [PATCH v2 5/6] vaapi_encode: Add ROI support
> >>
> >> ---
> >> libavcodec/vaapi_encode.c | 128
> >> ++++++++++++++++++++++++++++++++
> >> libavcodec/vaapi_encode.h | 11 +++
> >> libavcodec/vaapi_encode_h264.c | 2 +
> >> libavcodec/vaapi_encode_h265.c | 2 +
> >> libavcodec/vaapi_encode_mpeg2.c | 2 +
> >> libavcodec/vaapi_encode_vp8.c | 2 +
> >> libavcodec/vaapi_encode_vp9.c | 2 +
> >> 7 files changed, 149 insertions(+)
> >>
> >> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> >> index 2dda451882..03a7f5ce3e 100644
> >> --- a/libavcodec/vaapi_encode.c
> >> +++ b/libavcodec/vaapi_encode.c
> >> @@ -143,6 +143,7 @@ static int vaapi_encode_issue(AVCodecContext
> >> *avctx,
> >> int err, i;
> >> char data[MAX_PARAM_BUFFER_SIZE];
> >> size_t bit_len;
> >> + AVFrameSideData *sd;
> >>
> >> av_log(avctx, AV_LOG_DEBUG, "Issuing encode for
> >> pic %"PRId64"/%"PRId64" "
> >> "as type %s.\n", pic->display_order, pic->encode_order,
> >> @@ -412,6 +413,91 @@ static int vaapi_encode_issue(AVCodecContext
> >> *avctx,
> >> }
> >> }
> >>
> >> + sd = av_frame_get_side_data(pic->input_image,
> >> + AV_FRAME_DATA_REGIONS_OF_INTEREST);
> >> +
> >> +#if VA_CHECK_VERSION(1, 0, 0)
> >> + if (sd && ctx->roi_allowed) {
> >> + const AVRegionOfInterest *roi;
> >> + int nb_roi;
> >> + VAEncMiscParameterBuffer param_misc = {
> >> + .type = VAEncMiscParameterTypeROI,
> >> + };
> >> + VAEncMiscParameterBufferROI param_roi;
> >> + // VAAPI requires the second structure to immediately follow the
> >> + // first in the parameter buffer, but they have different natural
> >> + // alignment on 64-bit architectures (4 vs. 8, since the ROI
> >> + // structure contains a pointer). This means we can't make a
> >> + // single working structure, nor can we make the buffer
> >> + // separately and map it because dereferencing the second pointer
> >> + // would be undefined. Therefore, construct the two parts
> >> + // separately and combine them into a single character buffer
> >> + // before uploading.
> >> + char param_buffer[sizeof(param_misc) + sizeof(param_roi)];
> >> + int i, v;
> >> +
> >> + roi = (const AVRegionOfInterest*)sd->data;
> >> + av_assert0(roi->self_size && sd->size % roi->self_size == 0);
> >
> > it is possible if the user forget to set the value of roi->self_size.
> > assert is not feasible.
>
> Not setting self_size violates the API contract ("Must be set to the size of this
> data structure") and therefore invokes undefined behaviour. Aborting when
> undefined behaviour is first detected is the correct response.
>
> >> + nb_roi = sd->size / roi->self_size;
> >> + if (nb_roi > ctx->roi_regions) {
> >> + if (!ctx->roi_warned) {
> >> + av_log(avctx, AV_LOG_WARNING, "More ROIs set than "
> >> + "supported by driver (%d > %d).\n",
> >> + nb_roi, ctx->roi_regions);
> >> + ctx->roi_warned = 1;
> >> + }
> >> + nb_roi = ctx->roi_regions;
> >> + }
> >> +
> >> + pic->roi = av_mallocz_array(nb_roi, sizeof(*pic->roi));
> >> + if (!pic->roi) {
> >> + err = AVERROR(ENOMEM);
> >> + goto fail;
> >> + }
> >
> > shall we add comments here to explain the list visit order?
>
> Sure, added.
>
> >> + for (i = 0; i < nb_roi; i++) {
> >> + roi = (const AVRegionOfInterest*)(sd->data + roi->self_size * i);
> >
> > same comment as libx264
>
> I guess, though I'm not sure filling the code with guards detecting undefined
> behaviour cases really has any value.
>
> I've added an additional note on the API that the value must be the same on
> all entries.
I personally prefer code check and report error, anyway, I do not insist it.
>
> >> +
> >> + v = roi->qoffset.num * ctx->roi_quant_range / roi->qoffset.den;
> >
> > shall we check here if roi->qoffset-den is zero?
>
> Sure, I'll add an assert for the invalid fraction since the API requires [-1,+1].
>
> >> + av_log(avctx, AV_LOG_DEBUG, "ROI region: (%d,%d) (%dx%d) -
> >>> %+d.\n",
> >> + roi->x, roi->y, roi->width, roi->height, v);
> >> +
> >> + pic->roi[i] = (VAEncROI) {
> >> + .roi_rectangle = {
> >> + .x = roi->x,
> >> + .y = roi->y,
> >> + .width = roi->width,
> >> + .height = roi->height,
> >> + },
> >> + .roi_value = av_clip_c(v, INT8_MIN, INT8_MAX),
> >> + };
> >> + }
> >> +
> >> + param_roi = (VAEncMiscParameterBufferROI) {
> >> + .num_roi = nb_roi,
> >> + .max_delta_qp = INT8_MAX,
> >> + .min_delta_qp = 0,
> >> + .roi = pic->roi,
> >> + .roi_flags.bits.roi_value_is_qp_delta = 1,
> >> + };
> >> +
> >> + memcpy(param_buffer, ¶m_misc, sizeof(param_misc));
> >> + memcpy(param_buffer + sizeof(param_misc),
> >> + ¶m_roi, sizeof(param_roi));
> >> +
> >> + err = vaapi_encode_make_param_buffer(avctx, pic,
> >> + VAEncMiscParameterBufferType,
> >> + param_buffer,
> >> + sizeof(param_buffer));
> >> + if (err < 0)
> >> + goto fail;
> >> + } else
> >> +#endif
> >> + if (sd && !ctx->roi_warned) {
> >> + av_log(avctx, AV_LOG_WARNING, "ROI side data on input "
> >> + "frames ignored due to lack of driver support.\n");
> >> + ctx->roi_warned = 1;
> >> + }
> >> +
> >> vas = vaBeginPicture(ctx->hwctx->display, ctx->va_context,
> >> pic->input_surface);
> >> if (vas != VA_STATUS_SUCCESS) {
> >> @@ -477,6 +563,7 @@ fail_at_end:
> >> av_freep(&pic->codec_picture_params);
> >> av_freep(&pic->param_buffers);
> >> av_freep(&pic->slices);
> >> + av_freep(&pic->roi);
> >> av_frame_free(&pic->recon_image);
> >> av_buffer_unref(&pic->output_buffer_ref);
> >> pic->output_buffer = VA_INVALID_ID;
> >> @@ -611,6 +698,7 @@ static int vaapi_encode_free(AVCodecContext
> *avctx,
> >>
> >> av_freep(&pic->priv_data);
> >> av_freep(&pic->codec_picture_params);
> >> + av_freep(&pic->roi);
> >
> > it is unnecessary since it is already freed in function vaapi_encode_issue.
>
> Not if the issue succeeds. (Indeed, it can't be in that case because the
> encode might be asynchronous.)
if the encoder is asynchronous, the av_freep(&pic->roi) in function vaapi_encode_issue is wrong,
it might free the memory too early. I don't see a mechanism in av_freep that will wait
for the asynchronous model, it just free the memory immediately.
we can just remove av_freep(&pic->roi) in function vaapi_encode_issue.
>
> >>
> >> av_free(pic);
> >>
> >> @@ -1899,6 +1987,42 @@ static av_cold int
> >> vaapi_encode_init_quality(AVCodecContext *avctx)
> >> return 0;
> >> }
> >>
> >> +static av_cold int vaapi_encode_init_roi(AVCodecContext *avctx)
> >> +{
> >> + VAAPIEncodeContext *ctx = avctx->priv_data;
> >> +
> >> +#if VA_CHECK_VERSION(1, 0, 0)
> >> + VAStatus vas;
> >> + VAConfigAttrib attr = { VAConfigAttribEncROI };
> >> +
> >> + vas = vaGetConfigAttributes(ctx->hwctx->display,
> >> + ctx->va_profile,
> >> + ctx->va_entrypoint,
> >> + &attr, 1);
> >> + if (vas != VA_STATUS_SUCCESS) {
> >> + av_log(avctx, AV_LOG_ERROR, "Failed to query ROI "
> >> + "config attribute: %d (%s).\n", vas, vaErrorStr(vas));
> >> + return AVERROR_EXTERNAL;
> >> + }
> >> +
> >> + if (attr.value == VA_ATTRIB_NOT_SUPPORTED) {
> >> + ctx->roi_allowed = 0;
> >> + } else {
> >> + VAConfigAttribValEncROI roi = {
> >> + .value = attr.value,
> >> + };
> >> +
> >> + ctx->roi_regions = roi.bits.num_roi_regions;
> >> + ctx->roi_allowed = ctx->roi_regions > 0 &&
> >> + (ctx->va_rc_mode == VA_RC_CQP ||
> >> + roi.bits.roi_rc_qp_delta_support);
> >> + }
> >> +#else
> >> + ctx->roi_allowed = 0;
> >
> > it is unnecessary since it is never checked out of the #if body
>
> I guess, removed.
>
> >> +#endif
> >> + return 0;
> >> +}
> >> +
> >> static void vaapi_encode_free_output_buffer(void *opaque,
> >> uint8_t *data)
> >> {
> >> @@ -2089,6 +2213,10 @@ av_cold int
> >> ff_vaapi_encode_init(AVCodecContext *avctx)
> >> if (err < 0)
> >> goto fail;
> >>
> >> + err = vaapi_encode_init_roi(avctx);
> >> + if (err < 0)
> >> + goto fail;
> >> +
> >> if (avctx->compression_level >= 0) {
> >> err = vaapi_encode_init_quality(avctx);
> >> if (err < 0)
> >> diff --git a/libavcodec/vaapi_encode.h b/libavcodec/vaapi_encode.h
> >> index 44a8db566e..ee074b4dc1 100644
> >> --- a/libavcodec/vaapi_encode.h
> >> +++ b/libavcodec/vaapi_encode.h
> >> @@ -69,6 +69,11 @@ typedef struct VAAPIEncodePicture {
> >> int64_t pts;
> >> int force_idr;
> >>
> >> +#if VA_CHECK_VERSION(1, 0, 0)
> >> + // ROI regions.
> >> + VAEncROI *roi;
> >> +#endif
> >> +
> >> int type;
> >> int b_depth;
> >> int encode_issued;
> >> @@ -314,6 +319,12 @@ typedef struct VAAPIEncodeContext {
> >> int idr_counter;
> >> int gop_counter;
> >> int end_of_stream;
> >> +
> >> + // ROI state.
> >> + int roi_allowed;
> >> + int roi_regions;
> >
> > it might be more straight forward if rename it to roi_max_regions.
> > anyway, both are ok.
>
> Yes, I agree; changed.
>
> >> + int roi_quant_range;
> >> + int roi_warned;
> >> } VAAPIEncodeContext;
> >>
> >> enum {
> >> diff --git a/libavcodec/vaapi_encode_h264.c
> >> b/libavcodec/vaapi_encode_h264.c
> >> index 91be33f99f..7c55b8a4a7 100644
> >> --- a/libavcodec/vaapi_encode_h264.c
> >> +++ b/libavcodec/vaapi_encode_h264.c
> >> @@ -1123,6 +1123,8 @@ static av_cold int
> >> vaapi_encode_h264_configure(AVCodecContext *avctx)
> >> }
> >> }
> >>
> >> + ctx->roi_quant_range = 51 + 6 * (ctx->profile->depth - 8);
> >> +
> >> return 0;
> >> }
> >>
> >> diff --git a/libavcodec/vaapi_encode_h265.c
> >> b/libavcodec/vaapi_encode_h265.c
> >> index 758bd40a37..538862a9d5 100644
> >> --- a/libavcodec/vaapi_encode_h265.c
> >> +++ b/libavcodec/vaapi_encode_h265.c
> >> @@ -1102,6 +1102,8 @@ static av_cold int
> >> vaapi_encode_h265_configure(AVCodecContext *avctx)
> >> priv->fixed_qp_b = 30;
> >> }
> >>
> >> + ctx->roi_quant_range = 51 + 6 * (ctx->profile->depth - 8);
> >> +
> >> return 0;
> >> }
> >>
> >> diff --git a/libavcodec/vaapi_encode_mpeg2.c
> >> b/libavcodec/vaapi_encode_mpeg2.c
> >> index fb1ef71fdc..bac9ea1fa6 100644
> >> --- a/libavcodec/vaapi_encode_mpeg2.c
> >> +++ b/libavcodec/vaapi_encode_mpeg2.c
> >> @@ -552,6 +552,8 @@ static av_cold int
> >> vaapi_encode_mpeg2_configure(AVCodecContext *avctx)
> >> ctx->nb_slices = ctx->slice_block_rows;
> >> ctx->slice_size = 1;
> >>
> >> + ctx->roi_quant_range = 31;
> >> +
> >> return 0;
> >> }
> >>
> >> diff --git a/libavcodec/vaapi_encode_vp8.c
> >> b/libavcodec/vaapi_encode_vp8.c
> >> index ddbe4c9075..6e7bf9d106 100644
> >> --- a/libavcodec/vaapi_encode_vp8.c
> >> +++ b/libavcodec/vaapi_encode_vp8.c
> >> @@ -173,6 +173,8 @@ static av_cold int
> >> vaapi_encode_vp8_configure(AVCodecContext *avctx)
> >> else
> >> priv->q_index_i = priv->q_index_p;
> >>
> >> + ctx->roi_quant_range = VP8_MAX_QUANT;
> >> +
> >> return 0;
> >> }
> >>
> >> diff --git a/libavcodec/vaapi_encode_vp9.c
> >> b/libavcodec/vaapi_encode_vp9.c
> >> index f89fd0d07a..d7f415d704 100644
> >> --- a/libavcodec/vaapi_encode_vp9.c
> >> +++ b/libavcodec/vaapi_encode_vp9.c
> >> @@ -202,6 +202,8 @@ static av_cold int
> >> vaapi_encode_vp9_configure(AVCodecContext *avctx)
> >> priv->q_idx_idr = priv->q_idx_p = priv->q_idx_b = 100;
> >> }
> >>
> >> + ctx->roi_quant_range = VP9_MAX_QUANT;
> >> +
> >> return 0;
> >> }
> >>
> >> --
> >> 2.19.2
> >>
>
> Thanks,
>
> - Mark
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
More information about the ffmpeg-devel
mailing list