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

[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 <sw at jkqxz.net>
> > > ---
> > > 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 & 0xffff0000)
> > > -        framerate_per_100s = (misc->framerate & 0xffff) * 100 /
> > > ((misc->framerate >> 16) & 0xffff);
> > > +        framerate = (struct intel_fraction) { misc->framerate >>
> > > 16
> > > & 0xffff, misc->framerate & 0xffff };
> > 
> > 
> > 'misc->framerate >> 16 & 0xffff' 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
>