> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf > Of Mark Thompson > Sent: Wednesday, March 13, 2019 5:44 PM > To: ffmpeg-devel@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-boun...@ffmpeg.org] On > Behalf > >> Of Mark Thompson > >> Sent: Wednesday, March 13, 2019 8:18 AM > >> To: ffmpeg-devel@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel