[FFmpeg-devel] [PATCH v2 5/6] vaapi_encode: Add ROI support

Mark Thompson sw at jkqxz.net
Wed Mar 13 11:43:56 EET 2019


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.

>> +
>> +            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, &param_misc, sizeof(param_misc));
>> +        memcpy(param_buffer + sizeof(param_misc),
>> +               &param_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.)

>>
>>      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



More information about the ffmpeg-devel mailing list