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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/libva _______________________________________________ Libva mailing list Libva@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libva