[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 Tue, 2017-01-03 at 09:50 +0800, Zhao Yakui wrote:
> On 12/31/2016 01:54 AM, Mark Thompson wrote:
> > On 30/12/16 00:14, Zhao Yakui wrote:
> > > On 12/30/2016 02:52 AM, 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:
> > > 
> > > Hi, Mark
> > > 
> > >      Thanks for patch review and nice consideration.
> > >      As we know, IDR frame can indicate that a new video sequence
> > > is started. During the decoding, the previous frames before IDR
> > > are discarded and not used. Maybe it will be better that the
> > > encoding parameter related with RC is resent after a new
> > > sequence.  That is to say: The bps in SPS is used for the
> > > initialization for a new sequence. If the MiscRC is sent, it will
> > > override the bps in SPS. If there is no RC, the bps in SPS will
> > > be used.
> > > 
> > >      In such case the user-space middleware only needs to follow
> > > the same logic for every video sequence. Otherwise the different
> > > logic is considered for each video sequence.
> > 
> > Hmm, yes.  That would entail discarding the contents of
> > encoder_context->misc_param[VAEncMiscParameterTypeRateControl] when
> > new sequence parameters are provided?  (In the same way that ROI
> > parameters are discarded after every frame.)  I think that's a much
> > nicer answer than what I had, and is more consistent as well.
> 
> Yes. It need to discard the content of 
> encoder_context->misc_param[VAEncMiscParameterTypeRateControl] when
> the 
> new sequence is provided.
> Unfortunately the current code doesn't discard it.

That is because we didn't save the misc parameters in the common code
 before and we use some misc parameters for next frames,
hence intel_encoder_context[...]

> Will you please send out the patch that discards the 
> encoder_context->misc_param[VAEncMiscParameterTypeRateControl]?

Currently only gen6_mfc_common.c doesn't use generic ROI parameters
in intel_encoder_context. I am cleaning up gen6_mfc_common.c so that we
can clear all misc parameters in the same way.

Note VAEncMiscParameterTypeVP9PerSegmantParam is not a misc parameter
indeed :(


> 
> Thanks
>     Yakui
> 
> 
> > 
> > Thanks,
> > 
> > - Mark
> 
> _______________________________________________
> Libva mailing list
> Libva at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/libva