[Libva] [PATCH] H.264 encoder: respect min QP setting
Xiang, Haihao
haihao.xiang at intel.com
Wed Jan 4 08:50:48 UTC 2017
Could you send your new revision in a separate email with a version number in the future? I thought this email was just a reply
to the previous email.
BTW I failed to apply the 2nd patch in the patch series by git-am. Could you rebase the 2nd patch?
Thanks
Haihao
> Signed-off-by: Mark Thompson <sw at jkqxz.net>
> ---
> On 03/01/17 07:54, Xiang, Haihao wrote:
> > On Fri, 2016-12-30 at 16:15 +0000, Mark Thompson wrote:
> > > Signed-off-by: Mark Thompson <sw at jkqxz.net>
> > > ---
> > > 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] += (qpn - BRC_I_B_QP_DIFF - qpi) >> 2;
> }
> - BRC_CLIP(mfc_context->brc.qp_prime_y[next_frame_layer_id][SLICE_TYPE_I], 1, 51);
> - BRC_CLIP(mfc_context->brc.qp_prime_y[next_frame_layer_id][SLICE_TYPE_P], 1, 51);
> - BRC_CLIP(mfc_context->brc.qp_prime_y[next_frame_layer_id][SLICE_TYPE_B], 1, 51);
> + BRC_CLIP(mfc_context->brc.qp_prime_y[next_frame_layer_id][SLICE_TYPE_I], min_qp, 51);
> + BRC_CLIP(mfc_context->brc.qp_prime_y[next_frame_layer_id][SLICE_TYPE_P], min_qp, 51);
> + BRC_CLIP(mfc_context->brc.qp_prime_y[next_frame_layer_id][SLICE_TYPE_B], min_qp, 51);
> } else if (sts == BRC_UNDERFLOW) { // underflow
> if (qpn <= qp) qpn = qp + 1;
> if (qpn > 51) {
> @@ -349,8 +351,8 @@ int intel_mfc_brc_postpack(struct encode_state *encode_state,
> }
> } else if (sts == BRC_OVERFLOW) {
> if (qpn >= qp) qpn = qp - 1;
> - if (qpn < 1) { // < 0 (?) overflow with minQP
> - qpn = 1;
> + if (qpn < min_qp) { // overflow with minQP
> + qpn = min_qp;
> sts = BRC_OVERFLOW_WITH_MIN_QP; // bit stuffing to be done
> }
> }
> @@ -1815,6 +1817,7 @@ intel_h264_enc_roi_cbr(VADriverContextP ctx,
> struct intel_encoder_context *encoder_context)
> {
> int nonroi_qp;
> + int min_qp = MAX(1, encoder_context->brc.min_qp);
> VAEncROI *region_roi;
> bool quickfill = 0;
>
> @@ -1889,7 +1892,7 @@ intel_h264_enc_roi_cbr(VADriverContextP ctx,
> param_regions[i].height_mbs = roi_height_mbs;
>
> roi_qp = base_qp + region_roi->roi_value;
> - BRC_CLIP(roi_qp, 1, 51);
> + BRC_CLIP(roi_qp, min_qp, 51);
>
> param_regions[i].roi_qp = roi_qp;
> qstep_roi = intel_h264_qp_qstep(roi_qp);
> @@ -1911,7 +1914,7 @@ intel_h264_enc_roi_cbr(VADriverContextP ctx,
> nonroi_qp = intel_h264_qstep_qp(qstep_nonroi);
> }
>
> - BRC_CLIP(nonroi_qp, 1, 51);
> + BRC_CLIP(nonroi_qp, min_qp, 51);
>
> qp_fill:
> memset(vme_context->qp_per_mb, nonroi_qp, mbs_in_picture);
> @@ -1992,6 +1995,7 @@ intel_h264_enc_roi_config(VADriverContextP ctx,
> VAEncPictureParameterBufferH264 *pic_param = (VAEncPictureParameterBufferH264 *)encode_state->pic_param_ext->buffer;
> VAEncSliceParameterBufferH264 *slice_param = (VAEncSliceParameterBufferH264 *)encode_state->slice_params_ext[0]-
> >buffer;
> int qp;
> + int min_qp = MAX(1, encoder_context->brc.min_qp);
>
> qp = pic_param->pic_init_qp + slice_param->slice_qp_delta;
> memset(vme_context->qp_per_mb, qp, width_in_mbs * height_in_mbs);
> @@ -2015,7 +2019,7 @@ intel_h264_enc_roi_config(VADriverContextP ctx,
> qp_delta = region_roi->roi_value;
> qp_clip = qp + qp_delta;
>
> - BRC_CLIP(qp_clip, 1, 51);
> + BRC_CLIP(qp_clip, min_qp, 51);
>
> for (i = row_start; i < row_end; i++) {
> qp_ptr = vme_context->qp_per_mb + (i * width_in_mbs) + col_start;
> diff --git a/src/i965_encoder.c b/src/i965_encoder.c
> index ae31f01..3056900 100644
> --- a/src/i965_encoder.c
> +++ b/src/i965_encoder.c
> @@ -575,8 +575,10 @@ intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
> encoder_context->brc.need_reset = 1;
> }
>
> - if (encoder_context->brc.window_size != misc->window_size) {
> + if (encoder_context->brc.window_size != misc->window_size ||
> + encoder_context->brc.min_qp != misc->min_qp) {
> encoder_context->brc.window_size = misc->window_size;
> + encoder_context->brc.min_qp = misc->min_qp;
> encoder_context->brc.need_reset = 1;
> }
>
> diff --git a/src/i965_encoder.h b/src/i965_encoder.h
> index be19ce6..16a6f3f 100644
> --- a/src/i965_encoder.h
> +++ b/src/i965_encoder.h
> @@ -92,6 +92,7 @@ struct intel_encoder_context
> unsigned int hrd_buffer_size;
> unsigned int hrd_initial_buffer_fullness;
> unsigned int window_size;
> + unsigned int min_qp;
> unsigned int need_reset;
>
> unsigned int num_roi;
More information about the Libva
mailing list