[Libva] [PATCH v2] Don't automatically destroy the buffer(s) passed to vaRenderPicture
Instead the user must call vaDestroyBuffer() to destroy a buffer explicitly.
If following the previous API specification,
1. Violate "who allocate who release" principle
2. The user cannot re-use VA buffer flexibly
3. The user still has to call vaDestroyBuffer() to destroy the buffers which
are not going to be passed to vaRenderPicture()
We discussed the change at https://bugs.freedesktop.org/show_bug.cgi?id=97970
v2: bump version to 0.40 because this is a incompatible change to VA-API
Signed-off-by: Xiang, Haihao
---
configure.ac | 6 +++---
va/va.h | 9 +++--
va/va_enc_h264.h | 3 +--
va/va_vpp.h | 12 +++-
4 files changed, 14 insertions(+), 16 deletions(-)
diff --git a/configure.ac b/configure.ac
index 6617a0e..dd0c611 100644
--- a/configure.ac
+++ b/configure.ac
@@ -27,8 +27,8 @@
# - reset micro version to zero when minor version is incremented
# - reset minor version to zero when major version is incremented
m4_define([va_api_major_version], [0])
-m4_define([va_api_minor_version], [39])
-m4_define([va_api_micro_version], [4])
+m4_define([va_api_minor_version], [40])
+m4_define([va_api_micro_version], [0])
m4_define([va_api_version],
[va_api_major_version.va_api_minor_version.va_api_micro_version])
@@ -42,7 +42,7 @@ m4_define([va_api_version],
# - reset micro version to zero when VA-API major or minor version is changed
m4_define([libva_major_version], [m4_eval(va_api_major_version + 1)])
m4_define([libva_minor_version], [m4_eval(va_api_minor_version - 32)])
-m4_define([libva_micro_version], [4])
+m4_define([libva_micro_version], [0])
m4_define([libva_pre_version], [1])
m4_define([libva_version],
diff --git a/va/va.h b/va/va.h
index 8791906..86bef97 100644
--- a/va/va.h
+++ b/va/va.h
@@ -2156,6 +2156,7 @@ typedef struct _VAEncPictureParameterBufferMPEG4
* eliminates this copy is to pass null as "data" when calling
vaCreateBuffer(),
* and then use vaMapBuffer() to map the data store from the server side to the
* client address space for access.
+ * The user must call vaDestroyBuffer() to destroy a buffer.
* Note: image buffers are created by the library, not the client. Please see
*vaCreateImage on how image buffers are managed.
*/
@@ -2276,7 +2277,12 @@ VAStatus vaUnmapBuffer (
/**
* After this call, the buffer is deleted and this buffer_id is no longer valid
- * Only call this if the buffer is not going to be passed to vaRenderBuffer
+ *
+ * A buffer can be re-used and sent to the server by another Begin/Render/End
+ * sequence if vaDestroyBuffer() is not called with this buffer.
+ *
+ * Note re-using a shared buffer (e.g. a slice data buffer) between the host
and the
+ * hardware accelerator can result in performance dropping.
*/
VAStatus vaDestroyBuffer (
VADisplay dpy,
@@ -2404,7 +2410,6 @@ VAStatus vaBeginPicture (
/**
* Send decode buffers to the server.
- * Buffers are automatically destroyed afterwards
*/
VAStatus vaRenderPicture (
VADisplay dpy,
diff --git a/va/va_enc_h264.h b/va/va_enc_h264.h
index 604877f..2e7eb8d 100644
--- a/va/va_enc_h264.h
+++ b/va/va_enc_h264.h
@@ -386,8 +386,7 @@ typedef struct _VAEncPictureParameterBufferH264 {
*
* If per-macroblock encoder configuration is needed, \c macroblock_info
* references a buffer of type #VAEncMacroblockParameterBufferH264. This
- * buffer is not passed to vaRenderPicture(). i.e. it is not destroyed
- * by subsequent calls to vaRenderPicture() and then can be re-used
+ * buffer is not passed to vaRenderPicture() and it can be re-used
* without re-allocating the whole buffer.
*/
typedef struct _VAEncSliceParameterBufferH264 {
diff --git a/va/va_vpp.h b/va/va_vpp.h
index 8ac0923..082f1d5 100644
--- a/va/va_vpp.h
+++ b/va/va_vpp.h
@@ -371,11 +371,9 @@ typedef struct _VAProcFilterValueRange {
/**
* \brief Video processing pipeline configuration.
*
- * This buffer defines a video processing pipeline. As for any buffer
- * passed to \c vaRenderPicture(), this is a one-time usage model.
- * However, the actual filters to be applied are provided in the
- * \c filters field, so they can be re-used in other processing
- * pipelines.
+ * This buffer defines a video processing pipeline. The actual filters to
+ * be applied are provided in the \c filters field, they can be re-used
+ * in other processing pipelines.
*
* The target surface is specified by the \c render_target argument of
* \c vaBeginPicture(). The general usage model is described as follows:
@@ -491,10 +489,6 @@ typedef struct _VAProcPipelineParameterBuffer {
* #VA_STATUS_ERROR_UNSUPPORTED_FILTER is returned if the list
* contains an unsupported filter.
*
- * Note: no filter buffer is destroyed after a call to vaRenderPicture(),
- * only this pipeline buffer will be destroyed as per the core API
- * specification. This allows for flexibility in re-using the filter for
- * other surfaces to be processed.
*/
V
[Libva] [Libva-intel-driver][PATCH 1/2] AVC encoder: use generic ROI parameters
Presently ROI parameters are stored in the common structure, each
codec can use these parameters.
Signed-off-by: Xiang, Haihao
---
src/gen6_mfc_common.c | 60 +--
1 file changed, 20 insertions(+), 40 deletions(-)
diff --git a/src/gen6_mfc_common.c b/src/gen6_mfc_common.c
index 15c0637..aca161a 100644
--- a/src/gen6_mfc_common.c
+++ b/src/gen6_mfc_common.c
@@ -1810,12 +1810,10 @@ typedef struct {
static VAStatus
intel_h264_enc_roi_cbr(VADriverContextP ctx,
int base_qp,
- VAEncMiscParameterBufferROI *pMiscParamROI,
struct encode_state *encode_state,
struct intel_encoder_context *encoder_context)
{
int nonroi_qp;
-VAEncROI *region_roi;
bool quickfill = 0;
ROIRegionParam param_regions[I965_MAX_NUM_ROI_REGIONS];
@@ -1835,17 +1833,14 @@ intel_h264_enc_roi_cbr(VADriverContextP ctx,
struct gen6_vme_context *vme_context = encoder_context->vme_context;
VAStatus vaStatus = VA_STATUS_SUCCESS;
-if(pMiscParamROI != NULL)
-{
-num_roi = (pMiscParamROI->num_roi > I965_MAX_NUM_ROI_REGIONS) ?
I965_MAX_NUM_ROI_REGIONS : pMiscParamROI->num_roi;
-
-/* currently roi_value_is_qp_delta is the only supported mode of
priority.
-*
-* qp_delta set by user is added to base_qp, which is then clapped by
-* [base_qp-min_delta, base_qp+max_delta].
-*/
-
ASSERT_RET(pMiscParamROI->roi_flags.bits.roi_value_is_qp_delta,VA_STATUS_ERROR_INVALID_PARAMETER);
-}
+/* currently roi_value_is_qp_delta is the only supported mode of priority.
+ *
+ * qp_delta set by user is added to base_qp, which is then clapped by
+ * [base_qp-min_delta, base_qp+max_delta].
+ */
+ASSERT_RET(encoder_context->brc.roi_value_is_qp_delta,
VA_STATUS_ERROR_INVALID_PARAMETER);
+
+num_roi = encoder_context->brc.num_roi;
/* when the base_qp is lower than 12, the quality is quite good based
* on the H264 test experience.
@@ -1866,12 +1861,11 @@ intel_h264_enc_roi_cbr(VADriverContextP ctx,
int roi_qp;
float qstep_roi;
-region_roi = (VAEncROI *)pMiscParamROI->roi + i;
+col_start = encoder_context->brc.roi[i].left;
+col_end = encoder_context->brc.roi[i].right;
+row_start = encoder_context->brc.roi[i].top;
+row_end = encoder_context->brc.roi[i].bottom;
-col_start = region_roi->roi_rectangle.x;
-col_end = col_start + region_roi->roi_rectangle.width;
-row_start = region_roi->roi_rectangle.y;
-row_end = row_start + region_roi->roi_rectangle.height;
col_start = col_start / 16;
col_end = (col_end + 15) / 16;
row_start = row_start / 16;
@@ -1888,7 +1882,7 @@ intel_h264_enc_roi_cbr(VADriverContextP ctx,
param_regions[i].width_mbs = roi_width_mbs;
param_regions[i].height_mbs = roi_height_mbs;
-roi_qp = base_qp + region_roi->roi_value;
+roi_qp = base_qp + encoder_context->brc.roi[i].value;
BRC_CLIP(roi_qp, 1, 51);
param_regions[i].roi_qp = roi_qp;
@@ -1935,10 +1929,7 @@ intel_h264_enc_roi_config(VADriverContextP ctx,
{
char *qp_ptr;
int i, j;
-VAEncROI *region_roi;
struct i965_driver_data *i965 = i965_driver_data(ctx);
-VAEncMiscParameterBuffer* pMiscParamROI;
-VAEncMiscParameterBufferROI *pParamROI = NULL;
struct gen6_vme_context *vme_context = encoder_context->vme_context;
struct gen6_mfc_context *mfc_context = encoder_context->mfc_context;
VAEncSequenceParameterBufferH264 *pSequenceParameter =
(VAEncSequenceParameterBufferH264 *)encode_state->seq_param_ext->buffer;
@@ -1953,16 +1944,7 @@ intel_h264_enc_roi_config(VADriverContextP ctx,
if (!encoder_context->context_roi || (encode_state->num_slice_params_ext >
1))
return;
-if (encode_state->misc_param[VAEncMiscParameterTypeROI][0] != NULL) {
-pMiscParamROI =
(VAEncMiscParameterBuffer*)encode_state->misc_param[VAEncMiscParameterTypeROI][0]->buffer;
-pParamROI = (VAEncMiscParameterBufferROI *)pMiscParamROI->data;
-
-/* check whether number of ROI is correct */
-num_roi = (pParamROI->num_roi > I965_MAX_NUM_ROI_REGIONS) ?
I965_MAX_NUM_ROI_REGIONS : pParamROI->num_roi;
-}
-
-if (num_roi > 0)
-vme_context->roi_enabled = 1;
+vme_context->roi_enabled = !!encoder_context->brc.num_roi;
if (!vme_context->roi_enabled)
return;
@@ -1986,7 +1968,7 @@ intel_h264_enc_roi_config(VADriverContextP ctx,
int slice_type =
intel_avc_enc_slice_type_fixup(slice_param->slice_type);
qp =
mfc_context->brc.qp_prime_y[encoder_context->layer.curr_frame_layer_id][slice_type];
-intel_h264_enc_roi_cbr(ctx, qp, pParamROI,encode_state,
encoder_context);
+intel_h264_enc_roi_cbr(ctx, qp, encode_state, encoder_context);
} else if (encoder_c
[Libva] [Libva-intel-driver][PATCH 2/2] Encoder: release all misc parameter buffers
User can still use the old setting if needed because the setting is
stored in a common structure now.
Signed-off-by: Xiang, Haihao
---
src/i965_drv_video.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/src/i965_drv_video.c b/src/i965_drv_video.c
index 51a708c..76cb915 100644
--- a/src/i965_drv_video.c
+++ b/src/i965_drv_video.c
@@ -2798,7 +2798,7 @@ i965_BeginPicture(VADriverContextP ctx,
struct object_surface *obj_surface = SURFACE(render_target);
struct object_config *obj_config;
VAStatus vaStatus = VA_STATUS_SUCCESS;
-int i;
+int i, j;
ASSERT_RET(obj_context, VA_STATUS_ERROR_INVALID_CONTEXT);
ASSERT_RET(obj_surface, VA_STATUS_ERROR_INVALID_SURFACE);
@@ -2841,15 +2841,10 @@ i965_BeginPicture(VADriverContextP ctx,
obj_context->codec_state.encode.num_packed_header_data_ext = 0;
obj_context->codec_state.encode.slice_index = 0;
obj_context->codec_state.encode.vps_sps_seq_index = 0;
-/*
-* Based on ROI definition in va/va.h, the ROI set through this
-* structure is applicable only to the current frame or field.
-* That is to say: it is on-the-fly setting. If it is not set,
-* the current frame doesn't use ROI.
-* It is uncertain whether the other misc buffer should be released.
-* So only release the previous ROI buffer.
-*/
-
i965_release_buffer_store(&obj_context->codec_state.encode.misc_param[VAEncMiscParameterTypeROI][0]);
+
+for (i = 0; i <
ARRAY_ELEMS(obj_context->codec_state.encode.misc_param); i++)
+for (j = 0; j <
ARRAY_ELEMS(obj_context->codec_state.encode.misc_param[0]); j++)
+
i965_release_buffer_store(&obj_context->codec_state.encode.misc_param[i][j]);
i965_release_buffer_store(&obj_context->codec_state.encode.encmb_map);
} else {
--
1.9.1
___
Libva mailing list
Libva@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libva
Re: [Libva] [Libva-intel-driver][PATCH] Needn't reset brc if the bitrate setting isn't changed in the Begin/Render/End sequence
On 03/01/17 06:42, Xiang, Haihao wrote: > On Fri, 2016-12-30 at 17:44 +, Mark Thompson wrote: >> On 30/12/16 00:51, Xiang, Haihao wrote: >>> On Thu, 2016-12-29 at 18:52 +, Mark Thompson wrote: On 29/12/16 07:09, Zhao Yakui wrote: > On 12/29/2016 03:08 PM, Xiang, Haihao wrote: >> >>> On 12/29/2016 01:22 PM, Xiang, Haihao wrote: User can use VAEncMiscParameterRateControl to update bitrate, so we should ignore the bitrate in the sequence parameter if VAEncMiscParameterRateControl is present. >>> >>> This makes sense. The MiscRateControl mentions that it can >>> override >>> the >>> bps setting in sequence parameter. >>> Hence it is not needed to reset brc if VAEncMiscParameterRateControl doesn't change the used bitrate. >>> >>> Does it still need to send the >>> VAEncMiscParameterRateControl >>> parameter >>> if the bps is not changed for the new sequence? >> >> Yes if bits_per_second in the sequence parameter is not equal >> to >> the >> value for the previous Begin/Render/End sequence except >> bits_per_second >> in the sequence parameter is 0. >> > > OK. > It makes sense. > > This looks good to me. I'm not sure this behaviour is correct. Should not the most recently given bitrate value from the user, either from sequence parameters or RC parameters, be used? When is_new_sequence is set, the user will have provided a set of sequence parameters (because we are making an IDR frame), so the bitrate there is provided by the user and should override any previous RC parameters given before that frame (but not any on this one). That is, I think it should work like: Sequence parameters with bps = 1M -> IDR frame, at 1Mbps -> some P frames, at 1Mbps RC parameters with bps = 2M -> some P frames, at 2Mbps Sequence parameters with bps = 3M -> IDR frame, at 3Mbps -> some P frames, at 3Mbps >>> >>> >>> The 2nd sequence is generated at 3Mbps with the current >>> logic. hl_bitrate_updated is 0 for your case and we use the >>> sequence >>> parameters. >> >> This is not the case - in the absence of a new RC parameter >> structure, the one that was set in the middle of the first sequence >> continues to apply forever (encoder_context->misc_param[] is never >> cleared), so hl_bitrate_updated is always 1 after that point. >> > > You are right, I forgot encoder_context->misc_param[] > (except VAEncMiscParameterTypeROI) is not cleared presently. It would > be better we use the same way for all misc parameters, I will file a > patch to clear encoder_context->misc_param[]. Ok, sounds good :) Thanks, - Mark ___ Libva mailing list Libva@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libva
[Libva] [PATCH] H.264 encoder: respect min QP setting
Signed-off-by: Mark Thompson
---
On 03/01/17 07:54, Xiang, Haihao wrote:
> On Fri, 2016-12-30 at 16:15 +, Mark Thompson wrote:
>> Signed-off-by: Mark Thompson
>> ---
>> This is helpful to avoid some extremes of the CBR rate control (the QP can
>> easily get down to 1 on a static image, which then
>> consumes the entire HRD buffer encoding the next non-static frame with far
>> more detail that is wanted).
>>
>> The ROI code is not affected by this, it can still go below the configured
>> min QP - I think this behaviour is the right one,
>> but if you don't think so I update that as well.
>
> I agree ROI should also follow the min_qp setting.
Ok, I've added it in a new version of the patch.
>>
>>
>> src/gen6_mfc_common.c | 18 +-
>> src/i965_encoder.c| 4 +++-
>> src/i965_encoder.h| 1 +
>> 3 files changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/gen6_mfc_common.c b/src/gen6_mfc_common.c
>> index 15c0637..e8596ff 100644
>> --- a/src/gen6_mfc_common.c
>> +++ b/src/gen6_mfc_common.c
>> @@ -179,9 +179,9 @@ static void intel_mfc_brc_init(struct encode_state
>> *encode_state,
>> mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I] =
>> mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P];
>> mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B] =
>> mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I];
>>
>> -BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I], 1, 51);
>> -BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P], 1, 51);
>> -BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B], 1, 51);
>> +BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I],
>> (int)encoder_context->brc.min_qp, 51);
>> +BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P],
>> (int)encoder_context->brc.min_qp, 51);
>> +BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B],
>> (int)encoder_context->brc.min_qp, 51);
>
> How about to set the clamp range to (MAX(1, encoder_context->brc.min_qp), 51)
> ? According to the API comment, setting min_qp to
> 0 means the driver can choose a minimal QP. Usually user doesn't set min_qp
> and we can keep the behavior unchanged for min_qp =
> 0.
Yes, sorry, that change was an error on my part - indeed it should continue to
be bounded below at 1. Fixed in the new patch using MAX.
Thanks,
- Mark
src/gen6_mfc_common.c | 28
src/i965_encoder.c| 4 +++-
src/i965_encoder.h| 1 +
3 files changed, 20 insertions(+), 13 deletions(-)
diff --git a/src/gen6_mfc_common.c b/src/gen6_mfc_common.c
index 15c0637..90cee05 100644
--- a/src/gen6_mfc_common.c
+++ b/src/gen6_mfc_common.c
@@ -98,6 +98,7 @@ static void intel_mfc_brc_init(struct encode_state
*encode_state,
double frame_per_bits = 8 * 3 * encoder_context->frame_width_in_pixel *
encoder_context->frame_height_in_pixel / 2;
double qp1_size = 0.1 * frame_per_bits;
double qp51_size = 0.001 * frame_per_bits;
+int min_qp = MAX(1, encoder_context->brc.min_qp);
double bpf, factor, hrd_factor;
int inum = encoder_context->brc.num_iframes_in_gop,
pnum = encoder_context->brc.num_pframes_in_gop,
@@ -179,9 +180,9 @@ static void intel_mfc_brc_init(struct encode_state
*encode_state,
mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I] =
mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P];
mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B] =
mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I];
-BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I], 1, 51);
-BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P], 1, 51);
-BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B], 1, 51);
+BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I], min_qp, 51);
+BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P], min_qp, 51);
+BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B], min_qp, 51);
}
}
@@ -226,6 +227,7 @@ int intel_mfc_brc_postpack(struct encode_state
*encode_state,
int qpn; // predicted quantizer for next frame of current type in integer
format
double qpf; // predicted quantizer for next frame of current type in float
format
double delta_qp; // QP correction
+int min_qp = MAX(1, encoder_context->brc.min_qp);
int target_frame_size, frame_size_next;
/* Notes:
* x - how far we are from HRD buffer borders
@@ -318,7 +320,7 @@ int intel_mfc_brc_postpack(struct encode_state
*encode_state,
qpn = (int)(qpn + delta_qp + 0.5);
/* making sure that with QP predictions we did do not leave QPs range */
-BRC_CLIP(qpn, 1, 51);
+BRC_CLIP(qpn, min_qp, 51);
if (sts == BRC_NO_HRD_VIOLATION) { // no HRD violation
/* correcting QPs of slices of other types */
@@ -338,9 +340,9 @@ int intel_mfc_brc_postpack(struct encode_state
*encode_state,
if (abs(qpn - BRC_I_B_QP_DIFF - qpi) > 4)
mfc_context->brc.qp_prime_y[next_frame_layer_id][SLICE_TYPE_I]
+= (qp
