The patch series looks good to me and I also tested with VDEnc. Thanks Haihao
>-----Original Message----- >From: Libva [mailto:libva-boun...@lists.freedesktop.org] On Behalf Of Mark >Thompson >Sent: Friday, December 23, 2016 9:08 AM >To: libva@lists.freedesktop.org >Subject: [Libva] [PATCH 1/4] i965_encoder: consistently represent framerate >as a fraction > >Update references in both H.264 encoders (gen6_mfc and gen9_vdenc). > >Signed-off-by: Mark Thompson <s...@jkqxz.net> >--- >Changes for the series: >* Derive numerator and denominator from the correct parts of the framerate >parameter (see also earlier patch fixing the comment in va.h). >* Make VP8 HRD buffer size derivation the same as other codecs. >* Make the VP9 rate control setup behave more consistently (use window >size if needed for VBR, always use HRD parameters if present). >* Fix warnings. > >Thanks, > >- Mark > > > src/gen6_mfc_common.c | 11 +++++++---- > src/gen9_vdenc.c | 28 ++++++++++++++++------------ > src/gen9_vdenc.h | 2 +- > src/i965_encoder.c | 49 ++++++++++++++++++++++++++++++++++--------- >------ > src/i965_encoder.h | 8 +++++++- > 5 files changed, 65 insertions(+), 33 deletions(-) > >diff --git a/src/gen6_mfc_common.c b/src/gen6_mfc_common.c index >68d030e..15c0637 100644 >--- a/src/gen6_mfc_common.c >+++ b/src/gen6_mfc_common.c >@@ -119,16 +119,19 @@ static void intel_mfc_brc_init(struct encode_state >*encode_state, > > if (i == 0) { > bitrate = encoder_context->brc.bits_per_second[0]; >- framerate = (double)encoder_context->brc.framerate_per_100s[0] / >100.0; >+ framerate = (double)encoder_context->brc.framerate[0].num / >+ (double)encoder_context->brc.framerate[0].den; > } else { > bitrate = (encoder_context->brc.bits_per_second[i] - >encoder_context->brc.bits_per_second[i - 1]); >- framerate = (double)(encoder_context->brc.framerate_per_100s[i] - >encoder_context->brc.framerate_per_100s[i - 1]) / 100.0; >+ framerate = ((double)encoder_context->brc.framerate[i].num / >(double)encoder_context->brc.framerate[i].den) - >+ ((double)encoder_context->brc.framerate[i - 1].num / >+ (double)encoder_context->brc.framerate[i - 1].den); > } > > if (i == encoder_context->layer.num_layers - 1) > factor = 1.0; >- else >- factor = (double)encoder_context->brc.framerate_per_100s[i] / >encoder_context->brc.framerate_per_100s[encoder_context- >>layer.num_layers - 1]; >+ else { >+ factor = ((double)encoder_context->brc.framerate[i].num / >(double)encoder_context->brc.framerate[i].den) / >+ ((double)encoder_context->brc.framerate[i - 1].num / >(double)encoder_context->brc.framerate[i - 1].den); >+ } > > hrd_factor = (double)bitrate / encoder_context- >>brc.bits_per_second[encoder_context->layer.num_layers - 1]; > >diff --git a/src/gen9_vdenc.c b/src/gen9_vdenc.c index 8009b31..691faec >100644 >--- a/src/gen9_vdenc.c >+++ b/src/gen9_vdenc.c >@@ -851,7 +851,7 @@ >gen9_vdenc_update_misc_parameters(VADriverContextP ctx, > if (vdenc_context->internal_rate_mode != I965_BRC_CQP && > encoder_context->brc.need_reset) { > /* So far, vdenc doesn't support temporal layer */ >- vdenc_context->frames_per_100s = encoder_context- >>brc.framerate_per_100s[0]; >+ vdenc_context->framerate = encoder_context->brc.framerate[0]; > > vdenc_context->vbv_buffer_size_in_bit = encoder_context- >>brc.hrd_buffer_size; > vdenc_context->init_vbv_buffer_fullness_in_bit = encoder_context- >>brc.hrd_initial_buffer_fullness; >@@ -927,7 +927,8 @@ gen9_vdenc_update_parameters(VADriverContextP >ctx, > !vdenc_context->vbv_buffer_size_in_bit || > !vdenc_context->max_bit_rate || > !vdenc_context->target_bit_rate || >- !vdenc_context->frames_per_100s)) >+ !vdenc_context->framerate.num || >+ !vdenc_context->framerate.den)) > vdenc_context->brc_enabled = 0; > > if (!vdenc_context->brc_enabled) { >@@ -1565,7 +1566,8 @@ >gen9_vdenc_get_profile_level_max_frame(VADriverContextP ctx, > tmpf = max_mbps / 172.0; > > max_byte_per_frame0 = (uint64_t)(tmpf * bits_per_mb); >- max_byte_per_frame1 = (uint64_t)(((double)max_mbps * 100) / >vdenc_context->frames_per_100s *bits_per_mb); >+ max_byte_per_frame1 = (uint64_t)(((double)max_mbps * >vdenc_context->framerate.den) / >+ >+ (double)vdenc_context->framerate.num * bits_per_mb); > > /* TODO: check VAEncMiscParameterTypeMaxFrameSize */ > ret = (unsigned int)MIN(max_byte_per_frame0, max_byte_per_frame1); >@@ -1586,12 +1588,12 @@ >gen9_vdenc_calculate_initial_qp(VADriverContextP ctx, > > frame_size = (vdenc_context->frame_width * vdenc_context- >>frame_height * 3 / 2); > qp = (int)(1.0 / 1.2 * pow(10.0, >- (log10(frame_size * 2.0 / 3.0 * >((float)vdenc_context- >>frames_per_100s) / >- ((float)(vdenc_context->target_bit_rate >* 1000) * 100)) - >x0) * >+ (log10(frame_size * 2.0 / 3.0 * vdenc_context- >>framerate.num / >+ >+ ((double)vdenc_context->target_bit_rate * 1000.0 * >+ vdenc_context->framerate.den)) - x0) * > (y1 - y0) / (x1 - x0) + y0) + 0.5); > qp += 2; >- delat_qp = (int)(9 - (vdenc_context->vbv_buffer_size_in_bit * >((float)vdenc_context->frames_per_100s) / >- ((float)(vdenc_context->target_bit_rate * 1000) * >100))); >+ delat_qp = (int)(9 - (vdenc_context->vbv_buffer_size_in_bit * >((double)vdenc_context->framerate.num) / >+ ((double)vdenc_context->target_bit_rate * >+ 1000.0 * vdenc_context->framerate.den))); > if (delat_qp > 0) > qp += delat_qp; > >@@ -1615,7 +1617,8 @@ >gen9_vdenc_update_huc_brc_init_dmem(VADriverContextP ctx, > double input_bits_per_frame, bps_ratio; > int i; > >- vdenc_context->brc_init_reset_input_bits_per_frame = >((double)(vdenc_context->max_bit_rate * 1000) * 100) / vdenc_context- >>frames_per_100s; >+ vdenc_context->brc_init_reset_input_bits_per_frame = >+ ((double)vdenc_context->max_bit_rate * 1000.0 * >+ vdenc_context->framerate.den) / vdenc_context->framerate.num; > vdenc_context->brc_init_current_target_buf_full_in_bits = >vdenc_context->brc_init_reset_input_bits_per_frame; > vdenc_context->brc_target_size = vdenc_context- >>init_vbv_buffer_fullness_in_bit; > >@@ -1645,8 +1648,8 @@ >gen9_vdenc_update_huc_brc_init_dmem(VADriverContextP ctx, > else if (vdenc_context->internal_rate_mode == I965_BRC_VBR) > dmem->brc_flag |= 0x20; > >- dmem->frame_rate_m = vdenc_context->frames_per_100s; >- dmem->frame_rate_d = 100; >+ dmem->frame_rate_m = vdenc_context->framerate.num; >+ dmem->frame_rate_d = vdenc_context->framerate.den; > > dmem->profile_level_max_frame = >gen9_vdenc_get_profile_level_max_frame(ctx, encoder_context, >seq_param->level_idc); > >@@ -1656,8 +1659,9 @@ >gen9_vdenc_update_huc_brc_init_dmem(VADriverContextP ctx, > dmem->min_qp = 10; > dmem->max_qp = 51; > >- input_bits_per_frame = ((double)vdenc_context->max_bit_rate * 1000 * >100) / vdenc_context->frames_per_100s; >- bps_ratio = input_bits_per_frame / ((double)vdenc_context- >>vbv_buffer_size_in_bit * 100 / vdenc_context->frames_per_100s); >+ input_bits_per_frame = ((double)vdenc_context->max_bit_rate * 1000.0 >* vdenc_context->framerate.den) / vdenc_context->framerate.num; >+ bps_ratio = input_bits_per_frame / >+ ((double)vdenc_context->vbv_buffer_size_in_bit * >+ vdenc_context->framerate.den / vdenc_context->framerate.num); > > if (bps_ratio < 0.1) > bps_ratio = 0.1; >diff --git a/src/gen9_vdenc.h b/src/gen9_vdenc.h index e790497..41e4362 >100644 >--- a/src/gen9_vdenc.h >+++ b/src/gen9_vdenc.h >@@ -741,7 +741,7 @@ struct gen9_vdenc_context > uint32_t min_bit_rate; /* in kbps */ > uint64_t init_vbv_buffer_fullness_in_bit; > uint64_t vbv_buffer_size_in_bit; >- uint32_t frames_per_100s; >+ struct intel_fraction framerate; > uint32_t gop_size; > uint32_t ref_dist; > uint32_t brc_target_size; >diff --git a/src/i965_encoder.c b/src/i965_encoder.c index d874322..3aaac34 >100644 >--- a/src/i965_encoder.c >+++ b/src/i965_encoder.c >@@ -42,6 +42,17 @@ > > #include "i965_post_processing.h" > >+static struct intel_fraction >+reduce_fraction(struct intel_fraction f) { >+ unsigned int a = f.num, b = f.den, c; >+ while ((c = a % b)) { >+ a = b; >+ b = c; >+ } >+ return (struct intel_fraction) { f.num / b, f.den / b }; } >+ > static VAStatus > clear_border(struct object_surface *obj_surface) { @@ -306,8 +317,9 @@ >intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx, > struct intel_encoder_context > *encoder_context) { > VAEncSequenceParameterBufferH264 *seq_param = >(VAEncSequenceParameterBufferH264 *)encode_state->seq_param_ext- >>buffer; >+ struct intel_fraction framerate; >+ unsigned int bits_per_second; > unsigned short num_pframes_in_gop, num_bframes_in_gop; >- unsigned int bits_per_second, framerate_per_100s; > > if (!encoder_context->is_new_sequence) > return VA_STATUS_SUCCESS; >@@ -315,10 +327,13 @@ >intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx, > assert(seq_param); > bits_per_second = seq_param->bits_per_second; // for the highest layer > >- if (!seq_param->num_units_in_tick || !seq_param->time_scale) >- framerate_per_100s = 3000; >- else >- framerate_per_100s = seq_param->time_scale * 100 / (2 * seq_param- >>num_units_in_tick); // for the highest layer >+ if (!seq_param->num_units_in_tick || !seq_param->time_scale) { >+ framerate = (struct intel_fraction) { 30, 1 }; >+ } else { >+ // for the highest layer >+ framerate = (struct intel_fraction) { seq_param->time_scale, 2 * >seq_param->num_units_in_tick }; >+ } >+ framerate = reduce_fraction(framerate); > > encoder_context->brc.num_iframes_in_gop = 1; // Always 1 > >@@ -326,7 +341,7 @@ >intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx, > if (seq_param->ip_period == 0) > goto error; > >- encoder_context->brc.gop_size = (unsigned int)(framerate_per_100s / >100.0 + 0.5); // fake >+ encoder_context->brc.gop_size = (framerate.num + framerate.den >+ - 1) / framerate.den; // fake > num_pframes_in_gop = (encoder_context->brc.gop_size + > seq_param->ip_period - 1) / > seq_param->ip_period - 1; > } else if (seq_param->intra_period == 1) { // E.g. IDRIII... >@@ -347,11 +362,12 @@ >intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx, > if (num_pframes_in_gop != encoder_context->brc.num_pframes_in_gop >|| > num_bframes_in_gop != encoder_context->brc.num_bframes_in_gop >|| > bits_per_second != encoder_context- >>brc.bits_per_second[encoder_context->layer.num_layers - 1] || >- framerate_per_100s != encoder_context- >>brc.framerate_per_100s[encoder_context->layer.num_layers - 1]) { >+ framerate.num != encoder_context->brc.framerate[encoder_context- >>layer.num_layers - 1].num || >+ framerate.den != >+ encoder_context->brc.framerate[encoder_context->layer.num_layers - >+ 1].den) { > encoder_context->brc.num_pframes_in_gop = num_pframes_in_gop; > encoder_context->brc.num_bframes_in_gop = num_bframes_in_gop; > encoder_context->brc.bits_per_second[encoder_context- >>layer.num_layers - 1] = bits_per_second; >- encoder_context->brc.framerate_per_100s[encoder_context- >>layer.num_layers - 1] = framerate_per_100s; >+ >+ encoder_context->brc.framerate[encoder_context->layer.num_layers - 1] >+ = framerate; > encoder_context->brc.need_reset = 1; > } > >@@ -392,8 +408,9 @@ >intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx, > num_pframes_in_gop = encoder_context->brc.gop_size - 1; > bits_per_second = seq_param->bits_per_second; // for the highest >layer > >- if (!encoder_context->brc.framerate_per_100s[encoder_context- >>layer.num_layers - 1]) { >- encoder_context->brc.framerate_per_100s[encoder_context- >>layer.num_layers - 1] = 3000; // for the highest layer >+ if (!encoder_context->brc.framerate[encoder_context->layer.num_layers >- 1].num) { >+ // for the highest layer >+ >+ encoder_context->brc.framerate[encoder_context->layer.num_layers - 1] >+ = (struct intel_fraction) { 30, 1 }; > encoder_context->brc.need_reset = 1; > } > >@@ -480,7 +497,7 @@ >intel_encoder_check_framerate_parameter(VADriverContextP ctx, > struct intel_encoder_context > *encoder_context, > VAEncMiscParameterFrameRate *misc) { >- int framerate_per_100s; >+ struct intel_fraction framerate; > int temporal_id = 0; > > if (encoder_context->layer.num_layers >= 2) @@ -490,12 +507,14 @@ >intel_encoder_check_framerate_parameter(VADriverContextP ctx, > return; > > if (misc->framerate & 0xffff0000) >- framerate_per_100s = (misc->framerate & 0xffff) * 100 / ((misc- >>framerate >> 16) & 0xffff); >+ framerate = (struct intel_fraction) { misc->framerate & 0xffff, >+ misc->framerate >> 16 & 0xffff }; > else >- framerate_per_100s = misc->framerate * 100; >+ framerate = (struct intel_fraction) { misc->framerate, 1 }; >+ framerate = reduce_fraction(framerate); > >- if (encoder_context->brc.framerate_per_100s[temporal_id] != >framerate_per_100s) { >- encoder_context->brc.framerate_per_100s[temporal_id] = >framerate_per_100s; >+ if (encoder_context->brc.framerate[temporal_id].num != framerate.num >|| >+ encoder_context->brc.framerate[temporal_id].den != framerate.den) { >+ encoder_context->brc.framerate[temporal_id] = framerate; > encoder_context->brc.need_reset = 1; > } > } >diff --git a/src/i965_encoder.h b/src/i965_encoder.h index 0b636d6..7016975 >100644 >--- a/src/i965_encoder.h >+++ b/src/i965_encoder.h >@@ -55,6 +55,12 @@ struct intel_roi > char value; > }; > >+struct intel_fraction >+{ >+ unsigned int num; >+ unsigned int den; >+}; >+ > struct intel_encoder_context > { > struct hw_context base; >@@ -80,7 +86,7 @@ struct intel_encoder_context > unsigned short num_pframes_in_gop; > unsigned short num_bframes_in_gop; > unsigned int bits_per_second[MAX_TEMPORAL_LAYERS]; >- unsigned int framerate_per_100s[MAX_TEMPORAL_LAYERS]; >+ struct intel_fraction framerate[MAX_TEMPORAL_LAYERS]; > unsigned int mb_rate_control[MAX_TEMPORAL_LAYERS]; > unsigned int target_percentage[MAX_TEMPORAL_LAYERS]; > unsigned int hrd_buffer_size; >-- >2.11.0 > >_______________________________________________ >Libva mailing list >Libva@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/libva _______________________________________________ Libva mailing list Libva@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libva