> 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 <s...@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
>
_______________________________________________
Libva mailing list
Libva@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libva
