Discussion:
[PATCH 1/2] H.264 encoder: respect min QP setting
(too old to reply)
Mark Thompson
2016-12-30 16:15:19 UTC
Permalink
Signed-off-by: Mark Thompson <***@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.


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

@@ -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;
--
2.11.0
Mark Thompson
2016-12-30 16:22:46 UTC
Permalink
Signed-off-by: Mark Thompson <***@jkqxz.net>
---
This can be set by the user to ensure that the first frame of a stream is encoded at with a good QP (often it starts too high and takes a few frames to get down to the right range). The current code continues to be used if the value is not set (is zero).


src/gen6_mfc_common.c | 24 +++++++++++++++---------
src/i965_encoder.c | 2 ++
src/i965_encoder.h | 1 +
3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/src/gen6_mfc_common.c b/src/gen6_mfc_common.c
index e8596ff..8907751 100644
--- a/src/gen6_mfc_common.c
+++ b/src/gen6_mfc_common.c
@@ -168,16 +168,22 @@ static void intel_mfc_brc_init(struct encode_state *encode_state,

bpf = mfc_context->brc.bits_per_frame[i] = bitrate/framerate;

- if ((bpf > qp51_size) && (bpf < qp1_size)) {
- mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51 - 50*(bpf - qp51_size)/(qp1_size - qp51_size);
- }
- else if (bpf >= qp1_size)
- mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 1;
- else if (bpf <= qp51_size)
- mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51;
+ if (encoder_context->brc.initial_qp) {
+ mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I] = encoder_context->brc.initial_qp;
+ mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = encoder_context->brc.initial_qp;
+ mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B] = encoder_context->brc.initial_qp;
+ } else {
+ if ((bpf > qp51_size) && (bpf < qp1_size)) {
+ mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51 - 50*(bpf - qp51_size)/(qp1_size - qp51_size);
+ }
+ else if (bpf >= qp1_size)
+ mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 1;
+ else if (bpf <= qp51_size)
+ mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51;

- 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];
+ 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], (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);
diff --git a/src/i965_encoder.c b/src/i965_encoder.c
index 3056900..0a648d4 100644
--- a/src/i965_encoder.c
+++ b/src/i965_encoder.c
@@ -576,8 +576,10 @@ intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
}

if (encoder_context->brc.window_size != misc->window_size ||
+ encoder_context->brc.initial_qp != misc->initial_qp ||
encoder_context->brc.min_qp != misc->min_qp) {
encoder_context->brc.window_size = misc->window_size;
+ encoder_context->brc.initial_qp = misc->initial_qp;
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 16a6f3f..829df9d 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 initial_qp;
unsigned int min_qp;
unsigned int need_reset;
--
2.11.0
Xiang, Haihao
2017-01-03 07:54:56 UTC
Permalink
Post 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.
Post by Mark Thompson
 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.
Post by Mark Thompson
     }
 }
 
@@ -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;
Mark Thompson
2017-01-03 23:43:38 UTC
Permalink
Signed-off-by: Mark Thompson <***@jkqxz.net>
---
Post by Xiang, Haihao
Post 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.
Post by Xiang, Haihao
Post by Mark Thompson
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;
--
2.11.0
Xiang, Haihao
2017-01-04 08:50:48 UTC
Permalink
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
Post by Mark Thompson
---
Post by Xiang, Haihao
Post 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.
Post by Xiang, Haihao
Post by Mark Thompson
 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;
      *  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);
 
     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]-
Post by Xiang, Haihao
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;
Mark Thompson
2017-01-04 09:15:06 UTC
Permalink
Signed-off-by: Mark Thompson <***@jkqxz.net>
---
Post by Xiang, Haihao
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.
Ok, will do.
Post by Xiang, Haihao
BTW I failed to apply the 2nd patch in the patch series by git-am. Could you rebase the 2nd patch?
Sure. Here it is rebased against v2 of the min-qp patch (and otherwise unchanged).

Thanks,

- Mark


src/gen6_mfc_common.c | 24 +++++++++++++++---------
src/i965_encoder.c | 2 ++
src/i965_encoder.h | 1 +
3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/src/gen6_mfc_common.c b/src/gen6_mfc_common.c
index 90cee05..1643efc 100644
--- a/src/gen6_mfc_common.c
+++ b/src/gen6_mfc_common.c
@@ -169,16 +169,22 @@ static void intel_mfc_brc_init(struct encode_state *encode_state,

bpf = mfc_context->brc.bits_per_frame[i] = bitrate/framerate;

- if ((bpf > qp51_size) && (bpf < qp1_size)) {
- mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51 - 50*(bpf - qp51_size)/(qp1_size - qp51_size);
- }
- else if (bpf >= qp1_size)
- mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 1;
- else if (bpf <= qp51_size)
- mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51;
+ if (encoder_context->brc.initial_qp) {
+ mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I] = encoder_context->brc.initial_qp;
+ mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = encoder_context->brc.initial_qp;
+ mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B] = encoder_context->brc.initial_qp;
+ } else {
+ if ((bpf > qp51_size) && (bpf < qp1_size)) {
+ mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51 - 50*(bpf - qp51_size)/(qp1_size - qp51_size);
+ }
+ else if (bpf >= qp1_size)
+ mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 1;
+ else if (bpf <= qp51_size)
+ mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51;

- 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];
+ 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], min_qp, 51);
BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P], min_qp, 51);
diff --git a/src/i965_encoder.c b/src/i965_encoder.c
index 3056900..0a648d4 100644
--- a/src/i965_encoder.c
+++ b/src/i965_encoder.c
@@ -576,8 +576,10 @@ intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
}

if (encoder_context->brc.window_size != misc->window_size ||
+ encoder_context->brc.initial_qp != misc->initial_qp ||
encoder_context->brc.min_qp != misc->min_qp) {
encoder_context->brc.window_size = misc->window_size;
+ encoder_context->brc.initial_qp = misc->initial_qp;
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 16a6f3f..829df9d 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 initial_qp;
unsigned int min_qp;
unsigned int need_reset;
--
2.11.0
Xiang, Haihao
2017-01-05 04:00:56 UTC
Permalink
Thanks for the patch, applied.
Post by Mark Thompson
---
Post by Xiang, Haihao
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.
Ok, will do.
Post by Xiang, Haihao
BTW I failed to apply the 2nd patch in the patch series by git-am. Could you rebase the 2nd patch?
Sure.  Here it is rebased against v2 of the min-qp patch (and otherwise unchanged).
Thanks,
- Mark
 src/gen6_mfc_common.c | 24 +++++++++++++++---------
 src/i965_encoder.c    |  2 ++
 src/i965_encoder.h    |  1 +
 3 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/src/gen6_mfc_common.c b/src/gen6_mfc_common.c
index 90cee05..1643efc 100644
--- a/src/gen6_mfc_common.c
+++ b/src/gen6_mfc_common.c
@@ -169,16 +169,22 @@ static void intel_mfc_brc_init(struct encode_state *encode_state,
 
         bpf = mfc_context->brc.bits_per_frame[i] = bitrate/framerate;
 
-        if ((bpf > qp51_size) && (bpf < qp1_size)) {
-            mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51 - 50*(bpf - qp51_size)/(qp1_size - qp51_size);
-        }
-        else if (bpf >= qp1_size)
-            mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 1;
-        else if (bpf <= qp51_size)
-            mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51;
+        if (encoder_context->brc.initial_qp) {
+            mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I] = encoder_context->brc.initial_qp;
+            mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = encoder_context->brc.initial_qp;
+            mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B] = encoder_context->brc.initial_qp;
+        } else {
+            if ((bpf > qp51_size) && (bpf < qp1_size)) {
+                mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51 - 50*(bpf - qp51_size)/(qp1_size - qp51_size);
+            }
+            else if (bpf >= qp1_size)
+                mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 1;
+            else if (bpf <= qp51_size)
+                mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51;
 
-        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];
+            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], min_qp, 51);
         BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P], min_qp, 51);
diff --git a/src/i965_encoder.c b/src/i965_encoder.c
index 3056900..0a648d4 100644
--- a/src/i965_encoder.c
+++ b/src/i965_encoder.c
@@ -576,8 +576,10 @@ intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
     }
 
     if (encoder_context->brc.window_size != misc->window_size ||
+        encoder_context->brc.initial_qp  != misc->initial_qp ||
         encoder_context->brc.min_qp      != misc->min_qp) {
         encoder_context->brc.window_size = misc->window_size;
+        encoder_context->brc.initial_qp  = misc->initial_qp;
         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 16a6f3f..829df9d 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 initial_qp;
         unsigned int min_qp;
         unsigned int need_reset;
 
Loading...