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

[Mark Thompson]

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,
>>                                          VAEncMiscParameterFrameRate
>> *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?  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).

> 
> 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.)

Thanks,

- Mark