Re: [Libva] [PATCH 4/4] VP9 encoder: use generic rate control parameters

2016-12-21 Thread Xiang, Haihao 

> On 21/12/16 04:46, Xiang, Haihao wrote:
> > On Mon, 2016-12-19 at 23:01 +, Mark Thompson wrote:
> > > Also adds support for fractional framerate.
> > > 
> > > Signed-off-by: Mark Thompson 
> > > ---
> > >  src/gen9_vp9_encoder.c | 240 ---
> > > --
> > >  src/gen9_vp9_encoder.h |  10 +--
> > >  src/i965_drv_video.c   |  10 +--
> > >  src/i965_encoder.c |  36 
> > >  src/i965_encoder.h |   1 +
> > >  5 files changed, 77 insertions(+), 220 deletions(-)
> > > ...
> > >  /* check the corresponding BRC parameter for CBR and
> > > VBR */
> > >  if (encoder_context->rate_control_mode == VA_RC_CBR)
> > > {
> > > -vp9_state->target_bit_rate = seq_param-
> > > >bits_per_second;
> > > -vp9_state->gop_size = seq_param->intra_period;
> > > -
> > > -if (vp9_state->brc_flag_check & VP9_BRC_HRD) {
> > > -VAEncMiscParameterHRD *misc_param_hrd;
> > > -
> > > -misc_param = (VAEncMiscParameterBuffer *)
> > > -encode_state-
> > > >misc_param[VAEncMiscParameterTypeHRD][0]->buffer;
> > > -misc_param_hrd = (VAEncMiscParameterHRD
> > > *)misc_param->data;
> > > -
> > > -vp9_state->init_vbv_buffer_fullness_in_bit =
> > > misc_param_hrd->initial_buffer_fullness;
> > > -vp9_state->vbv_buffer_size_in_bit =
> > > misc_param_hrd->buffer_size;
> > > -}
> > > -
> > > -if (vp9_state->brc_flag_check & VP9_BRC_FR) {
> > > -VAEncMiscParameterFrameRate *misc_param_fr;
> > > -
> > > -misc_param = (VAEncMiscParameterBuffer *)
> > > -encode_state-
> > > >misc_param[VAEncMiscParameterTypeFrameRate][0]->buffer;
> > > -misc_param_fr = (VAEncMiscParameterFrameRate
> > > *)misc_param->data;
> > > -
> > > -vp9_state->frame_rate = misc_param_fr-
> > > >framerate;
> > > -} else {
> > > -/* Assign the default frame rate */
> > > -vp9_state->frame_rate = 30;
> > > -}
> > > -
> > > -/* RC misc will override HRD parameter */
> > > -if (vp9_state->brc_flag_check & VP9_BRC_RC) {
> > > -VAEncMiscParameterRateControl
> > > *misc_param_rc;
> > > -
> > > -misc_param = (VAEncMiscParameterBuffer *)
> > > -encode_state-
> > > >misc_param[VAEncMiscParameterTypeRateControl][0]->buffer;
> > > -misc_param_rc =
> > > (VAEncMiscParameterRateControl *)misc_param->data;
> > > -
> > > -vp9_state->target_bit_rate = misc_param_rc-
> > > >bits_per_second;
> > > -vp9_state->vbv_buffer_size_in_bit =
> > > (misc_param_rc->bits_per_second / 1000) *
> > > - misc_param_rc-
> > > >window_size;
> > > -vp9_state->init_vbv_buffer_fullness_in_bit =
> > > vp9_state->vbv_buffer_size_in_bit / 2;
> > > -vp9_state->window_size = misc_param_rc-
> > > >window_size;
> > > -}
> > > +if (!encoder_context->brc.framerate[0].num ||
> > > !encoder_context->brc.framerate[0].den ||
> > > +!encoder_context->brc.bits_per_second[0])
> > > +return VA_STATUS_ERROR_INVALID_PARAMETER;
> > > +
> > > +vp9_state->gop_size = encoder_context-
> > > >brc.gop_size;
> > > +
> > > +vp9_state->framerate = encoder_context-
> > > >brc.framerate[0];
> > > +vp9_state->target_bit_rate = encoder_context-
> > > >brc.bits_per_second[0];
> > >  vp9_state->max_bit_rate = vp9_state-
> > > >target_bit_rate;
> > >  vp9_state->min_bit_rate = vp9_state-
> > > >target_bit_rate;
> > > -} else {
> > > -/* VBR mode */
> > > -brc_flag = VP9_BRC_SEQ | VP9_BRC_RC;
> > > -vp9_state->target_bit_rate = seq_param-
> > > >bits_per_second;
> > > -vp9_state->gop_size = seq_param->intra_period;
> > > -
> > > -if (vp9_state->brc_flag_check & VP9_BRC_FR) {
> > > -VAEncMiscParameterFrameRate *misc_param_fr;
> > > -
> > > -misc_param = (VAEncMiscParameterBuffer *)
> > > -encode_state-
> > > >misc_param[VAEncMiscParameterTypeFrameRate][0]->buffer;
> > > -misc_param_fr = (VAEncMiscParameterFrameRate
> > > *)misc_param->data;
> > > -
> > > -vp9_state->frame_rate = misc_param_fr-
> > > >framerate;
> > > -} else {
> > > -/* Assign the default frame rate */
> > > -vp9_state->frame_rate = 30;
> > > -}
> > > -
> > > -

Re: [Libva] [PATCH 1/4] i965_encoder: consistently represent framerate as a fraction

2016-12-21 Thread Xiang, Haihao 

> On 21/12/16 04:21, Xiang, Haihao wrote:
> > 
> > > Update references in both H.264 encoders (gen6_mfc and
> > > gen9_vdenc).
> > > 
> > > Signed-off-by: Mark Thompson 
> > > ---
> > > New version.
> > > 
> > > Changes for the whole series:
> > > * Use a single field for framerate (adding a new struct to
> > > represent
> > > it), as recommended by Haihao.
> > > * Fix some missed cases where bitrate was read from the initial
> > > sequence parameters rather than the most recent RC parameters.
> > > 
> > > The new struct is called "struct intel_fraction" to be consistent
> > > with the namespacing in the rest of that file, it could go
> > > somewhere
> > > else if that is preferred.  Relatedly, the code might be nicer
> > > with
> > > some more convenience functions around it (notably comparison and
> > > fraction -> double, which are both used in a number of places),
> > > but
> > > I'm not sure where they would go or what they would be called so
> > > I
> > > have not done this.
> > > 
> > > 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(-)
> > > ...
> > > @@ -480,7 +497,7 @@
> > > intel_encoder_check_framerate_parameter(VADriverContextP ctx,
> > >  struct
> > > intel_encoder_context
> > > *encoder_context,
> > >  VAEncMiscParameterFrameR
> > > ate
> > > *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 & 0x)
> > > -framerate_per_100s = (misc->framerate & 0x) * 100 /
> > > ((misc->framerate >> 16) & 0x);
> > > +framerate = (struct intel_fraction) { misc->framerate >>
> > > 16
> > > & 0x, misc->framerate & 0x };
> > 
> > 
> > 'misc->framerate >> 16 & 0x' is denominator, not numerator. 
> > 
> 
> Hmm.  Reading the comment in va/va.h again:
> 
> /*
>  * fps = numerator / denominator
>  * The high 2 bytes (bits 16 to 31) of framerate specifies the
> numerator, and
>  * the low 2 bytes (bits 0 to 15) of framerate specifies the
> denominator. For
>  * example, ((100 < 16 ) | 750) is 7.5 fps
>  *
>  * If the high 2 btyes is 0, the frame rate is specified by the
> low 2 bytes.
>  */
> unsigned int framerate;
> 
> the example and the text do not agree (I was following the text and
> didn't read the example carefully).  Looking at the previous code
> there, apparently the example is the one which should be followed?  

It was my fault when I added the comment. Yes, we should follow the
example, some applications have already uses the form of the example.
BTW the example should be ((100 << 16) | 750) :( 

> If you could confirm that this is the intention and will not break
> any other drivers, I will send a new patch to libva to fix the text
> of the comment (and also change my code to match).

Yes, please.

> 
> > 
> > BTW could you refine your patch series to avoid compiler warning?
> > 
> 
> Looking at these with gcc (5 or 6), I have:
> 
>   CC   libi965_drv_video_la-i965_encoder.lo
> ../../src/i965_encoder.c: In function ‘reduce_fraction’:
> ../../src/i965_encoder.c:49:5: warning: suggest parentheses around
> assignment used as truth value [-Wparentheses]
>  while (c = a % b) {
>  ^
> 
> -> add redundant parentheses to suppress the warning, in patch 1.
> 
>   CC   libi965_drv_video_la-gen8_mfc.lo
> ../../src/gen8_mfc.c: In function ‘gen8_mfc_vp8_hrd_context_init’:
> ../../src/gen8_mfc.c:3489:38: warning: unused variable ‘seq_param’ [-
> Wunused-variable]
>  VAEncSequenceParameterBufferVP8 *seq_param =
> (VAEncSequenceParameterBufferVP8 *)encode_state->seq_param_ext-
> >buffer;
>   ^
> 
> -> remove the now-unused variable, in patch 3.
> 
> Do you have any other warnings?  (I'll send a new series correcting
> these once the other questions are resolved.)

I just saw the above 2 warnings.

Thanks
Haihao


> 
> Thanks,
> 
> - Mark
> 
___
Libva mailing list
Libva@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libva

Re: [Libva] [PATCH 4/4] VP9 encoder: use generic rate control parameters

2016-12-21 Thread Mark Thompson 
On 21/12/16 04:46, Xiang, Haihao wrote:
> On Mon, 2016-12-19 at 23:01 +, Mark Thompson wrote:
>> Also adds support for fractional framerate.
>>
>> Signed-off-by: Mark Thompson 
>> ---
>>  src/gen9_vp9_encoder.c | 240 
>> -
>>  src/gen9_vp9_encoder.h |  10 +--
>>  src/i965_drv_video.c   |  10 +--
>>  src/i965_encoder.c |  36 
>>  src/i965_encoder.h |   1 +
>>  5 files changed, 77 insertions(+), 220 deletions(-)
>> ...
>>  /* check the corresponding BRC parameter for CBR and VBR */
>>  if (encoder_context->rate_control_mode == VA_RC_CBR) {
>> -vp9_state->target_bit_rate = seq_param->bits_per_second;
>> -vp9_state->gop_size = seq_param->intra_period;
>> -
>> -if (vp9_state->brc_flag_check & VP9_BRC_HRD) {
>> -VAEncMiscParameterHRD *misc_param_hrd;
>> -
>> -misc_param = (VAEncMiscParameterBuffer *)
>> -
>> encode_state->misc_param[VAEncMiscParameterTypeHRD][0]->buffer;
>> -misc_param_hrd = (VAEncMiscParameterHRD 
>> *)misc_param->data;
>> -
>> -vp9_state->init_vbv_buffer_fullness_in_bit = 
>> misc_param_hrd->initial_buffer_fullness;
>> -vp9_state->vbv_buffer_size_in_bit = 
>> misc_param_hrd->buffer_size;
>> -}
>> -
>> -if (vp9_state->brc_flag_check & VP9_BRC_FR) {
>> -VAEncMiscParameterFrameRate *misc_param_fr;
>> -
>> -misc_param = (VAEncMiscParameterBuffer *)
>> -
>> encode_state->misc_param[VAEncMiscParameterTypeFrameRate][0]->buffer;
>> -misc_param_fr = (VAEncMiscParameterFrameRate 
>> *)misc_param->data;
>> -
>> -vp9_state->frame_rate = misc_param_fr->framerate;
>> -} else {
>> -/* Assign the default frame rate */
>> -vp9_state->frame_rate = 30;
>> -}
>> -
>> -/* RC misc will override HRD parameter */
>> -if (vp9_state->brc_flag_check & VP9_BRC_RC) {
>> -VAEncMiscParameterRateControl *misc_param_rc;
>> -
>> -misc_param = (VAEncMiscParameterBuffer *)
>> -
>> encode_state->misc_param[VAEncMiscParameterTypeRateControl][0]->buffer;
>> -misc_param_rc = (VAEncMiscParameterRateControl 
>> *)misc_param->data;
>> -
>> -vp9_state->target_bit_rate = 
>> misc_param_rc->bits_per_second;
>> -vp9_state->vbv_buffer_size_in_bit = 
>> (misc_param_rc->bits_per_second / 1000) *
>> - misc_param_rc->window_size;
>> -vp9_state->init_vbv_buffer_fullness_in_bit = 
>> vp9_state->vbv_buffer_size_in_bit / 2;
>> -vp9_state->window_size = misc_param_rc->window_size;
>> -}
>> +if (!encoder_context->brc.framerate[0].num || 
>> !encoder_context->brc.framerate[0].den ||
>> +!encoder_context->brc.bits_per_second[0])
>> +return VA_STATUS_ERROR_INVALID_PARAMETER;
>> +
>> +vp9_state->gop_size = encoder_context->brc.gop_size;
>> +
>> +vp9_state->framerate = encoder_context->brc.framerate[0];
>> +vp9_state->target_bit_rate = 
>> encoder_context->brc.bits_per_second[0];
>>  vp9_state->max_bit_rate = vp9_state->target_bit_rate;
>>  vp9_state->min_bit_rate = vp9_state->target_bit_rate;
>> -} else {
>> -/* VBR mode */
>> -brc_flag = VP9_BRC_SEQ | VP9_BRC_RC;
>> -vp9_state->target_bit_rate = seq_param->bits_per_second;
>> -vp9_state->gop_size = seq_param->intra_period;
>> -
>> -if (vp9_state->brc_flag_check & VP9_BRC_FR) {
>> -VAEncMiscParameterFrameRate *misc_param_fr;
>> -
>> -misc_param = (VAEncMiscParameterBuffer *)
>> -
>> encode_state->misc_param[VAEncMiscParameterTypeFrameRate][0]->buffer;
>> -misc_param_fr = (VAEncMiscParameterFrameRate 
>> *)misc_param->data;
>> -
>> -vp9_state->frame_rate = misc_param_fr->framerate;
>> -} else {
>> -/* Assign the default frame rate */
>> -vp9_state->frame_rate = 30;
>> -}
>> -
>> -if (vp9_state->brc_flag_check & VP9_BRC_RC) {
>> -VAEncMiscParameterRateControl *misc_param_rc;
>> -
>> -misc_param = (VAEncMiscParameterBuffer *)
>> -
>> encode_state->misc_param[VAEncMiscParameterTypeRateControl][0]->buffer;
>> -misc_param_rc = (VAEncMiscParameterRateControl 
>> *)misc_param->data;