[Libva] [Libva-intel-driver][PATCH] Needn't reset brc if the bitrate setting isn't changed in the Begin/Render/End sequence

[Xiang, Haihao]

On Fri, 2016-12-30 at 17:44 +0000, Mark Thompson wrote:
> On 30/12/16 00:51, Xiang, Haihao wrote:
> > 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.
> 
> This is not the case - in the absence of a new RC parameter
> structure, the one that was set in the middle of the first sequence
> continues to apply forever (encoder_context->misc_param[] is never
> cleared), so hl_bitrate_updated is always 1 after that point.
> 

You are right, I forgot encoder_context->misc_param[] 
(except VAEncMiscParameterTypeROI) is not cleared presently. It would
be better we use the same way for all misc parameters, I will file a
patch to clear encoder_context->misc_param[]. 

> To show that more concretely, with this patch applied to print the
> internal state:
> 
> diff --git a/src/i965_encoder.c b/src/i965_encoder.c
> index 0a648d4..a7c92ce 100644
> --- a/src/i965_encoder.c
> +++ b/src/i965_encoder.c
> @@ -718,6 +718,12 @@
> intel_encoder_check_brc_parameter(VADriverContextP ctx,
>  
>      }
>  
> +    printf("hl_bitrate_updated = %d, is_new_sequence = %d,
> need_reset = %d, "
> +           "seq_bits_per_second = %d, brc bits_per_second = %d\n",
> +           hl_bitrate_updated, encoder_context->is_new_sequence,
> +           encoder_context->brc.need_reset, seq_bits_per_second,
> +           encoder_context->brc.bits_per_second[0]);
> +
>      return VA_STATUS_SUCCESS;
>  }

Thanks for your patch to demonstrate that problem. I should check the
code more carefully.

> 
> I then encode with a GOP size of 2 and and bitrate increasing by 1M
> on each IDR frame, providing RC parameters only with the first:
> 
> hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 1,
> seq_bits_per_second = 1000000, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
> seq_bits_per_second = 0, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
> seq_bits_per_second = 2000000, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
> seq_bits_per_second = 0, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
> seq_bits_per_second = 3000000, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
> seq_bits_per_second = 0, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
> seq_bits_per_second = 4000000, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
> seq_bits_per_second = 0, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
> seq_bits_per_second = 5000000, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
> seq_bits_per_second = 0, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
> seq_bits_per_second = 6000000, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
> seq_bits_per_second = 0, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
> seq_bits_per_second = 7000000, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
> seq_bits_per_second = 0, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
> seq_bits_per_second = 8000000, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
> seq_bits_per_second = 0, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
> seq_bits_per_second = 9000000, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
> seq_bits_per_second = 0, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
> seq_bits_per_second = 10000000, brc bits_per_second = 1000000
> hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
> seq_bits_per_second = 0, brc bits_per_second = 1000000
> 
> 
> Thanks,
> 
> - Mark