Re: [FFmpeg-devel] [PATCH v2 5/6] vaapi_encode: Add ROI support
> -Original Message-
> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
> Of Mark Thompson
> Sent: Wednesday, March 13, 2019 08:18
> 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;
How about using packed attribute to cope with this? Like:
struct {
VAEncMiscParameterBuffer misc;
VAEncMiscParameterBufferROI param_roi;
} __attribute__((packed)) mfs_params;
This happens a lot during vaapi feature development, like
MaxFrameSize:
struct {
VAEncMiscParameterBuffer misc;
VAEncMiscParameterBufferMultiPassFrameSize mfs;
} __attribute__((packed)) mfs_params;
Trellis: (currently waiting for bug fix in libva)
https://patchwork.ffmpeg.org/patch/11733/
Thanks,
Linjie
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2 5/6] vaapi_encode: Add ROI support
> -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
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.
>> +
>> +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_DEB
Re: [FFmpeg-devel] [PATCH v2 5/6] vaapi_encode: Add ROI support
> -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.
> +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?
> +for (i = 0; i < nb_roi; i++) {
> +roi = (const AVRegionOfInterest*)(sd->data + roi->self_size * i);
same comment as libx264
> +
> +v = roi->qoffset.num * ctx->roi_quant_range / roi->qoffset.den;
shall we check here if roi->qoffset-den is zero?
> +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, _misc, sizeof(param_misc));
> +
[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);
+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;
+}
+for (i = 0; i < nb_roi; i++) {
+roi = (const AVRegionOfInterest*)(sd->data + roi->self_size * i);
+
+v = roi->qoffset.num * ctx->roi_quant_range / roi->qoffset.den;
+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, _misc, sizeof(param_misc));
+memcpy(param_buffer + sizeof(param_misc),
+ _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(>codec_picture_params);
av_freep(>param_buffers);
av_freep(>slices);
+av_freep(>roi);
av_frame_free(>recon_image);
av_buffer_unref(>output_buffer_ref);
pic->output_buffer = VA_INVALID_ID;
@@ -611,6 +698,7 @@ static int vaapi_encode_free(AVCodecContext *avctx,
av_freep(>priv_data);
av_freep(>codec_picture_params);
+av_freep(>roi);
av_free(pic);
@@
