[Libva] [PATCH 1/2] H.264 encoder: respect min QP setting

[Xiang, Haihao]

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.

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