[Libva] [Libva-intel-driver][PATCH] Needn't reset brc if the bitrate setting isn't changed in the Begin/Render/End sequence
Xiang, Haihao
haihao.xiang at intel.com
Fri Dec 30 00:51:48 UTC 2016
On Thu, 2016-12-29 at 18:52 +0000, Mark Thompson wrote:
> On 29/12/16 07:09, Zhao Yakui wrote:
> > On 12/29/2016 03:08 PM, Xiang, Haihao wrote:
> > >
> > > > On 12/29/2016 01:22 PM, Xiang, Haihao wrote:
> > > > > User can use VAEncMiscParameterRateControl to update bitrate,
> > > > > so we
> > > > > should ignore
> > > > > the bitrate in the sequence parameter if
> > > > > VAEncMiscParameterRateControl is present.
> > > >
> > > > This makes sense. The MiscRateControl mentions that it can
> > > > override
> > > > the
> > > > bps setting in sequence parameter.
> > > >
> > > > > Hence it is not needed to reset brc if
> > > > > VAEncMiscParameterRateControl doesn't change
> > > > > the used bitrate.
> > > >
> > > > Does it still need to send the VAEncMiscParameterRateControl
> > > > parameter
> > > > if the bps is not changed for the new sequence?
> > >
> > > Yes if bits_per_second in the sequence parameter is not equal to
> > > the
> > > value for the previous Begin/Render/End sequence except
> > > bits_per_second
> > > in the sequence parameter is 0.
> > >
> >
> > OK.
> > It makes sense.
> >
> > This looks good to me.
>
> I'm not sure this behaviour is correct. Should not the most recently
> given bitrate value from the user, either from sequence parameters or
> RC parameters, be used? When is_new_sequence is set, the user will
> have provided a set of sequence parameters (because we are making an
> IDR frame), so the bitrate there is provided by the user and should
> override any previous RC parameters given before that frame (but not
> any on this one).
>
> That is, I think it should work like:
>
> Sequence parameters with bps = 1M
> -> IDR frame, at 1Mbps
> -> some P frames, at 1Mbps
> RC parameters with bps = 2M
> -> some P frames, at 2Mbps
> Sequence parameters with bps = 3M
> -> IDR frame, at 3Mbps
> -> some P frames, at 3Mbps
The 2nd sequence is generated at 3Mbps with the current
logic. hl_bitrate_updated is 0 for your case and we use the sequence
parameters.
>
> But with the current code the second sequence is generated at 2Mbps
> because the RC parameters stay forever and "win" in some sense when
> they disagree.
>
> So, I think a nicer solution would be to add some way to determine
> which parameter set was provided most recently, so that we can always
> use the right one (with RC parameters overriding sequence parameters
> when both are provided). See patch below for a possible approach
> (rather ugly and not very well tested).
>
> Thanks,
>
> - Mark
>
>
> diff --git a/src/i965_drv_video.c b/src/i965_drv_video.c
> index 15920f5..5d218d7 100644
> --- a/src/i965_drv_video.c
> +++ b/src/i965_drv_video.c
> @@ -3197,6 +3197,9 @@
> i965_encoder_render_misc_parameter_buffer(VADriverContextP ctx,
> if (param->type == VAEncMiscParameterTypeTemporalLayerStructure)
> encode->has_layers = 1;
>
> + if (param->type == VAEncMiscParameterTypeRateControl)
> + encode->frame_has_rc_parameters = 1;
> +
> index = i965_encoder_get_misc_paramerter_buffer_index(ctx,
> encode, param);
>
> if (index >= ARRAY_ELEMS(encode->misc_param[0]))
> @@ -3243,6 +3246,7 @@ i965_encoder_render_picture(VADriverContextP
> ctx,
>
> case VAEncSequenceParameterBufferType:
> vaStatus =
> I965_RENDER_ENCODE_BUFFER(sequence_parameter_ext);
> + encode->frame_has_sequence_parameters = 1;
> break;
>
> case VAEncPictureParameterBufferType:
> diff --git a/src/i965_drv_video.h b/src/i965_drv_video.h
> index 7cba3a3..b326e50 100644
> --- a/src/i965_drv_video.h
> +++ b/src/i965_drv_video.h
> @@ -272,6 +272,10 @@ struct encode_state
>
> int has_layers;
>
> + /* Whether these parameter sets were supplied with the current
> frame. */
> + int frame_has_rc_parameters;
> + int frame_has_sequence_parameters;
> +
> struct buffer_store *misc_param[16][8];
>
> VASurfaceID current_render_target;
> diff --git a/src/i965_encoder.c b/src/i965_encoder.c
> index d4d7445..0ae8965 100644
> --- a/src/i965_encoder.c
> +++ b/src/i965_encoder.c
> @@ -361,16 +361,20 @@
> 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.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[encoder_context-
> >layer.num_layers - 1] = framerate;
> encoder_context->brc.need_reset = 1;
> }
>
> + if (encode_state->frame_has_sequence_parameters &&
> !encode_state->frame_has_rc_parameters &&
> + bits_per_second != encoder_context-
> >brc.bits_per_second[encoder_context->layer.num_layers - 1]) {
> + encoder_context->brc.bits_per_second[encoder_context-
> >layer.num_layers - 1] = bits_per_second;
> + encoder_context->brc.need_reset = 1;
> + }
> +
> if (!encoder_context->brc.hrd_buffer_size ||
> !encoder_context->brc.hrd_initial_buffer_fullness) {
> encoder_context->brc.hrd_buffer_size = seq_param-
> >bits_per_second << 1;
> @@ -414,9 +418,13 @@
> intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
> encoder_context->brc.need_reset = 1;
> }
>
> - if (num_pframes_in_gop != encoder_context-
> >brc.num_pframes_in_gop ||
> - bits_per_second != encoder_context-
> >brc.bits_per_second[encoder_context->layer.num_layers - 1]) {
> + if (num_pframes_in_gop != encoder_context-
> >brc.num_pframes_in_gop) {
> encoder_context->brc.num_pframes_in_gop =
> num_pframes_in_gop;
> + encoder_context->brc.need_reset = 1;
> + }
> +
> + if (encode_state->frame_has_sequence_parameters &&
> !encode_state->frame_has_rc_parameters &&
> + bits_per_second != encoder_context-
> >brc.bits_per_second[encoder_context->layer.num_layers - 1]) {
> encoder_context->brc.bits_per_second[encoder_context-
> >layer.num_layers - 1] = bits_per_second;
> encoder_context->brc.need_reset = 1;
> }
> @@ -481,7 +489,8 @@
> intel_encoder_check_brc_hevc_sequence_parameter(VADriverContextP ctx,
> encoder_context->brc.need_reset = 1;
> }
>
> - if (encoder_context->brc.bits_per_second[0] != seq_param-
> >bits_per_second) {
> + if (encode_state->frame_has_sequence_parameters &&
> !encode_state->frame_has_rc_parameters &&
> + encoder_context->brc.bits_per_second[0] != seq_param-
> >bits_per_second) {
> encoder_context->brc.bits_per_second[0] = seq_param-
> >bits_per_second;
> encoder_context->brc.need_reset = 1;
> }
> @@ -507,13 +516,17 @@
> intel_encoder_check_brc_vp9_sequence_parameter(VADriverContextP ctx,
> else
> gop_size = seq_param->intra_period;
>
> - if (encoder_context->brc.bits_per_second[0] != seq_param-
> >bits_per_second ||
> - encoder_context->brc.gop_size != gop_size) {
> - encoder_context->brc.bits_per_second[0] = seq_param-
> >bits_per_second;
> + if (encoder_context->brc.gop_size != gop_size) {
> encoder_context->brc.gop_size = gop_size;
> encoder_context->brc.need_reset = 1;
> }
>
> + if (encode_state->frame_has_sequence_parameters &&
> !encode_state->frame_has_rc_parameters &&
> + encoder_context->brc.bits_per_second[0] != seq_param-
> >bits_per_second) {
> + encoder_context->brc.bits_per_second[0] = seq_param-
> >bits_per_second;
> + encoder_context->brc.need_reset = 1;
> + }
> +
> return VA_STATUS_SUCCESS;
> }
>
> @@ -544,6 +557,7 @@
> intel_encoder_check_brc_sequence_parameter(VADriverContextP ctx,
>
> static void
> intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
> + struct encode_state
> *encode_state,
> struct
> intel_encoder_context *encoder_context,
> VAEncMiscParameterRateCon
> trol *misc)
> {
> @@ -558,7 +572,8 @@
> intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
> if (misc->rc_flags.bits.reset)
> encoder_context->brc.need_reset = 1;
>
> - if (encoder_context->brc.bits_per_second[temporal_id] != misc-
> >bits_per_second) {
> + if (encode_state->frame_has_rc_parameters &&
> + encoder_context->brc.bits_per_second[temporal_id] != misc-
> >bits_per_second) {
> encoder_context->brc.bits_per_second[temporal_id] = misc-
> >bits_per_second;
> encoder_context->brc.need_reset = 1;
> }
> @@ -676,7 +691,7 @@
> intel_encoder_check_brc_parameter(VADriverContextP ctx,
>
> case VAEncMiscParameterTypeRateControl:
> intel_encoder_check_rate_control_parameter(ctx,
> - encoder_c
> ontext,
> + encode_st
> ate, encoder_context,
> (VAEncMis
> cParameterRateControl *)misc_param->data);
> break;
>
> @@ -1299,6 +1314,9 @@ intel_encoder_end_picture(VADriverContextP ctx,
> */
> encoder_context->brc.num_roi = 0;
>
> + encode_state->frame_has_sequence_parameters = 0;
> + encode_state->frame_has_rc_parameters = 0;
> +
> return VA_STATUS_SUCCESS;
> }
>
> _______________________________________________
> Libva mailing list
> Libva at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/libva
More information about the Libva
mailing list