On Fri, 2016-12-30 at 16:15 +0000, Mark Thompson wrote: > Signed-off-by: Mark Thompson <s...@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. > > > 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. > } > } > > @@ -318,7 +318,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, (int)encoder_context->brc.min_qp, 51); > > if (sts == BRC_NO_HRD_VIOLATION) { // no HRD violation > /* correcting QPs of slices of other types */ > @@ -338,9 +338,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], > (int)encoder_context->brc.min_qp, 51); > + > BRC_CLIP(mfc_context->brc.qp_prime_y[next_frame_layer_id][SLICE_TYPE_P], > (int)encoder_context->brc.min_qp, 51); > + > BRC_CLIP(mfc_context->brc.qp_prime_y[next_frame_layer_id][SLICE_TYPE_B], > (int)encoder_context->brc.min_qp, 51); > } else if (sts == BRC_UNDERFLOW) { // underflow > if (qpn <= qp) qpn = qp + 1; > if (qpn > 51) { > @@ -349,8 +349,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 < encoder_context->brc.min_qp) { // overflow with minQP > + qpn = encoder_context->brc.min_qp; > sts = BRC_OVERFLOW_WITH_MIN_QP; // bit stuffing to be done > } > } > 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; _______________________________________________ Libva mailing list Libva@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libva