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