Re: [Libva] [PATCH 2/4] Set the pipeline to use the new VP8 encoding shaders on BSW

[Mark Thompson]

On 13/01/17 00:02, Xiang, Haihao wrote:
> 
> Thanks for the detailed info, I will look into the issue. BTW can you try 
> other vaapi based tools, such as yamitranscode?

Yes, I can now reproduce this with yamitranscode as well.

Given the video below, extract the H.264 stream and then:

./yamitranscode -i in.h264 -f 60 --rcmode CBR --ow 1920 --oh 1080 -c VP8 -b 500 
-N 1000 -o out.ivf

consistently gives both a broken output stream (though partially playable) and 
a GPU hang somewhere in the middle on Skylake.

Example GPU error dump: 
<http://ixia.jkqxz.net/~mrt/libva/vp8/sys_class_drm_card0_error_yami>.

Thanks,

- Mark


>> -Original Message-
>> From: Mark Thompson [mailto:s...@jkqxz.net]
>> Sent: Friday, January 13, 2017 6:11 AM
>> To: libva@lists.freedesktop.org; Xiang, Haihao <haihao.xi...@intel.com>
>> Subject: Re: [Libva] [PATCH 2/4] Set the pipeline to use the new VP8 encoding
>> shaders on BSW
>>
>> On 12/01/17 07:30, Xiang, Haihao wrote:
>>>
>>> Hi Mark,
>>>
>>> Can you reproduce the issue you mentioned below? If yes, I would like
>>> to fix it in the new version of the patch series.
>>
>> Hi,
>>
>> Yes, I can reproduce it consistently on both Skylake and Kaby Lake.  Some
>> detailed instructions follow...
>>
>>
>> Get standard test input:
>> <http://distribution.bbb3d.renderfarming.net/video/mp4/bbb_sunflower_10
>> 80p_60fps_normal.mp4>.  (The input file does matter somewhat: I tried some
>> others and found it harder to reproduce.  Maybe the highly variable
>> complexity here helps to show the problem.)
>>
>> Get current libav from git: .
>>
>> Apply patches adding framerate configuration and VP8 encode support to
>> libav: <http://ixia.jkqxz.net/~mrt/libva/vp8/0001-vaapi_encode-Pass-
>> framerate-parameters-to-driver.patch>,
>> <http://ixia.jkqxz.net/~mrt/libva/vp8/0002-vaapi_encode-Add-VP8-
>> support.patch>.
>>
>> Configure libav with --enable-vaapi and build.
>>
>>
>> Now run a transcode from the H.264 of the input file to VP8.  What happens
>> varies with the bitrate selected (this is now on Skylake GT2, 6300):
>>
>>
>> ./avconv -y -threads 1 -vaapi_device /dev/dri/renderD128 -hwaccel vaapi -
>> hwaccel_output_format vaapi -i bbb_sunflower_1080p_60fps_normal.mp4 -
>> an -c:v vp8_vaapi -r 60 -b:v 5M out.webm
>>
>> (CBR at 5Mbps)  Everything works and the output looks good: yay!  (This is an
>> immense improvement over the current driver - if you run the same
>> command there, the output is working but terrible quality.)
>>
>>
>> ./avconv -y -threads 1 -vaapi_device /dev/dri/renderD128 -hwaccel vaapi -
>> hwaccel_output_format vaapi -i bbb_sunflower_1080p_60fps_normal.mp4 -
>> an -c:v vp8_vaapi -r 60 -b:v 5M out.webm
>>
>> (CBR at 2Mbps)  Mostly works, but the output is broken at times and the
>> bitrate target is often missed by a long way.
>>
>>
>> ./avconv -y -threads 1 -vaapi_device /dev/dri/renderD128 -hwaccel vaapi -
>> hwaccel_output_format vaapi -i bbb_sunflower_1080p_60fps_normal.mp4 -
>> an -c:v vp8_vaapi -r 60 -b:v 1M out.webm
>>
>> (CBR at 1Mbps)  Consistently hangs at around frame 570.
>>
>>
>> [1321697.079583] drm/i915: Resetting chip after gpu hang [1321697.081571]
>> [drm] GuC firmware load skipped [1321699.063831] [drm] RC6 on
>>
>> /sys/class/drm/card0/error for one case:
>> <http://ixia.jkqxz.net/~mrt/libva/vp8/sys_class_drm_card0_error>.
>>
>> Backtrace of userspace at the hang point:
>>
>> #0  0x76351cc7 in ioctl () at ../sysdeps/unix/syscall-template.S:84
>> #1  0x75821708 in drmIoctl () from /usr/lib/x86_64-linux-
>> gnu/libdrm.so.2
>> #2  0x748e3e00 in ?? () from /usr/lib/x86_64-linux-
>> gnu/libdrm_intel.so.1
>> #3  0x748e4036 in ?? () from /usr/lib/x86_64-linux-
>> gnu/libdrm_intel.so.1
>> #4  0x74bded57 in intel_batchbuffer_flush (batch=0x56c03240)
>> at ../../src/intel_batchbuffer.c:147
>> #5  0x74b9c821 in i965_run_kernel_media_object
>> (ctx=0x56ad15b0, encoder_context=0x56bfff10,
>> gpe_context=0x56b6da38, media_function=11, param=0x7fffd5f0)
>> at ../../src/i965_encoder_vp8.c:2174
>> #6  0x74baa044 in i965_encoder_vp8_pak_tpu (ctx=0x56ad15b0,
>> encode_state=0x56ae5cd0, encoder_context=0x56bfff10)
>> at ../../src/i965_encoder_vp8.c:6503
>> #7  0x74baa5a7 in i965_encoder_vp8_pak_pipeline
>> (ctx=0x56ad15b0, profile=VAProfileVP8Version0_3,
>> 

Re: [Libva] [PATCH v1 0/9]Encoder Architecture Changes (Primarily AVC)

[Mark Thompson]

On 20/01/17 10:31, Qu, Pengfei wrote:
> 
> 
> From: Qu, Pengfei
> Sent: Friday, January 20, 2017 10:07 AM
> To: Mark Thompson <s...@jkqxz.net>; libva@lists.freedesktop.org
> Subject: RE: [Libva] [PATCH v1 0/9]Encoder Architecture Changes (Primarily 
> AVC)
> 
> 
> Update again.
> 
> 
> 
> -Original Message-
> From: Mark Thompson [mailto:s...@jkqxz.net]
> Sent: Thursday, January 19, 2017 8:25 AM
> To: libva@lists.freedesktop.org<mailto:libva@lists.freedesktop.org>; Qu, 
> Pengfei <pengfei...@intel.com<mailto:pengfei...@intel.com>>
> Subject: Re: [Libva] [PATCH v1 0/9]Encoder Architecture Changes (Primarily 
> AVC)
> 
> 
> 
> On 13/01/17 09:24, Pengfei Qu wrote:
> 
>> Encoder architecture restructuring for H.264 (with some impact to HEVC now) 
>> on HSW+
> 
>> * Improvements to the shaders
> 
>> * Improvements to the B frame efficiency
> 
>> * Improvements to the low bit rate mode
> 
>> * Improved features in two stage VME/PAK pipeline
> 
>>
> 
>> v1:
> 
>> Reduce the patch number and re org for VME and MFX related patches.
> 
>> Patch re org for VME pipeline
> 
>> Patch re org for MFX pipeline
> 
>> keep assert for internal logic and replace assert for input validation 
>> function.
> 
>> Remove unnecessary comments and enum value.
> 
>> Use the 64bit version OUT_BCS_RELOC64.
> 
>> Move kernel binary into header file.
> 
>> use misc parameter from encoder_context structure.
> 
> 
> 
> I've had a go with this on Skylake.  In general, I see significant gains in 
> quality with similar performance (yay), however I found some issues as well.
> 
> [Pengfei] It is great to know you try it. ☺ Yes. Quality improvement is as 
> our expectation.

:)

> CQP mode seems to have regressed significantly in speed - it is maybe 25% 
> slower than CBR/VBR now (though indeed higher quality, particularly on 
> B-frames).  Is this expected?  I would have thought it should be the 
> "easiest" (and therefore fastest) mode.
> 
> [Pengfei]yes, CQP is the easiest way. it is “quality level” related. i think 
> you are using “avcenc” to do test. One new parameter “quality level” will be 
> set in the driver by now, and “avcenc” does not set this parameter by now. So 
> in default mode, CQP use the “best quality level” and CBR/VBR use the “normal 
> quality level”, that is the reason why the CQP performance slower. I will add 
> support in the “avcenc” and also fix the same default “quality level” in the 
> driver.
> 
> [Pengfei] sorry. Double confirm, now the same quality level is used for CQP 
> and CBR/VBR in the default mode(Best quality level). I will do investigation 
> why CQP slower than CBR/VBR.

I have been using libavcodec, and I am already setting quality to maximise 
performance (I found this immediately because without the quality setting it is 
a huge speed regression against the current code, and quality is signficantly 
improved with the highest speed setting anyway).

To offer some numbers (on 6300) which became this conclusion:

Transcoding H.264 1080p -> 1080p:

 Current   Patched   Patched
   default   quality=7
CQP (30/30/36)231fps 91fps173fps
CBR (5Mbps)   189fps120fps231fps
VBR (5Mbps)   190fps119fps231fps

Transcoding H.264 1080p -> 1080p, total throughput with four instances:

 Current   Patched   Patched
   default   quality=7
CQP (30/30/36)339fps120fps222fps
CBR (5Mbps)   340fps168fps330fps
VBR (5Mbps)   337fps170fps331fps


> Also, there seems to be something funny going on in the VBR rate controller.  
> Sometimes (nondeterministically, with the same parameters) the beginning of 
> the stream gets stuck at a very high QP / low bitrate for a long period, 
> making the output video terrible quality.  After some time (maybe a few 
> thousand frames) it recovers and thereafter acts normally.  It seems to 
> happen entirely randomly with low probability (less than 10%, maybe?), with 
> no obvious connection to the encoding parameters.
> 
> 
> 
> I found some things which might be related (but equally could just be 
> perturbing something else, for example by changing the timing):
> 
> * It never seems to happen if the encoder input comes directly from a decoder 
> - I have only seen it when there is a VPP instance in between them (though it 
> need not do anything to the video - it can just copy to a new surface of the 
> same size).
> 
> * I tested on two different machines and it only seems to happen on one of 
> them: it happens on a 6260U (GT3), but not on a 63

Re: [Libva] [PATCH v1 7/9] ENC: add VME pipeline for AVC encoder

[Mark Thompson]

On 13/01/17 09:24, Pengfei Qu wrote:
> VME pipeline:
> add resource and surface allocation and free function
> add init table for frame mbbrc update
> add scaling kernel for AVC encoder
> add BRC init reset kernel for AVC RC logic
> add BRC frame update-kernel for AVC RC logic
> add BRC MB level update kernel for AVC RC logic
> add REF frame QA caculation and MB level const data
> add MBENC kernel for AVC encoder
> add ME kernel for AVC encoder
> add WP/SFD kernel for AVC encoder
> add kernel init/destroy function for AVC encoder
> add kernel related parameter check function for AVC
> add VME pipeline init prepare/run function for AVC encoder
> 
> Reviewed-by: Sean V Kelley
> Signed-off-by: Pengfei Qu 
> ---
>  src/gen9_avc_encoder.c | 5745 
> 
>  1 file changed, 5745 insertions(+)
>  create mode 100755 src/gen9_avc_encoder.c

This patch and the following one are confused about the meaning of 
generic_enc_codec_state.internal_rate_mode - it appears both as a VA_RC_* value 
(bitmask, values 0x01 to 0x20) and as an INTEL_BRC_* value (#defined like an 
enum, values 0-4).

It happens that both VA_RC_CBR and INTEL_BRC_CBR have the same value (2) which 
probably means that that mode is fine, but I think it may be having weird 
effects on other modes - particularly VBR because VA_RC_VBR == INTEL_BRC_AVBR 
(4).

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

Re: [Libva] [PATCH v1 0/9]Encoder Architecture Changes (Primarily AVC)

[Mark Thompson]

On 13/01/17 09:24, Pengfei Qu wrote:
> Encoder architecture restructuring for H.264 (with some impact to HEVC now) 
> on HSW+
> * Improvements to the shaders
> * Improvements to the B frame efficiency
> * Improvements to the low bit rate mode
> * Improved features in two stage VME/PAK pipeline
> 
> v1:
> Reduce the patch number and re org for VME and MFX related patches.
> Patch re org for VME pipeline
> Patch re org for MFX pipeline
> keep assert for internal logic and replace assert for input validation 
> function.
> Remove unnecessary comments and enum value.
> Use the 64bit version OUT_BCS_RELOC64.
> Move kernel binary into header file.
> use misc parameter from encoder_context structure.

I've had a go with this on Skylake.  In general, I see significant gains in 
quality with similar performance (yay), however I found some issues as well.

CQP mode seems to have regressed significantly in speed - it is maybe 25% 
slower than CBR/VBR now (though indeed higher quality, particularly on 
B-frames).  Is this expected?  I would have thought it should be the "easiest" 
(and therefore fastest) mode.

Also, there seems to be something funny going on in the VBR rate controller.  
Sometimes (nondeterministically, with the same parameters) the beginning of the 
stream gets stuck at a very high QP / low bitrate for a long period, making the 
output video terrible quality.  After some time (maybe a few thousand frames) 
it recovers and thereafter acts normally.  It seems to happen entirely randomly 
with low probability (less than 10%, maybe?), with no obvious connection to the 
encoding parameters.

I found some things which might be related (but equally could just be 
perturbing something else, for example by changing the timing):
* It never seems to happen if the encoder input comes directly from a decoder - 
I have only seen it when there is a VPP instance in between them (though it 
need not do anything to the video - it can just copy to a new surface of the 
same size).
* I tested on two different machines and it only seems to happen on one of 
them: it happens on a 6260U (GT3), but not on a 6300 (GT2).

Can you offer any thoughts on what might be relevant which I could test for?  
(Currently my reproduction method is just "transcode videos between sizes 
repeatedly until it happens", which I realise is not very helpful.  I am happy 
to try to narrow that down a bit if I could have any idea what I can look for.)

Thanks,

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

Re: [Libva] [PATCH 2/4] Set the pipeline to use the new VP8 encoding shaders on BSW

[Mark Thompson]

55c43ce in poll_filter (ost=0x56c61680) at 
/home/mrt/video/libav/vp8/avconv.c:718
#19 0x555c4753 in poll_filters () at 
/home/mrt/video/libav/vp8/avconv.c:792
#20 0x555cab23 in transcode () at 
/home/mrt/video/libav/vp8/avconv.c:2730
#21 0x555cb063 in main (argc=20, argv=0x7fffe438) at 
/home/mrt/video/libav/vp8/avconv.c:2898


avconv there also supports CQP mode as well as CBR (pass "-qindex N" for N 
between 0 and 127 instead of the "-b:v M" above): this is always fine for me.

Can you reproduce a hang with that?  Is there any other information I can 
provide to help?

Thanks,

- Mark


>>> On Tue, Jan 10, 2017 at 4:21 PM, Mark Thompson <s...@jkqxz.net> wrote:
>>>> On 10/01/17 22:02, Sean V Kelley wrote:
>>>>> From: "Xiang, Haihao" <haihao.xi...@intel.com>
>>>>>
>>>>> Currently only one temporal layer is supported
>>>>>
>>>>> Signed-off-by: Xiang, Haihao <haihao.xi...@intel.com>
>>>>> Reviewed-by: Sean V Kelley <sea...@posteo.de>
>>>>> ---
>>>>>   src/Makefile.am|3 +
>>>>>   src/gen8_encoder_vp8.c |  140 +
>>>>>   src/gen8_mfc.c |8 +-
>>>>>   src/gen8_vme.c |5 +
>>>>>   src/i965_defines.h |   10 +
>>>>>   src/i965_encoder.c |2 +
>>>>>   src/i965_encoder_vp8.c | 6697
>>>> 
>>>>>   src/i965_encoder_vp8.h | 2643 +++
>>>>>   8 files changed, 9507 insertions(+), 1 deletion(-)
>>>>
>>>> I had a go with this on Kaby Lake.  In general, big win - looks like it
>>>> can
>>>> be under half the bitrate at comparable quality (though it was pretty
>>>> terrible before...).
>>>>
>>>> However, the rate control seems to do odd things at low bitrate relative
>>>> to
>>>> the frame size?  I can get GPU hangs and wildly varying output bitrate
>>>> with
>>>> it, though it seems ok at high bitrate.
>>> That's a concern.  Please report the If it really is a GPU hang, I need
>>> the error report for the DRM card0 log.
>>>
>>> cat /sys/class/drm/card0/error
>>>
>>> Please rerun and capture the DRM (i915) card0 error log.
>>>  
>>>>  
>>>> I had a look around the rate control and found two minor issues in the RC
>>>> configuration, though I don't think either of them are relevant to my
>>>> problem (see below).  I can try to make a reproducer if this is not
>>>> already
>>>> known?
>>>>
>>> Please do attempt to reproduce.  That's why I've put the patches out here to
>>> test.
>>
>> Thanks for testing the patch, could you detail the steps to reproduce this
>> issue?
>>
>>
>>>
>>> Thanks,
>>>
>>> Sean
>>>  
>>>>  Thanks,
>>>>
>>>> - Mark
>>>>

___
Libva mailing list
Libva@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libva

Re: [Libva] [PATCH 00/31] Encoder Architecture Changes (Primarily AVC)

[Mark Thompson]

On 10/01/17 23:37, Sean V Kelley wrote:
> Encoder architecture restructuring for H.264 (with some impact to HEVC now) 
> on HSW+
> * Improvements to the shaders
> * Improvements to the B frame efficiency
> * Improvements to the low bit rate mode
> * Improved features in two stage VME/PAK pipeline
> 
> 
> Pengfei Qu (31):
>   ENC: move gpe related function into src/i965_gpe_utils.h/c
>   ENC: add common structure for AVC/HEVC encoder
>   ENC:add context init function for AVC/HEVC encoder
>   ENC: add const data/table for AVC encoder
>   ENC: add AVC kernel binary on SKL

This patch (5/31) is missing?  (Not in the archive either: 
.)

Looks like it is this part:

>  src/gen9_avc_encoder.h |  2345 
>  src/gen9_avc_encoder_kernels.c | 12081 
> +++

so maybe it was rejected by the ML for being too big?

Thanks,

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

Re: [Libva] [PATCH 2/4] Set the pipeline to use the new VP8 encoding shaders on BSW

[Mark Thompson]

On 10/01/17 22:02, Sean V Kelley wrote:
> From: "Xiang, Haihao" 
> 
> Currently only one temporal layer is supported
> 
> Signed-off-by: Xiang, Haihao 
> Reviewed-by: Sean V Kelley 
> ---
>  src/Makefile.am|3 +
>  src/gen8_encoder_vp8.c |  140 +
>  src/gen8_mfc.c |8 +-
>  src/gen8_vme.c |5 +
>  src/i965_defines.h |   10 +
>  src/i965_encoder.c |2 +
>  src/i965_encoder_vp8.c | 6697 
> 
>  src/i965_encoder_vp8.h | 2643 +++
>  8 files changed, 9507 insertions(+), 1 deletion(-)

I had a go with this on Kaby Lake.  In general, big win - looks like it can be 
under half the bitrate at comparable quality (though it was pretty terrible 
before...).

However, the rate control seems to do odd things at low bitrate relative to the 
frame size?  I can get GPU hangs and wildly varying output bitrate with it, 
though it seems ok at high bitrate.

I had a look around the rate control and found two minor issues in the RC 
configuration, though I don't think either of them are relevant to my problem 
(see below).  I can try to make a reproducer if this is not already known?

Thanks,

- Mark


> ...
> +
> +static void
> +i965_encoder_vp8_get_misc_parameters(VADriverContextP ctx,
> + struct encode_state *encode_state,
> + struct intel_encoder_context 
> *encoder_context)
> +{
> +struct i965_encoder_vp8_context *vp8_context = 
> encoder_context->vme_context;
> +
> +if (vp8_context->internal_rate_mode == I965_BRC_CQP) {
> +vp8_context->init_vbv_buffer_fullness_in_bit = 0;
> +vp8_context->vbv_buffer_size_in_bit = 0;
> +vp8_context->target_bit_rate = 0;
> +vp8_context->max_bit_rate = 0;
> +vp8_context->min_bit_rate = 0;
> +vp8_context->brc_need_reset = 0;
> +} else {
> +vp8_context->gop_size = encoder_context->brc.gop_size;
> +
> +if (encoder_context->brc.need_reset) {
> +vp8_context->framerate = encoder_context->brc.framerate[0];
> +vp8_context->vbv_buffer_size_in_bit = 
> encoder_context->brc.hrd_buffer_size;
> +vp8_context->init_vbv_buffer_fullness_in_bit = 
> encoder_context->brc.hrd_initial_buffer_fullness;
> +vp8_context->max_bit_rate = 
> encoder_context->brc.bits_per_second[0]; // currently only one layer is 
> supported
> +vp8_context->brc_need_reset = (vp8_context->brc_initted && 
> encoder_context->brc.need_reset);
> +
> +if (vp8_context->internal_rate_mode == I965_BRC_CBR) {
> +vp8_context->min_bit_rate = vp8_context->max_bit_rate;
> +vp8_context->target_bit_rate = vp8_context->max_bit_rate;
> +} else {
> +assert(vp8_context->internal_rate_mode == I965_BRC_VBR);
> +vp8_context->min_bit_rate = vp8_context->max_bit_rate * (2 * 
> encoder_context->brc.target_percentage[0] - 100) / 100;

If target percentage is < 50 then (2 * 
encoder_context->brc.target_percentage[0] - 100) is negative.  Since it's 
unsigned, you end up with a garbage number in min_bit_rate.

> +vp8_context->target_bit_rate = vp8_context->max_bit_rate * 
> encoder_context->brc.target_percentage[0] / 100;
> +}
> +}
> +}
> +
> +if (encoder_context->quality_level == ENCODER_LOW_QUALITY)
> +vp8_context->hme_16x_supported = 0;
> +}
> +
> ...
> +
> +static void
> +i965_encoder_vp8_vme_brc_init_reset_set_curbe(VADriverContextP ctx,
> +  struct encode_state 
> *encode_state,
> +  struct intel_encoder_context 
> *encoder_context,
> +  struct i965_gpe_context 
> *gpe_context)
> +{
> +struct i965_encoder_vp8_context *vp8_context = 
> encoder_context->vme_context;
> +VAEncPictureParameterBufferVP8 *pic_param = 
> (VAEncPictureParameterBufferVP8 *)encode_state->pic_param_ext->buffer;
> +struct vp8_brc_init_reset_curbe_data *pcmd = 
> i965_gpe_context_map_curbe(gpe_context);
> +double input_bits_per_frame, bps_ratio;
> +
> +memset(pcmd, 0, sizeof(*pcmd));
> +
> +pcmd->dw0.profile_level_max_frame = vp8_context->frame_width * 
> vp8_context->frame_height;
> +pcmd->dw1.init_buf_full_in_bits = 
> vp8_context->init_vbv_buffer_fullness_in_bit;
> +pcmd->dw2.buf_size_in_bits = vp8_context->vbv_buffer_size_in_bit;
> +pcmd->dw3.average_bitrate = ALIGN(vp8_context->target_bit_rate, 
> VP8_BRC_KBPS) / VP8_BRC_KBPS * VP8_BRC_KBPS;
> +pcmd->dw4.max_bitrate = ALIGN(vp8_context->max_bit_rate, VP8_BRC_KBPS) / 
> VP8_BRC_KBPS * VP8_BRC_KBPS;

VP8_BRC_KBPS is 1000 which is not a power of two, so the ALIGN macro isn't 
doing anything sensible here.

> +pcmd->dw6.frame_rate_m = 

[Libva] [PATCH v3] H.264 encoder: add a simple reactive VBR rate control mode

[Mark Thompson]

This implements a simple reactive VBR rate control mode for single-layer H.264.
The primary aim here is to avoid the problematic behaviour that the CBR rate
controller displays on scene changes, where the QP can get pushed up by a large
amount in a short period and compromise the quality of following frames to a
very visible degree.

The main idea, then, is to try to keep the HRD buffering above the target level
most of the time, so that when a large frame is generated (on a scene change or
when the stream complexity increases) we have plenty of slack to be able to
encode the more difficult region without compromising quality immediately on
the following frames.   It is optimistic about the complexity of future frames,
so even after generating one or more large frames on a significant change it
will try to keep the QP at its current level until the HRD buffer bounds force
a change to maintain the intended rate.

Compared to the CBR rate controller, it keeps the quality level much more
stable - QP does not always spike up as large frames are generated when the
complexity of the stream increases transiently, but equally it does not reduce
as quickly when the complexity of the stream decreases.

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
On 09/01/17 05:23, Xiang, Haihao wrote:
>> +BRC_CLIP(mfc_context->brc.qp_prime_y[0][SLICE_TYPE_I], 
>> (int)encoder_context->brc.min_qp, 51);
>> +BRC_CLIP(mfc_context->brc.qp_prime_y[0][SLICE_TYPE_P], 
>> (int)encoder_context->brc.min_qp, 51);
>> +BRC_CLIP(mfc_context->brc.qp_prime_y[0][SLICE_TYPE_B], 
>> (int)encoder_context->brc.min_qp, 51);
> 
> The lower bound is 1 when encoder_context->brc.min_qp is equal to 0.
> 
>> +
>> +if (sts == BRC_UNDERFLOW && qp[slice_type] == 51)
>> +sts = BRC_UNDERFLOW_WITH_MAX_QP;
>> +if (sts == BRC_OVERFLOW && qp[slice_type] == 
>> encoder_context->brc.min_qp)
> 
> Same as above

Apologies, I missed updating it to match 
33a32935ac9e2622adc5c59045d565b4e5904749.

Fixed in the same way as that patch in the version.

Thanks,

- Mark


 src/gen6_mfc.c|  10 ++--
 src/gen6_mfc_common.c | 126 --
 src/gen75_mfc.c   |  10 ++--
 src/gen8_mfc.c|  10 ++--
 src/i965_drv_video.c  |   5 +-
 5 files changed, 141 insertions(+), 20 deletions(-)

diff --git a/src/gen6_mfc.c b/src/gen6_mfc.c
index 8077c14..1765530 100644
--- a/src/gen6_mfc.c
+++ b/src/gen6_mfc.c
@@ -798,7 +798,7 @@ gen6_mfc_avc_pipeline_slice_programing(VADriverContextP ctx,
 int qp_mb;
 
 qp_slice = qp;
-if (rate_control_mode == VA_RC_CBR) {
+if (rate_control_mode != VA_RC_CQP) {
 qp = 
mfc_context->brc.qp_prime_y[encoder_context->layer.curr_frame_layer_id][slice_type];
 if (encode_state->slice_header_index[slice_index] == 0) {
 pSliceParameter->slice_qp_delta = qp - pPicParameter->pic_init_qp;
@@ -816,7 +816,7 @@ gen6_mfc_avc_pipeline_slice_programing(VADriverContextP ctx,
  pPicParameter,
  pSliceParameter,
  encode_state, encoder_context,
- (rate_control_mode == VA_RC_CBR), qp_slice, 
slice_batch);
+ (rate_control_mode != VA_RC_CQP), qp_slice, 
slice_batch);
 
 if ( slice_index == 0) 
 intel_mfc_avc_pipeline_header_programing(ctx, encode_state, 
encoder_context, slice_batch);
@@ -1188,7 +1188,7 @@ gen6_mfc_avc_batchbuffer_slice(VADriverContextP ctx,
 int qp_slice;
 
 qp_slice = qp;
-if (rate_control_mode == VA_RC_CBR) {
+if (rate_control_mode != VA_RC_CQP) {
 qp = 
mfc_context->brc.qp_prime_y[encoder_context->layer.curr_frame_layer_id][slice_type];
 if (encode_state->slice_header_index[slice_index] == 0) {
 pSliceParameter->slice_qp_delta = qp - pPicParameter->pic_init_qp;
@@ -1209,7 +1209,7 @@ gen6_mfc_avc_batchbuffer_slice(VADriverContextP ctx,
  pSliceParameter,
  encode_state,
  encoder_context,
- (rate_control_mode == VA_RC_CBR),
+ (rate_control_mode != VA_RC_CQP),
  qp_slice,
  slice_batch);
 
@@ -1368,7 +1368,7 @@ gen6_mfc_avc_encode_picture(VADriverContextP ctx,
 /*Programing bcs pipeline*/
 gen6_mfc_avc_pipeline_programing(ctx, encode_state, encoder_context);  
//filling the pipeline
 gen6_mfc_run(ctx, encode_state, encoder_context);
-if (rate_control_mode == VA_RC_CBR /*|| rate_control_mode == 
VA_RC_VBR*/) {
+if (rate_control_mode == VA_RC_CBR || rate_control_mode == VA_RC_VBR) {
 gen6_mfc_stop(ctx, encode_state, encoder_context, 
_frame_bits_si

Re: [Libva] [Libva-intel-driver][PATCH 2/2] Encoder: release all misc parameter buffers

[Mark Thompson]

On 04/01/17 01:40, Xiang, Haihao wrote:
> User can still use the old setting if needed because the setting is
> stored in a common structure now.
> 
> Signed-off-by: Xiang, Haihao 
> ---
>  src/i965_drv_video.c | 15 +--
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/src/i965_drv_video.c b/src/i965_drv_video.c
> index 51a708c..76cb915 100644
> --- a/src/i965_drv_video.c
> +++ b/src/i965_drv_video.c
> @@ -2798,7 +2798,7 @@ i965_BeginPicture(VADriverContextP ctx,
>  struct object_surface *obj_surface = SURFACE(render_target);
>  struct object_config *obj_config;
>  VAStatus vaStatus = VA_STATUS_SUCCESS;
> -int i;
> +int i, j;
>  
>  ASSERT_RET(obj_context, VA_STATUS_ERROR_INVALID_CONTEXT);
>  ASSERT_RET(obj_surface, VA_STATUS_ERROR_INVALID_SURFACE);
> @@ -2841,15 +2841,10 @@ i965_BeginPicture(VADriverContextP ctx,
>  obj_context->codec_state.encode.num_packed_header_data_ext = 0;
>  obj_context->codec_state.encode.slice_index = 0;
>  obj_context->codec_state.encode.vps_sps_seq_index = 0;
> -/*
> -* Based on ROI definition in va/va.h, the ROI set through this
> -* structure is applicable only to the current frame or field.
> -* That is to say: it is on-the-fly setting. If it is not set,
> -* the current frame doesn't use ROI.
> -* It is uncertain whether the other misc buffer should be released.
> -* So only release the previous ROI buffer.
> -*/
> -
> i965_release_buffer_store(_context->codec_state.encode.misc_param[VAEncMiscParameterTypeROI][0]);
> +
> +for (i = 0; i < 
> ARRAY_ELEMS(obj_context->codec_state.encode.misc_param); i++)
> +for (j = 0; j < 
> ARRAY_ELEMS(obj_context->codec_state.encode.misc_param[0]); j++)
> +
> i965_release_buffer_store(_context->codec_state.encode.misc_param[i][j]);
>  
>  
> i965_release_buffer_store(_context->codec_state.encode.encmb_map);
>  } else {
> 

LGTM (tested with libavcodec).

Thanks,

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

[Libva] [PATCH v2] H.264 encoder: respect initial QP setting

[Mark Thompson]

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
On 04/01/17 08:50, Xiang, Haihao wrote:
> Could you send your new revision in a separate email with a version number in 
> the future? I thought this email was just a reply
> to the previous email. 

Ok, will do.

> BTW I failed to apply the 2nd patch in the patch series by git-am. Could you 
> rebase the 2nd patch?

Sure.  Here it is rebased against v2 of the min-qp patch (and otherwise 
unchanged).

Thanks,

- Mark


 src/gen6_mfc_common.c | 24 +++-
 src/i965_encoder.c|  2 ++
 src/i965_encoder.h|  1 +
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/src/gen6_mfc_common.c b/src/gen6_mfc_common.c
index 90cee05..1643efc 100644
--- a/src/gen6_mfc_common.c
+++ b/src/gen6_mfc_common.c
@@ -169,16 +169,22 @@ static void intel_mfc_brc_init(struct encode_state 
*encode_state,
 
 bpf = mfc_context->brc.bits_per_frame[i] = bitrate/framerate;
 
-if ((bpf > qp51_size) && (bpf < qp1_size)) {
-mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51 - 50*(bpf - 
qp51_size)/(qp1_size - qp51_size);
-}
-else if (bpf >= qp1_size)
-mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 1;
-else if (bpf <= qp51_size)
-mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51;
+if (encoder_context->brc.initial_qp) {
+mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I] = 
encoder_context->brc.initial_qp;
+mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 
encoder_context->brc.initial_qp;
+mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B] = 
encoder_context->brc.initial_qp;
+} else {
+if ((bpf > qp51_size) && (bpf < qp1_size)) {
+mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51 - 50*(bpf - 
qp51_size)/(qp1_size - qp51_size);
+}
+else if (bpf >= qp1_size)
+mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 1;
+else if (bpf <= qp51_size)
+mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51;
 
-mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I] = 
mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P];
-mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B] = 
mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I];
+mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I] = 
mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P];
+mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B] = 
mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I];
+}
 
 BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I], min_qp, 51);
 BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P], min_qp, 51);
diff --git a/src/i965_encoder.c b/src/i965_encoder.c
index 3056900..0a648d4 100644
--- a/src/i965_encoder.c
+++ b/src/i965_encoder.c
@@ -576,8 +576,10 @@ 
intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
 }
 
 if (encoder_context->brc.window_size != misc->window_size ||
+encoder_context->brc.initial_qp  != misc->initial_qp ||
 encoder_context->brc.min_qp  != misc->min_qp) {
 encoder_context->brc.window_size = misc->window_size;
+encoder_context->brc.initial_qp  = misc->initial_qp;
 encoder_context->brc.min_qp  = misc->min_qp;
 encoder_context->brc.need_reset = 1;
 }
diff --git a/src/i965_encoder.h b/src/i965_encoder.h
index 16a6f3f..829df9d 100644
--- a/src/i965_encoder.h
+++ b/src/i965_encoder.h
@@ -92,6 +92,7 @@ struct intel_encoder_context
 unsigned int hrd_buffer_size;
 unsigned int hrd_initial_buffer_fullness;
 unsigned int window_size;
+unsigned int initial_qp;
 unsigned int min_qp;
 unsigned int need_reset;
 
-- 
2.11.0
___
Libva mailing list
Libva@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libva

Re: [Libva] [Libva-intel-driver][PATCH] Needn't reset brc if the bitrate setting isn't changed in the Begin/Render/End sequence

[Mark Thompson]

On 03/01/17 06:42, Xiang, Haihao wrote:
> On Fri, 2016-12-30 at 17:44 +0000, Mark Thompson wrote:
>> On 30/12/16 00:51, Xiang, Haihao wrote:
>>> On Thu, 2016-12-29 at 18:52 +0000, 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:
>>>>
>>>> Sequence parameters with bps = 1M
>>>> -> IDR frame, at 1Mbps
>>>> -> some P frames, at 1Mbps
>>>> RC parameters with bps = 2M
>>>> -> some P frames, at 2Mbps
>>>> Sequence parameters with bps = 3M
>>>> -> IDR frame, at 3Mbps
>>>> -> some P frames, at 3Mbps
>>>
>>>
>>> The 2nd sequence is generated at 3Mbps with the current
>>> logic. hl_bitrate_updated is 0 for your case and we use the
>>> sequence
>>> parameters.
>>
>> This is not the case - in the absence of a new RC parameter
>> structure, the one that was set in the middle of the first sequence
>> continues to apply forever (encoder_context->misc_param[] is never
>> cleared), so hl_bitrate_updated is always 1 after that point.
>>
> 
> You are right, I forgot encoder_context->misc_param[] 
> (except VAEncMiscParameterTypeROI) is not cleared presently. It would
> be better we use the same way for all misc parameters, I will file a
> patch to clear encoder_context->misc_param[]. 

Ok, sounds good :)

Thanks,

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

[Libva] [PATCH] H.264 encoder: add a simple VBR rate control mode

[Mark Thompson]

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
From the comment in the code:

// This implements a simple reactive VBR rate controller for single-layer H.264.
// The main idea here is to try to keep the HRD buffer above the target level 
most of the time,
// so that when a large frame is generated (on a scene change) we have plenty 
of slack to be
// able to encode it without compromising quality on the following frames.  It 
is optimistic
// about the complexity of future frames, so after generating a large frame on 
a significant
// change (particularly whole-screen transitions) it will try to keep the QP at 
its current
// level unless the HRD buffer bounds force a change to maintain the intended 
rate.

The primary aim of this is to avoid the problematic behaviour that the CBR rate 
controller has on scene changes, where the QP can get pushed up by a large 
amount and compromise the quality of following frames to a very visible degree.

To visualise the effect of it, here is the QP and frame sizes of same sequence 
encoded with the same parameters with the CBR and VBR RC modes:

<http://ixia.jkqxz.net/~mrt/libva/rc/cbr.svg>
<http://ixia.jkqxz.net/~mrt/libva/rc/vbr.svg>

(The two graphs have identical scales.  The sequence is the first 1 frames 
of Big Buck Bunny (which usefully has very varied complexity): 1280x720 at 
60fps, target bitrate 2Mbps, HRD buffer 12Mb, 250 frame GOP, 2 B frames, min QP 
18, initial QP 32.)

Note in particular how the spikes in QP from the CBR rate controller are mostly 
avoided (around frames 1600, 3100, 6300, 9100 in the example), and how the VBR 
mode has much less variation in the QP level.  Also note how the VBR mode often 
has higher average QP than the CBR mode does, particularly when complexity is 
decreasing - this is what is lost in the attempt to improve the worst-case 
behaviour.

Written and tested on gen9; hopefully it works on the older platforms too 
though I haven't actually tested it.  It only works for single-layer video, I 
haven't considered multiple-layer video at all - probably it wants some code to 
at least reject that case, but I don't have any test setup for that so I've 
avoided it for now.

Thanks,

- Mark


 src/gen6_mfc.c|  10 ++---
 src/gen6_mfc_common.c | 118 --
 src/gen75_mfc.c   |  10 ++---
 src/gen8_mfc.c|  10 ++---
 src/i965_drv_video.c  |   5 ++-
 5 files changed, 133 insertions(+), 20 deletions(-)

diff --git a/src/gen6_mfc.c b/src/gen6_mfc.c
index 8077c14..1765530 100644
--- a/src/gen6_mfc.c
+++ b/src/gen6_mfc.c
@@ -798,7 +798,7 @@ gen6_mfc_avc_pipeline_slice_programing(VADriverContextP ctx,
 int qp_mb;
 
 qp_slice = qp;
-if (rate_control_mode == VA_RC_CBR) {
+if (rate_control_mode != VA_RC_CQP) {
 qp = 
mfc_context->brc.qp_prime_y[encoder_context->layer.curr_frame_layer_id][slice_type];
 if (encode_state->slice_header_index[slice_index] == 0) {
 pSliceParameter->slice_qp_delta = qp - pPicParameter->pic_init_qp;
@@ -816,7 +816,7 @@ gen6_mfc_avc_pipeline_slice_programing(VADriverContextP ctx,
  pPicParameter,
  pSliceParameter,
  encode_state, encoder_context,
- (rate_control_mode == VA_RC_CBR), qp_slice, 
slice_batch);
+ (rate_control_mode != VA_RC_CQP), qp_slice, 
slice_batch);
 
 if ( slice_index == 0) 
 intel_mfc_avc_pipeline_header_programing(ctx, encode_state, 
encoder_context, slice_batch);
@@ -1188,7 +1188,7 @@ gen6_mfc_avc_batchbuffer_slice(VADriverContextP ctx,
 int qp_slice;
 
 qp_slice = qp;
-if (rate_control_mode == VA_RC_CBR) {
+if (rate_control_mode != VA_RC_CQP) {
 qp = 
mfc_context->brc.qp_prime_y[encoder_context->layer.curr_frame_layer_id][slice_type];
 if (encode_state->slice_header_index[slice_index] == 0) {
 pSliceParameter->slice_qp_delta = qp - pPicParameter->pic_init_qp;
@@ -1209,7 +1209,7 @@ gen6_mfc_avc_batchbuffer_slice(VADriverContextP ctx,
  pSliceParameter,
  encode_state,
  encoder_context,
- (rate_control_mode == VA_RC_CBR),
+ (rate_control_mode != VA_RC_CQP),
  qp_slice,
  slice_batch);
 
@@ -1368,7 +1368,7 @@ gen6_mfc_avc_encode_picture(VADriverContextP ctx,
 /*Programing bcs pipeline*/
 gen6_mfc_avc_pipeline_programing(ctx, encode_state, encoder_context);  
//filling the pipeline
 gen6_mfc_run(ctx, encode_state, encoder_context);
-if (rate_control_mode == VA_RC_CBR /*|| rate_control_mode == 
VA_RC_VBR*/) {
+if (rate_control_mode == VA_RC_CBR || rate_control_mode == VA_RC_VBR) {
 gen6_mfc_stop(ctx, 

Re: [Libva] [Libva-intel-driver][PATCH] Needn't reset brc if the bitrate setting isn't changed in the Begin/Render/End sequence

[Mark Thompson]

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.

Thanks,

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

Re: [Libva] [Libva-intel-driver][PATCH] Needn't reset brc if the bitrate setting isn't changed in the Begin/Render/End sequence

[Mark Thompson]

On 30/12/16 00:51, Xiang, Haihao wrote:
> On Thu, 2016-12-29 at 18:52 +0000, 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:
>>
>> Sequence parameters with bps = 1M
>> -> IDR frame, at 1Mbps
>> -> some P frames, at 1Mbps
>> RC parameters with bps = 2M
>> -> some P frames, at 2Mbps
>> Sequence parameters with bps = 3M
>> -> IDR frame, at 3Mbps
>> -> some P frames, at 3Mbps
> 
> 
> The 2nd sequence is generated at 3Mbps with the current
> logic. hl_bitrate_updated is 0 for your case and we use the sequence
> parameters.

This is not the case - in the absence of a new RC parameter structure, the one 
that was set in the middle of the first sequence continues to apply forever 
(encoder_context->misc_param[] is never cleared), so hl_bitrate_updated is 
always 1 after that point.

To show that more concretely, with this patch applied to print the internal 
state:

diff --git a/src/i965_encoder.c b/src/i965_encoder.c
index 0a648d4..a7c92ce 100644
--- a/src/i965_encoder.c
+++ b/src/i965_encoder.c
@@ -718,6 +718,12 @@ intel_encoder_check_brc_parameter(VADriverContextP ctx,
 
 }
 
+printf("hl_bitrate_updated = %d, is_new_sequence = %d, need_reset = %d, "
+   "seq_bits_per_second = %d, brc bits_per_second = %d\n",
+   hl_bitrate_updated, encoder_context->is_new_sequence,
+   encoder_context->brc.need_reset, seq_bits_per_second,
+   encoder_context->brc.bits_per_second[0]);
+
 return VA_STATUS_SUCCESS;
 }
 

I then encode with a GOP size of 2 and and bitrate increasing by 1M on each IDR 
frame, providing RC parameters only with the first:

hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 1, 
seq_bits_per_second = 100, brc bits_per_second = 100
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, 
seq_bits_per_second = 0, brc bits_per_second = 100
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, 
seq_bits_per_second = 200, brc bits_per_second = 100
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, 
seq_bits_per_second = 0, brc bits_per_second = 100
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, 
seq_bits_per_second = 300, brc bits_per_second = 100
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, 
seq_bits_per_second = 0, brc bits_per_second = 100
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, 
seq_bits_per_second = 400, brc bits_per_second = 100
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, 
seq_bits_per_second = 0, brc bits_per_second = 100
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, 
seq_bits_per_second = 500, brc bits_per_second = 100
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, 
seq_bits_per_second = 0, brc bits_per_second = 100
hl_bitrate_updated = 1, is_new_sequence = 1, need

[Libva] [PATCH 2/2] H.264 encoder: respect initial QP setting

[Mark Thompson]

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
This can be set by the user to ensure that the first frame of a stream is 
encoded at with a good QP (often it starts too high and takes a few frames to 
get down to the right range).  The current code continues to be used if the 
value is not set (is zero).


 src/gen6_mfc_common.c | 24 +++-
 src/i965_encoder.c|  2 ++
 src/i965_encoder.h|  1 +
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/src/gen6_mfc_common.c b/src/gen6_mfc_common.c
index e8596ff..8907751 100644
--- a/src/gen6_mfc_common.c
+++ b/src/gen6_mfc_common.c
@@ -168,16 +168,22 @@ static void intel_mfc_brc_init(struct encode_state 
*encode_state,
 
 bpf = mfc_context->brc.bits_per_frame[i] = bitrate/framerate;
 
-if ((bpf > qp51_size) && (bpf < qp1_size)) {
-mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51 - 50*(bpf - 
qp51_size)/(qp1_size - qp51_size);
-}
-else if (bpf >= qp1_size)
-mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 1;
-else if (bpf <= qp51_size)
-mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51;
+if (encoder_context->brc.initial_qp) {
+mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I] = 
encoder_context->brc.initial_qp;
+mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 
encoder_context->brc.initial_qp;
+mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B] = 
encoder_context->brc.initial_qp;
+} else {
+if ((bpf > qp51_size) && (bpf < qp1_size)) {
+mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51 - 50*(bpf - 
qp51_size)/(qp1_size - qp51_size);
+}
+else if (bpf >= qp1_size)
+mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 1;
+else if (bpf <= qp51_size)
+mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P] = 51;
 
-mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I] = 
mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P];
-mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B] = 
mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I];
+mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I] = 
mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P];
+mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B] = 
mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I];
+}
 
 BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I], 
(int)encoder_context->brc.min_qp, 51);
 BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P], 
(int)encoder_context->brc.min_qp, 51);
diff --git a/src/i965_encoder.c b/src/i965_encoder.c
index 3056900..0a648d4 100644
--- a/src/i965_encoder.c
+++ b/src/i965_encoder.c
@@ -576,8 +576,10 @@ 
intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
 }
 
 if (encoder_context->brc.window_size != misc->window_size ||
+encoder_context->brc.initial_qp  != misc->initial_qp ||
 encoder_context->brc.min_qp  != misc->min_qp) {
 encoder_context->brc.window_size = misc->window_size;
+encoder_context->brc.initial_qp  = misc->initial_qp;
 encoder_context->brc.min_qp  = misc->min_qp;
 encoder_context->brc.need_reset = 1;
 }
diff --git a/src/i965_encoder.h b/src/i965_encoder.h
index 16a6f3f..829df9d 100644
--- a/src/i965_encoder.h
+++ b/src/i965_encoder.h
@@ -92,6 +92,7 @@ struct intel_encoder_context
 unsigned int hrd_buffer_size;
 unsigned int hrd_initial_buffer_fullness;
 unsigned int window_size;
+unsigned int initial_qp;
 unsigned int min_qp;
 unsigned int need_reset;
 
-- 
2.11.0
___
Libva mailing list
Libva@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libva

[Libva] [PATCH 1/2] H.264 encoder: respect min QP setting

[Mark Thompson]

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
This is helpful to avoid some extremes of the CBR rate control (the QP can 
easily get down to 1 on a static image, which then consumes the entire HRD 
buffer encoding the next non-static frame with far more detail that is wanted).

The ROI code is not affected by this, it can still go below the configured min 
QP - I think this behaviour is the right one, but if you don't think so I 
update that as well.


 src/gen6_mfc_common.c | 18 +-
 src/i965_encoder.c|  4 +++-
 src/i965_encoder.h|  1 +
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/src/gen6_mfc_common.c b/src/gen6_mfc_common.c
index 15c0637..e8596ff 100644
--- a/src/gen6_mfc_common.c
+++ b/src/gen6_mfc_common.c
@@ -179,9 +179,9 @@ static void intel_mfc_brc_init(struct encode_state 
*encode_state,
 mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I] = 
mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P];
 mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B] = 
mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I];
 
-BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I], 1, 51);
-BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P], 1, 51);
-BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B], 1, 51);
+BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_I], 
(int)encoder_context->brc.min_qp, 51);
+BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_P], 
(int)encoder_context->brc.min_qp, 51);
+BRC_CLIP(mfc_context->brc.qp_prime_y[i][SLICE_TYPE_B], 
(int)encoder_context->brc.min_qp, 51);
 }
 }
 
@@ -318,7 +318,7 @@ int intel_mfc_brc_postpack(struct encode_state 
*encode_state,
 qpn = (int)(qpn + delta_qp + 0.5);
 
 /* making sure that with QP predictions we did do not leave QPs range */
-BRC_CLIP(qpn, 1, 51);
+BRC_CLIP(qpn, (int)encoder_context->brc.min_qp, 51);
 
 if (sts == BRC_NO_HRD_VIOLATION) { // no HRD violation
 /* correcting QPs of slices of other types */
@@ -338,9 +338,9 @@ int intel_mfc_brc_postpack(struct encode_state 
*encode_state,
 if (abs(qpn - BRC_I_B_QP_DIFF - qpi) > 4)
 mfc_context->brc.qp_prime_y[next_frame_layer_id][SLICE_TYPE_I] 
+= (qpn - BRC_I_B_QP_DIFF - qpi) >> 2;
 }
-
BRC_CLIP(mfc_context->brc.qp_prime_y[next_frame_layer_id][SLICE_TYPE_I], 1, 51);
-
BRC_CLIP(mfc_context->brc.qp_prime_y[next_frame_layer_id][SLICE_TYPE_P], 1, 51);
-
BRC_CLIP(mfc_context->brc.qp_prime_y[next_frame_layer_id][SLICE_TYPE_B], 1, 51);
+
BRC_CLIP(mfc_context->brc.qp_prime_y[next_frame_layer_id][SLICE_TYPE_I], 
(int)encoder_context->brc.min_qp, 51);
+
BRC_CLIP(mfc_context->brc.qp_prime_y[next_frame_layer_id][SLICE_TYPE_P], 
(int)encoder_context->brc.min_qp, 51);
+
BRC_CLIP(mfc_context->brc.qp_prime_y[next_frame_layer_id][SLICE_TYPE_B], 
(int)encoder_context->brc.min_qp, 51);
 } else if (sts == BRC_UNDERFLOW) { // underflow
 if (qpn <= qp) qpn = qp + 1;
 if (qpn > 51) {
@@ -349,8 +349,8 @@ int intel_mfc_brc_postpack(struct encode_state 
*encode_state,
 }
 } else if (sts == BRC_OVERFLOW) {
 if (qpn >= qp) qpn = qp - 1;
-if (qpn < 1) { // < 0 (?) overflow with minQP
-qpn = 1;
+if (qpn < encoder_context->brc.min_qp) { // overflow with minQP
+qpn = encoder_context->brc.min_qp;
 sts = BRC_OVERFLOW_WITH_MIN_QP; // bit stuffing to be done
 }
 }
diff --git a/src/i965_encoder.c b/src/i965_encoder.c
index ae31f01..3056900 100644
--- a/src/i965_encoder.c
+++ b/src/i965_encoder.c
@@ -575,8 +575,10 @@ 
intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
 encoder_context->brc.need_reset = 1;
 }
 
-if (encoder_context->brc.window_size != misc->window_size) {
+if (encoder_context->brc.window_size != misc->window_size ||
+encoder_context->brc.min_qp  != misc->min_qp) {
 encoder_context->brc.window_size = misc->window_size;
+encoder_context->brc.min_qp  = misc->min_qp;
 encoder_context->brc.need_reset = 1;
 }
 
diff --git a/src/i965_encoder.h b/src/i965_encoder.h
index be19ce6..16a6f3f 100644
--- a/src/i965_encoder.h
+++ b/src/i965_encoder.h
@@ -92,6 +92,7 @@ struct intel_encoder_context
 unsigned int hrd_buffer_size;
 unsigned int hrd_initial_buffer_fullness;
 unsigned int window_size;
+unsigned int min_qp;
 unsigned int need_reset;
 
 unsigned int num_roi;
-- 
2.11.0

___
Libva mailing list
Libva@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libva

Re: [Libva] [Libva-intel-driver][PATCH] Needn't reset brc if the bitrate setting isn't changed in the Begin/Render/End sequence

[Mark Thompson]

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:

Sequence parameters with bps = 1M
-> IDR frame, at 1Mbps
-> some P frames, at 1Mbps
RC parameters with bps = 2M
-> some P frames, at 2Mbps
Sequence parameters with bps = 3M
-> IDR frame, at 3Mbps
-> some P frames, at 3Mbps

But with the current code the second sequence is generated at 2Mbps because the 
RC parameters stay forever and "win" in some sense when they disagree.

So, I think a nicer solution would be to add some way to determine which 
parameter set was provided most recently, so that we can always use the right 
one (with RC parameters overriding sequence parameters when both are provided). 
 See patch below for a possible approach (rather ugly and not very well tested).

Thanks,

- Mark


diff --git a/src/i965_drv_video.c b/src/i965_drv_video.c
index 15920f5..5d218d7 100644
--- a/src/i965_drv_video.c
+++ b/src/i965_drv_video.c
@@ -3197,6 +3197,9 @@ 
i965_encoder_render_misc_parameter_buffer(VADriverContextP ctx,
 if (param->type == VAEncMiscParameterTypeTemporalLayerStructure)
 encode->has_layers = 1;
 
+if (param->type == VAEncMiscParameterTypeRateControl)
+encode->frame_has_rc_parameters = 1;
+
 index = i965_encoder_get_misc_paramerter_buffer_index(ctx, encode, param);
 
 if (index >= ARRAY_ELEMS(encode->misc_param[0]))
@@ -3243,6 +3246,7 @@ i965_encoder_render_picture(VADriverContextP ctx,
 
 case VAEncSequenceParameterBufferType:
 vaStatus = I965_RENDER_ENCODE_BUFFER(sequence_parameter_ext);
+encode->frame_has_sequence_parameters = 1;
 break;
 
 case VAEncPictureParameterBufferType:
diff --git a/src/i965_drv_video.h b/src/i965_drv_video.h
index 7cba3a3..b326e50 100644
--- a/src/i965_drv_video.h
+++ b/src/i965_drv_video.h
@@ -272,6 +272,10 @@ struct encode_state
 
 int has_layers;
 
+/* Whether these parameter sets were supplied with the current frame. */
+int frame_has_rc_parameters;
+int frame_has_sequence_parameters;
+
 struct buffer_store *misc_param[16][8];
 
 VASurfaceID current_render_target;
diff --git a/src/i965_encoder.c b/src/i965_encoder.c
index d4d7445..0ae8965 100644
--- a/src/i965_encoder.c
+++ b/src/i965_encoder.c
@@ -361,16 +361,20 @@ 
intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx,
 
 if (num_pframes_in_gop != encoder_context->brc.num_pframes_in_gop ||
 num_bframes_in_gop != encoder_context->brc.num_bframes_in_gop ||
-bits_per_second != 
encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1] ||
 framerate.num != 
encoder_context->brc.framerate[encoder_context->layer.num_layers - 1].num ||
 framerate.den != 
encoder_context->brc.framerate[encoder_context->layer.num_layers - 1].den) {
 encoder_context->brc.num_pframes_in_gop = num_pframes_in_gop;
 encoder_context->brc.num_bframes_in_gop = num_bframes_in_gop;
-encoder_context->brc.bits_per_second[encoder_context->layer.num_layers 
- 1] = bits_per_second;
 encoder_context->brc.framerate[encoder_context->layer.num_layers - 1] 
= framerate;
 encoder_context->brc.need_reset = 1;
 }
 
+if (encode_state->frame_has_sequence_parameters && 
!encode_state->frame_has_rc_parameters &&
+bits_per_second != 
encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1]) {
+encoder_context->brc.bits_per_second[encoder_context->layer.num_layers 
- 1] = bits_per_second;
+encoder_context->brc.need_reset = 1;
+}
+
 if (!encoder_context->brc.hrd_buffer_size ||
 

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

[Mark Thompson]

Also adds support for fractional framerate.

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
 src/gen9_vp9_encoder.c | 232 +++--
 src/gen9_vp9_encoder.h |  12 +--
 src/i965_drv_video.c   |  10 +--
 src/i965_encoder.c |  36 
 src/i965_encoder.h |   1 +
 5 files changed, 69 insertions(+), 222 deletions(-)

diff --git a/src/gen9_vp9_encoder.c b/src/gen9_vp9_encoder.c
index 3ea1537..05d86da 100644
--- a/src/gen9_vp9_encoder.c
+++ b/src/gen9_vp9_encoder.c
@@ -1201,8 +1201,8 @@ void gen9_vp9_set_curbe_brc(VADriverContextP ctx,
   VP9_BRC_KBPS;
 cmd->dw9.min_bit_rate   = (vp9_state->min_bit_rate  + 
VP9_BRC_KBPS - 1) / VP9_BRC_KBPS *
   VP9_BRC_KBPS;
-cmd->dw10.frame_ratem   = vp9_state->frame_rate;
-cmd->dw11.frame_rated   = 1;
+cmd->dw10.frame_ratem   = vp9_state->framerate.num;
+cmd->dw11.frame_rated   = vp9_state->framerate.den;
 
 cmd->dw14.avbr_accuracy = 30;
 cmd->dw14.avbr_convergence  = 150;
@@ -1235,8 +1235,8 @@ void gen9_vp9_set_curbe_brc(VADriverContextP ctx,
 cmd->dw17.enable_dynamic_scaling = vp9_state->dys_in_use;
 cmd->dw17.brc_overshoot_cbr_pct = 150;
 
-dInputBitsPerFrame = (double)(cmd->dw8.max_bit_rate) / 
(vp9_state->frame_rate);
-dbps_ratio = dInputBitsPerFrame / 
((double)(vp9_state->vbv_buffer_size_in_bit) / 30);
+dInputBitsPerFrame = (double)cmd->dw8.max_bit_rate * 
(double)vp9_state->framerate.den / (double)vp9_state->framerate.num;
+dbps_ratio = dInputBitsPerFrame / 
((double)vp9_state->vbv_buffer_size_in_bit / 30.0);
 if (dbps_ratio < 0.1)
 dbps_ratio = 0.1;
 if (dbps_ratio > 3.5)
@@ -1423,7 +1423,6 @@ gen9_vp9_brc_init_reset_kernel(VADriverContextP ctx,
 brc_initreset_curbe.initbrc= !vp9_state->brc_inited;
 brc_initreset_curbe.mbbrc_enabled  = 0;
 brc_initreset_curbe.ref_frame_flag  = vp9_state->ref_frame_flag;
-brc_initreset_curbe.frame_rate   = vp9_state->frame_rate;
 
 vme_context->pfn_set_curbe_brc(ctx, encode_state,
gpe_context,
@@ -1523,7 +1522,6 @@ gen9_vp9_brc_intra_dist_kernel(VADriverContextP ctx,
 brc_intra_dist_curbe.initbrc= !vp9_state->brc_inited;
 brc_intra_dist_curbe.mbbrc_enabled  = 0;
 brc_intra_dist_curbe.ref_frame_flag  = vp9_state->ref_frame_flag;
-brc_intra_dist_curbe.frame_rate   = vp9_state->frame_rate;
 
 vme_context->pfn_set_curbe_brc(ctx, encode_state,
gpe_context,
@@ -3926,169 +3924,40 @@ gen9_encode_vp9_check_parameter(VADriverContextP ctx,
 return VA_STATUS_ERROR_UNIMPLEMENTED;
 
 if (vp9_state->brc_enabled) {
-if (vp9_state->brc_flag_check & VP9_BRC_FAILURE) {
-WARN_ONCE("Rate control misc_parameter is required for BRC\n");
-return VA_STATUS_ERROR_INVALID_PARAMETER;
-}
+if (vp9_state->first_frame || vp9_state->picture_coding_type == 
KEY_FRAME) {
+vp9_state->brc_reset = encoder_context->brc.need_reset || 
vp9_state->first_frame;
 
-if (vp9_state->first_frame) {
-unsigned int brc_flag;
-VAEncMiscParameterBuffer *misc_param;
-
-brc_flag = VP9_BRC_SEQ | VP9_BRC_RC;
-if ((vp9_state->brc_flag_check & brc_flag) != brc_flag) {
-WARN_ONCE("SPS/RC misc is required for BRC\n");
+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;
-}
 
-/* 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;
-   

[Libva] [PATCH 3/4] VP8 encoder: use generic rate control parameters

[Mark Thompson]

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
 src/gen6_mfc.h |  6 -
 src/gen8_mfc.c | 73 ++
 2 files changed, 17 insertions(+), 62 deletions(-)

diff --git a/src/gen6_mfc.h b/src/gen6_mfc.h
index 290922b..7a5d940 100644
--- a/src/gen6_mfc.h
+++ b/src/gen6_mfc.h
@@ -235,12 +235,6 @@ struct gen6_mfc_context
 double qpf_rounding_accumulator[MAX_TEMPORAL_LAYERS];
 int bits_prev_frame[MAX_TEMPORAL_LAYERS];
 int prev_slice_type[MAX_TEMPORAL_LAYERS];
-
-double saved_bps;
-double saved_fps;
-int saved_intra_period;
-int saved_ip_period;
-int saved_idr_period;
 } brc;
 
 struct {
diff --git a/src/gen8_mfc.c b/src/gen8_mfc.c
index d1de92c..90119d7 100644
--- a/src/gen8_mfc.c
+++ b/src/gen8_mfc.c
@@ -3324,12 +3324,8 @@ static void gen8_mfc_vp8_brc_init(struct encode_state 
*encode_state,
 {
 struct gen6_mfc_context *mfc_context = encoder_context->mfc_context;
 VAEncSequenceParameterBufferVP8 *seq_param = 
(VAEncSequenceParameterBufferVP8 *)encode_state->seq_param_ext->buffer;
-VAEncMiscParameterBuffer* misc_param_hrd = 
(VAEncMiscParameterBuffer*)encode_state->misc_param[VAEncMiscParameterTypeHRD][0]->buffer;
-VAEncMiscParameterHRD* param_hrd = 
(VAEncMiscParameterHRD*)misc_param_hrd->data;
-VAEncMiscParameterBuffer* misc_param_frame_rate_buffer = 
(VAEncMiscParameterBuffer*)encode_state->misc_param[VAEncMiscParameterTypeFrameRate][0]->buffer;
-VAEncMiscParameterFrameRate* param_frame_rate = 
(VAEncMiscParameterFrameRate*)misc_param_frame_rate_buffer->data;
-double bitrate = seq_param->bits_per_second;
-unsigned int frame_rate = param_frame_rate->framerate;
+double bitrate = encoder_context->brc.bits_per_second[0];
+double framerate = (double)encoder_context->brc.framerate[0].num / 
(double)encoder_context->brc.framerate[0].den;
 int inum = 1, pnum = 0;
 int intra_period = seq_param->intra_period;
 int width_in_mbs = ALIGN(seq_param->frame_width, 16) / 16;
@@ -3340,14 +3336,14 @@ static void gen8_mfc_vp8_brc_init(struct encode_state 
*encode_state,
 
 mfc_context->brc.mode = encoder_context->rate_control_mode;
 
-mfc_context->brc.target_frame_size[0][SLICE_TYPE_I] = 
(int)((double)((bitrate * intra_period)/frame_rate) /
+mfc_context->brc.target_frame_size[0][SLICE_TYPE_I] = 
(int)((double)((bitrate * intra_period) / framerate) /
  (double)(inum + 
BRC_PWEIGHT * pnum ));
 mfc_context->brc.target_frame_size[0][SLICE_TYPE_P] = BRC_PWEIGHT * 
mfc_context->brc.target_frame_size[0][SLICE_TYPE_I];
 
 mfc_context->brc.gop_nums[0][SLICE_TYPE_I] = inum;
 mfc_context->brc.gop_nums[0][SLICE_TYPE_P] = pnum;
 
-mfc_context->brc.bits_per_frame[0] = bitrate/frame_rate;
+mfc_context->brc.bits_per_frame[0] = bitrate / framerate;
 
 mfc_context->brc.qp_prime_y[0][SLICE_TYPE_I] = 
gen8_mfc_vp8_qindex_estimate(encode_state,

 mfc_context,
@@ -3358,12 +3354,17 @@ static void gen8_mfc_vp8_brc_init(struct encode_state 
*encode_state,

 mfc_context->brc.target_frame_size[0][SLICE_TYPE_P],

 0);
 
-mfc_context->hrd.buffer_size[0] = (double)param_hrd->buffer_size;
-mfc_context->hrd.current_buffer_fullness[0] =
-(double)(param_hrd->initial_buffer_fullness < 
mfc_context->hrd.buffer_size[0])?
-param_hrd->initial_buffer_fullness: mfc_context->hrd.buffer_size[0]/2.;
-mfc_context->hrd.target_buffer_fullness[0] = 
(double)mfc_context->hrd.buffer_size[0]/2.;
-mfc_context->hrd.buffer_capacity[0] = 
(double)mfc_context->hrd.buffer_size[0]/max_frame_size;
+if (encoder_context->brc.hrd_buffer_size)
+mfc_context->hrd.buffer_size[0] = 
(double)encoder_context->brc.hrd_buffer_size;
+else
+mfc_context->hrd.buffer_size[0] = bitrate;
+if (encoder_context->brc.hrd_initial_buffer_fullness &&
+encoder_context->brc.hrd_initial_buffer_fullness < 
mfc_context->hrd.buffer_size[0])
+mfc_context->hrd.current_buffer_fullness[0] = 
(double)encoder_context->brc.hrd_initial_buffer_fullness;
+else
+mfc_context->hrd.current_buffer_fullness[0] = 
mfc_context->hrd.buffer_size[0] / 2.0;
+mfc_context->hrd.target_buffer_fullness[0] = 
(double)mfc_context->hrd.buffer_size[0] / 2.0;
+mfc_context->hrd.buffer_capacity[0] = 
(double)mfc_context->hrd.buffer_size[0] / max_frame_size;
 mfc_context->hrd.violation_noted = 0;
 }
 
@@ -3485,9 +3486,8 @@ static void gen8_mfc_vp8_hrd_context_init(s

[Libva] [PATCH 2/4] HEVC encoder: use generic rate control parameters

[Mark Thompson]

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
 src/gen9_mfc.h  |  6 
 src/gen9_mfc_hevc.c | 83 ++---
 src/i965_encoder.c  | 74 +++
 3 files changed, 84 insertions(+), 79 deletions(-)

diff --git a/src/gen9_mfc.h b/src/gen9_mfc.h
index f7dc572..8e7d5ad 100644
--- a/src/gen9_mfc.h
+++ b/src/gen9_mfc.h
@@ -153,12 +153,6 @@ struct gen9_hcpe_context {
 int target_frame_size[3]; // I,P,B
 double bits_per_frame;
 double qpf_rounding_accumulator;
-
-double saved_bps;
-double saved_fps;
-int saved_intra_period;
-int saved_ip_period;
-int saved_idr_period;
 } brc;
 
 struct {
diff --git a/src/gen9_mfc_hevc.c b/src/gen9_mfc_hevc.c
index be5666c..8a84c1c 100644
--- a/src/gen9_mfc_hevc.c
+++ b/src/gen9_mfc_hevc.c
@@ -524,7 +524,7 @@ gen9_hcpe_hevc_pic_state(VADriverContextP ctx, struct 
encode_state *encode_state
 int ctb_size = 1 << log2_ctb_size;
 double rawctubits = 8 * 3 * ctb_size * ctb_size / 2.0;
 int maxctubits = (int)(5 * rawctubits / 3) ;
-double bitrate = seq_param->bits_per_second * 1.0;
+double bitrate = (double)encoder_context->brc.bits_per_second[0];
 double framebitrate = bitrate / 32 / 8; //32 byte unit
 int minframebitrate = 0;//(int) (framebitrate * 3 / 10);
 int maxframebitrate = (int)(framebitrate * 10 / 10);
@@ -2163,15 +2163,16 @@ VAStatus intel_hcpe_hevc_prepare(VADriverContextP ctx,
 
 static void
 intel_hcpe_bit_rate_control_context_init(struct encode_state *encode_state,
-struct gen9_hcpe_context *mfc_context)
+ struct intel_encoder_context 
*encoder_context)
 {
+struct gen9_hcpe_context *mfc_context = encoder_context->mfc_context;
 VAEncSequenceParameterBufferHEVC *pSequenceParameter = 
(VAEncSequenceParameterBufferHEVC *)encode_state->seq_param_ext->buffer;
 int ctb_size = 16;
 int width_in_mbs = (pSequenceParameter->pic_width_in_luma_samples + 
ctb_size - 1) / ctb_size;
 int height_in_mbs = (pSequenceParameter->pic_height_in_luma_samples + 
ctb_size - 1) / ctb_size;
 
-float fps =  pSequenceParameter->vui_time_scale / 
pSequenceParameter->vui_num_units_in_tick ;
-double bitrate = pSequenceParameter->bits_per_second * 1.0;
+double fps = (double)encoder_context->brc.framerate[0].num / 
(double)encoder_context->brc.framerate[0].den;
+double bitrate = encoder_context->brc.bits_per_second[0];
 int inter_mb_size = bitrate * 1.0 / (fps + 4.0) / width_in_mbs / 
height_in_mbs;
 int intra_mb_size = inter_mb_size * 5.0;
 int i;
@@ -2214,11 +2215,9 @@ static void intel_hcpe_brc_init(struct encode_state 
*encode_state,
 {
 struct gen9_hcpe_context *mfc_context = encoder_context->mfc_context;
 VAEncSequenceParameterBufferHEVC *pSequenceParameter = 
(VAEncSequenceParameterBufferHEVC *)encode_state->seq_param_ext->buffer;
-VAEncMiscParameterHRD* pParameterHRD = NULL;
-VAEncMiscParameterBuffer* pMiscParamHRD = NULL;
 
-double bitrate = pSequenceParameter->bits_per_second * 1.0;
-double framerate = (double)pSequenceParameter->vui_time_scale / 
(double)pSequenceParameter->vui_num_units_in_tick;
+double bitrate = (double)encoder_context->brc.bits_per_second[0];
+double framerate = (double)encoder_context->brc.framerate[0].num / 
(double)encoder_context->brc.framerate[0].den;
 int inum = 1, pnum = 0, bnum = 0; /* Gop structure: number of I, P, B 
frames in the Gop. */
 int intra_period = pSequenceParameter->intra_period;
 int ip_period = pSequenceParameter->ip_period;
@@ -2238,12 +2237,6 @@ static void intel_hcpe_brc_init(struct encode_state 
*encode_state,
 qp1_size = qp1_size * bpp;
 qp51_size = qp51_size * bpp;
 
-if (!encode_state->misc_param[VAEncMiscParameterTypeHRD][0] || 
!encode_state->misc_param[VAEncMiscParameterTypeHRD][0]->buffer)
-return;
-
-pMiscParamHRD = 
(VAEncMiscParameterBuffer*)encode_state->misc_param[VAEncMiscParameterTypeHRD][0]->buffer;
-pParameterHRD = (VAEncMiscParameterHRD*)pMiscParamHRD->data;
-
 if (pSequenceParameter->ip_period) {
 pnum = (intra_period + ip_period - 1) / ip_period - 1;
 bnum = intra_period - inum - pnum;
@@ -2262,7 +2255,7 @@ static void intel_hcpe_brc_init(struct encode_state 
*encode_state,
 
 bpf = mfc_context->brc.bits_per_frame = bitrate / framerate;
 
-if (!pParameterHRD || pParameterHRD->buffer_size <= 0)
+if (!encoder_context->brc.hrd_buffer_size)
 {
 mfc_context->hrd.buffer_size = bitrate * ratio;
 mfc_context->hrd.current_buffer_fullness =
@@ -2270,7 +2263,7 @@ static void intel_hcpe_brc_init(struct encode_state 
*encode_state,
 bitrate * ratio/2 : mfc_context->hrd.buffer_size / 2.

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

[Mark Thompson]

Update references in both H.264 encoders (gen6_mfc and gen9_vdenc).

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
Changes for the series:
* Derive numerator and denominator from the correct parts of the framerate 
parameter (see also earlier patch fixing the comment in va.h).
* Make VP8 HRD buffer size derivation the same as other codecs.
* Make the VP9 rate control setup behave more consistently (use window size if 
needed for VBR, always use HRD parameters if present).
* Fix warnings.

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

diff --git a/src/gen6_mfc_common.c b/src/gen6_mfc_common.c
index 68d030e..15c0637 100644
--- a/src/gen6_mfc_common.c
+++ b/src/gen6_mfc_common.c
@@ -119,16 +119,19 @@ static void intel_mfc_brc_init(struct encode_state 
*encode_state,
 
 if (i == 0) {
 bitrate = encoder_context->brc.bits_per_second[0];
-framerate = (double)encoder_context->brc.framerate_per_100s[0] / 
100.0;
+framerate = (double)encoder_context->brc.framerate[0].num / 
(double)encoder_context->brc.framerate[0].den;
 } else {
 bitrate = (encoder_context->brc.bits_per_second[i] - 
encoder_context->brc.bits_per_second[i - 1]);
-framerate = (double)(encoder_context->brc.framerate_per_100s[i] - 
encoder_context->brc.framerate_per_100s[i - 1]) / 100.0;
+framerate = ((double)encoder_context->brc.framerate[i].num / 
(double)encoder_context->brc.framerate[i].den) -
+((double)encoder_context->brc.framerate[i - 1].num / 
(double)encoder_context->brc.framerate[i - 1].den);
 }
 
 if (i == encoder_context->layer.num_layers - 1)
 factor = 1.0;
-else
-factor = (double)encoder_context->brc.framerate_per_100s[i] / 
encoder_context->brc.framerate_per_100s[encoder_context->layer.num_layers - 1];
+else {
+factor = ((double)encoder_context->brc.framerate[i].num / 
(double)encoder_context->brc.framerate[i].den) /
+((double)encoder_context->brc.framerate[i - 1].num / 
(double)encoder_context->brc.framerate[i - 1].den);
+}
 
 hrd_factor = (double)bitrate / 
encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1];
 
diff --git a/src/gen9_vdenc.c b/src/gen9_vdenc.c
index 8009b31..691faec 100644
--- a/src/gen9_vdenc.c
+++ b/src/gen9_vdenc.c
@@ -851,7 +851,7 @@ gen9_vdenc_update_misc_parameters(VADriverContextP ctx,
 if (vdenc_context->internal_rate_mode != I965_BRC_CQP &&
 encoder_context->brc.need_reset) {
 /* So far, vdenc doesn't support temporal layer */
-vdenc_context->frames_per_100s = 
encoder_context->brc.framerate_per_100s[0];
+vdenc_context->framerate = encoder_context->brc.framerate[0];
 
 vdenc_context->vbv_buffer_size_in_bit = 
encoder_context->brc.hrd_buffer_size;
 vdenc_context->init_vbv_buffer_fullness_in_bit = 
encoder_context->brc.hrd_initial_buffer_fullness;
@@ -927,7 +927,8 @@ gen9_vdenc_update_parameters(VADriverContextP ctx,
  !vdenc_context->vbv_buffer_size_in_bit ||
  !vdenc_context->max_bit_rate ||
  !vdenc_context->target_bit_rate ||
- !vdenc_context->frames_per_100s))
+ !vdenc_context->framerate.num ||
+ !vdenc_context->framerate.den))
 vdenc_context->brc_enabled = 0;
 
 if (!vdenc_context->brc_enabled) {
@@ -1565,7 +1566,8 @@ gen9_vdenc_get_profile_level_max_frame(VADriverContextP 
ctx,
 tmpf = max_mbps / 172.0;
 
 max_byte_per_frame0 = (uint64_t)(tmpf * bits_per_mb);
-max_byte_per_frame1 = (uint64_t)(((double)max_mbps * 100) / 
vdenc_context->frames_per_100s *bits_per_mb);
+max_byte_per_frame1 = (uint64_t)(((double)max_mbps * 
vdenc_context->framerate.den) /
+ (double)vdenc_context->framerate.num * 
bits_per_mb);
 
 /* TODO: check VAEncMiscParameterTypeMaxFrameSize */
 ret = (unsigned int)MIN(max_byte_per_frame0, max_byte_per_frame1);
@@ -1586,12 +1588,12 @@ gen9_vdenc_calculate_initial_qp(VADriverContextP ctx,
 
 frame_size = (vdenc_context->frame_width * vdenc_context->frame_height * 3 
/ 2);
 qp = (int)(1.0 / 1.2 * pow(10.0,
-   (log10(frame_size * 2.0 / 3.0 * 
((float)vdenc_context->frames_per_100s) /
-  ((float)(vdenc_context->target_bit_rate 
* 1000) * 100)) - x0) *
+   (log10(frame_size * 2.0 / 3.0 * 
vdenc_context->framerate.num /
+  ((double)vden

[Libva] [PATCH] va.h: Improve the comment on the encode framerate parameter

[Mark Thompson]

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
On 22/12/16 01:32, Xiang, Haihao wrote:
> 
>> 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.
> 

Enclosing patch fixes the incorrect comment, and also adds a bit more 
explanation to hopefully make the use clearer.

Thanks,

- Mark


 va/va.h | 20 +++-
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/va/va.h b/va/va.h
index 8791906..a1ab4a8 100644
--- a/va/va.h
+++ b/va/va.h
@@ -1277,12 +1277,22 @@ typedef struct _VAEncMiscParameterRateControl
 typedef struct _VAEncMiscParameterFrameRate
 {
 /*
- * 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
+ * The framerate is specified as a number of frames per second, as a
+ * fraction.  The denominator of the fraction is given in the top half
+ * (the high two bytes) of the framerate field, and the numerator is
+ * given in the bottom half (the low two bytes).
  *
- * If the high 2 btyes is 0, the frame rate is specified by the low 2 
bytes.
+ * That is:
+ * denominator = framerate >> 16 & 0x;
+ * numerator   = framerate & 0x;
+ * fps = numerator / denominator;
+ *
+ * For example, if framerate is set to (100 << 16 | 750), this is
+ * 750 / 100, hence 7.5fps.
+ *
+ * If the denominator is zero (the high two bytes are both zero) then
+ * it takes the value one instead, so the framerate is just the integer
+ * in the low 2 bytes.
  */
 unsigned int framerate;
 union
-- 
2.11.0
___
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

[Mark Thompson]

On 21/12/16 04:46, Xiang, Haihao wrote:
> On Mon, 2016-12-19 at 23:01 +0000, Mark Thompson wrote:
>> Also adds support for fractional framerate.
>>
>> Signed-off-by: Mark Thompson <s...@jkqxz.net>
>> ---
>>  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;
>> -  

[Libva] [PATCH 3/4] VP8 encoder: use generic rate control parameters

[Mark Thompson]

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
 src/gen6_mfc.h |  6 --
 src/gen8_mfc.c | 68 +-
 2 files changed, 15 insertions(+), 59 deletions(-)

diff --git a/src/gen6_mfc.h b/src/gen6_mfc.h
index 290922b..7a5d940 100644
--- a/src/gen6_mfc.h
+++ b/src/gen6_mfc.h
@@ -235,12 +235,6 @@ struct gen6_mfc_context
 double qpf_rounding_accumulator[MAX_TEMPORAL_LAYERS];
 int bits_prev_frame[MAX_TEMPORAL_LAYERS];
 int prev_slice_type[MAX_TEMPORAL_LAYERS];
-
-double saved_bps;
-double saved_fps;
-int saved_intra_period;
-int saved_ip_period;
-int saved_idr_period;
 } brc;
 
 struct {
diff --git a/src/gen8_mfc.c b/src/gen8_mfc.c
index d1de92c..7811cd0 100644
--- a/src/gen8_mfc.c
+++ b/src/gen8_mfc.c
@@ -3324,12 +3324,8 @@ static void gen8_mfc_vp8_brc_init(struct encode_state 
*encode_state,
 {
 struct gen6_mfc_context *mfc_context = encoder_context->mfc_context;
 VAEncSequenceParameterBufferVP8 *seq_param = 
(VAEncSequenceParameterBufferVP8 *)encode_state->seq_param_ext->buffer;
-VAEncMiscParameterBuffer* misc_param_hrd = 
(VAEncMiscParameterBuffer*)encode_state->misc_param[VAEncMiscParameterTypeHRD][0]->buffer;
-VAEncMiscParameterHRD* param_hrd = 
(VAEncMiscParameterHRD*)misc_param_hrd->data;
-VAEncMiscParameterBuffer* misc_param_frame_rate_buffer = 
(VAEncMiscParameterBuffer*)encode_state->misc_param[VAEncMiscParameterTypeFrameRate][0]->buffer;
-VAEncMiscParameterFrameRate* param_frame_rate = 
(VAEncMiscParameterFrameRate*)misc_param_frame_rate_buffer->data;
-double bitrate = seq_param->bits_per_second;
-unsigned int frame_rate = param_frame_rate->framerate;
+double bitrate = encoder_context->brc.bits_per_second[0];
+double framerate = (double)encoder_context->brc.framerate[0].num / 
(double)encoder_context->brc.framerate[0].den;
 int inum = 1, pnum = 0;
 int intra_period = seq_param->intra_period;
 int width_in_mbs = ALIGN(seq_param->frame_width, 16) / 16;
@@ -3340,14 +3336,14 @@ static void gen8_mfc_vp8_brc_init(struct encode_state 
*encode_state,
 
 mfc_context->brc.mode = encoder_context->rate_control_mode;
 
-mfc_context->brc.target_frame_size[0][SLICE_TYPE_I] = 
(int)((double)((bitrate * intra_period)/frame_rate) /
+mfc_context->brc.target_frame_size[0][SLICE_TYPE_I] = 
(int)((double)((bitrate * intra_period) / framerate) /
  (double)(inum + 
BRC_PWEIGHT * pnum ));
 mfc_context->brc.target_frame_size[0][SLICE_TYPE_P] = BRC_PWEIGHT * 
mfc_context->brc.target_frame_size[0][SLICE_TYPE_I];
 
 mfc_context->brc.gop_nums[0][SLICE_TYPE_I] = inum;
 mfc_context->brc.gop_nums[0][SLICE_TYPE_P] = pnum;
 
-mfc_context->brc.bits_per_frame[0] = bitrate/frame_rate;
+mfc_context->brc.bits_per_frame[0] = bitrate / framerate;
 
 mfc_context->brc.qp_prime_y[0][SLICE_TYPE_I] = 
gen8_mfc_vp8_qindex_estimate(encode_state,

 mfc_context,
@@ -3358,10 +3354,15 @@ static void gen8_mfc_vp8_brc_init(struct encode_state 
*encode_state,

 mfc_context->brc.target_frame_size[0][SLICE_TYPE_P],

 0);
 
-mfc_context->hrd.buffer_size[0] = (double)param_hrd->buffer_size;
-mfc_context->hrd.current_buffer_fullness[0] =
-(double)(param_hrd->initial_buffer_fullness < 
mfc_context->hrd.buffer_size[0])?
-param_hrd->initial_buffer_fullness: mfc_context->hrd.buffer_size[0]/2.;
+if (encoder_context->brc.hrd_buffer_size)
+mfc_context->hrd.buffer_size[0] = 
(double)encoder_context->brc.hrd_buffer_size;
+else
+mfc_context->hrd.buffer_size[0] = 2.0 * bitrate;
+if (encoder_context->brc.hrd_initial_buffer_fullness &&
+encoder_context->brc.hrd_initial_buffer_fullness < 
mfc_context->hrd.buffer_size[0])
+mfc_context->hrd.current_buffer_fullness[0] = 
(double)encoder_context->brc.hrd_initial_buffer_fullness;
+else
+mfc_context->hrd.current_buffer_fullness[0] = 
mfc_context->hrd.buffer_size[0] / 2.0;
 mfc_context->hrd.target_buffer_fullness[0] = 
(double)mfc_context->hrd.buffer_size[0]/2.;
 mfc_context->hrd.buffer_capacity[0] = 
(double)mfc_context->hrd.buffer_size[0]/max_frame_size;
 mfc_context->hrd.violation_noted = 0;
@@ -3487,7 +3488,7 @@ static void gen8_mfc_vp8_hrd_context_init(struct 
encode_state *encode_state,
 struct gen6_mfc_context *mfc_context = encoder_context->mfc_context;
 VAEncSequenceParameterBufferVP8 *seq_param = 
(VAEncSequenceParameterBufferVP8 

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

[Mark Thompson]

Also adds support for fractional framerate.

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
 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(-)

diff --git a/src/gen9_vp9_encoder.c b/src/gen9_vp9_encoder.c
index 3ea1537..2e884e2 100644
--- a/src/gen9_vp9_encoder.c
+++ b/src/gen9_vp9_encoder.c
@@ -1201,8 +1201,8 @@ void gen9_vp9_set_curbe_brc(VADriverContextP ctx,
   VP9_BRC_KBPS;
 cmd->dw9.min_bit_rate   = (vp9_state->min_bit_rate  + 
VP9_BRC_KBPS - 1) / VP9_BRC_KBPS *
   VP9_BRC_KBPS;
-cmd->dw10.frame_ratem   = vp9_state->frame_rate;
-cmd->dw11.frame_rated   = 1;
+cmd->dw10.frame_ratem   = vp9_state->framerate.num;
+cmd->dw11.frame_rated   = vp9_state->framerate.den;
 
 cmd->dw14.avbr_accuracy = 30;
 cmd->dw14.avbr_convergence  = 150;
@@ -1235,8 +1235,8 @@ void gen9_vp9_set_curbe_brc(VADriverContextP ctx,
 cmd->dw17.enable_dynamic_scaling = vp9_state->dys_in_use;
 cmd->dw17.brc_overshoot_cbr_pct = 150;
 
-dInputBitsPerFrame = (double)(cmd->dw8.max_bit_rate) / 
(vp9_state->frame_rate);
-dbps_ratio = dInputBitsPerFrame / 
((double)(vp9_state->vbv_buffer_size_in_bit) / 30);
+dInputBitsPerFrame = (double)cmd->dw8.max_bit_rate * 
(double)vp9_state->framerate.den / (double)vp9_state->framerate.num;
+dbps_ratio = dInputBitsPerFrame / 
(double)vp9_state->vbv_buffer_size_in_bit / 30.0;
 if (dbps_ratio < 0.1)
 dbps_ratio = 0.1;
 if (dbps_ratio > 3.5)
@@ -1423,7 +1423,6 @@ gen9_vp9_brc_init_reset_kernel(VADriverContextP ctx,
 brc_initreset_curbe.initbrc= !vp9_state->brc_inited;
 brc_initreset_curbe.mbbrc_enabled  = 0;
 brc_initreset_curbe.ref_frame_flag  = vp9_state->ref_frame_flag;
-brc_initreset_curbe.frame_rate   = vp9_state->frame_rate;
 
 vme_context->pfn_set_curbe_brc(ctx, encode_state,
gpe_context,
@@ -1523,7 +1522,6 @@ gen9_vp9_brc_intra_dist_kernel(VADriverContextP ctx,
 brc_intra_dist_curbe.initbrc= !vp9_state->brc_inited;
 brc_intra_dist_curbe.mbbrc_enabled  = 0;
 brc_intra_dist_curbe.ref_frame_flag  = vp9_state->ref_frame_flag;
-brc_intra_dist_curbe.frame_rate   = vp9_state->frame_rate;
 
 vme_context->pfn_set_curbe_brc(ctx, encode_state,
gpe_context,
@@ -3926,168 +3924,47 @@ gen9_encode_vp9_check_parameter(VADriverContextP ctx,
 return VA_STATUS_ERROR_UNIMPLEMENTED;
 
 if (vp9_state->brc_enabled) {
-if (vp9_state->brc_flag_check & VP9_BRC_FAILURE) {
-WARN_ONCE("Rate control misc_parameter is required for BRC\n");
-return VA_STATUS_ERROR_INVALID_PARAMETER;
-}
-
-if (vp9_state->first_frame) {
-unsigned int brc_flag;
-VAEncMiscParameterBuffer *misc_param;
-
-brc_flag = VP9_BRC_SEQ | VP9_BRC_RC;
-if ((vp9_state->brc_flag_check & brc_flag) != brc_flag) {
-WARN_ONCE("SPS/RC misc is required for BRC\n");
-return VA_STATUS_ERROR_INVALID_PARAMETER;
-}
+if (vp9_state->first_frame || vp9_state->picture_coding_type == 
KEY_FRAME) {
+vp9_state->brc_reset = encoder_context->brc.need_reset || 
vp9_state->first_frame;
 
 /* 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;
-
-   

[Libva] [PATCH 2/4] HEVC encoder: use generic rate control parameters

[Mark Thompson]

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
 src/gen9_mfc.h  |  6 
 src/gen9_mfc_hevc.c | 83 ++---
 src/i965_encoder.c  | 74 +++
 3 files changed, 84 insertions(+), 79 deletions(-)

diff --git a/src/gen9_mfc.h b/src/gen9_mfc.h
index f7dc572..8e7d5ad 100644
--- a/src/gen9_mfc.h
+++ b/src/gen9_mfc.h
@@ -153,12 +153,6 @@ struct gen9_hcpe_context {
 int target_frame_size[3]; // I,P,B
 double bits_per_frame;
 double qpf_rounding_accumulator;
-
-double saved_bps;
-double saved_fps;
-int saved_intra_period;
-int saved_ip_period;
-int saved_idr_period;
 } brc;
 
 struct {
diff --git a/src/gen9_mfc_hevc.c b/src/gen9_mfc_hevc.c
index be5666c..8a84c1c 100644
--- a/src/gen9_mfc_hevc.c
+++ b/src/gen9_mfc_hevc.c
@@ -524,7 +524,7 @@ gen9_hcpe_hevc_pic_state(VADriverContextP ctx, struct 
encode_state *encode_state
 int ctb_size = 1 << log2_ctb_size;
 double rawctubits = 8 * 3 * ctb_size * ctb_size / 2.0;
 int maxctubits = (int)(5 * rawctubits / 3) ;
-double bitrate = seq_param->bits_per_second * 1.0;
+double bitrate = (double)encoder_context->brc.bits_per_second[0];
 double framebitrate = bitrate / 32 / 8; //32 byte unit
 int minframebitrate = 0;//(int) (framebitrate * 3 / 10);
 int maxframebitrate = (int)(framebitrate * 10 / 10);
@@ -2163,15 +2163,16 @@ VAStatus intel_hcpe_hevc_prepare(VADriverContextP ctx,
 
 static void
 intel_hcpe_bit_rate_control_context_init(struct encode_state *encode_state,
-struct gen9_hcpe_context *mfc_context)
+ struct intel_encoder_context 
*encoder_context)
 {
+struct gen9_hcpe_context *mfc_context = encoder_context->mfc_context;
 VAEncSequenceParameterBufferHEVC *pSequenceParameter = 
(VAEncSequenceParameterBufferHEVC *)encode_state->seq_param_ext->buffer;
 int ctb_size = 16;
 int width_in_mbs = (pSequenceParameter->pic_width_in_luma_samples + 
ctb_size - 1) / ctb_size;
 int height_in_mbs = (pSequenceParameter->pic_height_in_luma_samples + 
ctb_size - 1) / ctb_size;
 
-float fps =  pSequenceParameter->vui_time_scale / 
pSequenceParameter->vui_num_units_in_tick ;
-double bitrate = pSequenceParameter->bits_per_second * 1.0;
+double fps = (double)encoder_context->brc.framerate[0].num / 
(double)encoder_context->brc.framerate[0].den;
+double bitrate = encoder_context->brc.bits_per_second[0];
 int inter_mb_size = bitrate * 1.0 / (fps + 4.0) / width_in_mbs / 
height_in_mbs;
 int intra_mb_size = inter_mb_size * 5.0;
 int i;
@@ -2214,11 +2215,9 @@ static void intel_hcpe_brc_init(struct encode_state 
*encode_state,
 {
 struct gen9_hcpe_context *mfc_context = encoder_context->mfc_context;
 VAEncSequenceParameterBufferHEVC *pSequenceParameter = 
(VAEncSequenceParameterBufferHEVC *)encode_state->seq_param_ext->buffer;
-VAEncMiscParameterHRD* pParameterHRD = NULL;
-VAEncMiscParameterBuffer* pMiscParamHRD = NULL;
 
-double bitrate = pSequenceParameter->bits_per_second * 1.0;
-double framerate = (double)pSequenceParameter->vui_time_scale / 
(double)pSequenceParameter->vui_num_units_in_tick;
+double bitrate = (double)encoder_context->brc.bits_per_second[0];
+double framerate = (double)encoder_context->brc.framerate[0].num / 
(double)encoder_context->brc.framerate[0].den;
 int inum = 1, pnum = 0, bnum = 0; /* Gop structure: number of I, P, B 
frames in the Gop. */
 int intra_period = pSequenceParameter->intra_period;
 int ip_period = pSequenceParameter->ip_period;
@@ -2238,12 +2237,6 @@ static void intel_hcpe_brc_init(struct encode_state 
*encode_state,
 qp1_size = qp1_size * bpp;
 qp51_size = qp51_size * bpp;
 
-if (!encode_state->misc_param[VAEncMiscParameterTypeHRD][0] || 
!encode_state->misc_param[VAEncMiscParameterTypeHRD][0]->buffer)
-return;
-
-pMiscParamHRD = 
(VAEncMiscParameterBuffer*)encode_state->misc_param[VAEncMiscParameterTypeHRD][0]->buffer;
-pParameterHRD = (VAEncMiscParameterHRD*)pMiscParamHRD->data;
-
 if (pSequenceParameter->ip_period) {
 pnum = (intra_period + ip_period - 1) / ip_period - 1;
 bnum = intra_period - inum - pnum;
@@ -2262,7 +2255,7 @@ static void intel_hcpe_brc_init(struct encode_state 
*encode_state,
 
 bpf = mfc_context->brc.bits_per_frame = bitrate / framerate;
 
-if (!pParameterHRD || pParameterHRD->buffer_size <= 0)
+if (!encoder_context->brc.hrd_buffer_size)
 {
 mfc_context->hrd.buffer_size = bitrate * ratio;
 mfc_context->hrd.current_buffer_fullness =
@@ -2270,7 +2263,7 @@ static void intel_hcpe_brc_init(struct encode_state 
*encode_state,
 bitrate * ratio/2 : mfc_context->hrd.buffer_size / 2.

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

[Mark Thompson]

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

diff --git a/src/gen6_mfc_common.c b/src/gen6_mfc_common.c
index 68d030e..15c0637 100644
--- a/src/gen6_mfc_common.c
+++ b/src/gen6_mfc_common.c
@@ -119,16 +119,19 @@ static void intel_mfc_brc_init(struct encode_state 
*encode_state,
 
 if (i == 0) {
 bitrate = encoder_context->brc.bits_per_second[0];
-framerate = (double)encoder_context->brc.framerate_per_100s[0] / 
100.0;
+framerate = (double)encoder_context->brc.framerate[0].num / 
(double)encoder_context->brc.framerate[0].den;
 } else {
 bitrate = (encoder_context->brc.bits_per_second[i] - 
encoder_context->brc.bits_per_second[i - 1]);
-framerate = (double)(encoder_context->brc.framerate_per_100s[i] - 
encoder_context->brc.framerate_per_100s[i - 1]) / 100.0;
+framerate = ((double)encoder_context->brc.framerate[i].num / 
(double)encoder_context->brc.framerate[i].den) -
+((double)encoder_context->brc.framerate[i - 1].num / 
(double)encoder_context->brc.framerate[i - 1].den);
 }
 
 if (i == encoder_context->layer.num_layers - 1)
 factor = 1.0;
-else
-factor = (double)encoder_context->brc.framerate_per_100s[i] / 
encoder_context->brc.framerate_per_100s[encoder_context->layer.num_layers - 1];
+else {
+factor = ((double)encoder_context->brc.framerate[i].num / 
(double)encoder_context->brc.framerate[i].den) /
+((double)encoder_context->brc.framerate[i - 1].num / 
(double)encoder_context->brc.framerate[i - 1].den);
+}
 
 hrd_factor = (double)bitrate / 
encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1];
 
diff --git a/src/gen9_vdenc.c b/src/gen9_vdenc.c
index 8009b31..691faec 100644
--- a/src/gen9_vdenc.c
+++ b/src/gen9_vdenc.c
@@ -851,7 +851,7 @@ gen9_vdenc_update_misc_parameters(VADriverContextP ctx,
 if (vdenc_context->internal_rate_mode != I965_BRC_CQP &&
 encoder_context->brc.need_reset) {
 /* So far, vdenc doesn't support temporal layer */
-vdenc_context->frames_per_100s = 
encoder_context->brc.framerate_per_100s[0];
+vdenc_context->framerate = encoder_context->brc.framerate[0];
 
 vdenc_context->vbv_buffer_size_in_bit = 
encoder_context->brc.hrd_buffer_size;
 vdenc_context->init_vbv_buffer_fullness_in_bit = 
encoder_context->brc.hrd_initial_buffer_fullness;
@@ -927,7 +927,8 @@ gen9_vdenc_update_parameters(VADriverContextP ctx,
  !vdenc_context->vbv_buffer_size_in_bit ||
  !vdenc_context->max_bit_rate ||
  !vdenc_context->target_bit_rate ||
- !vdenc_context->frames_per_100s))
+ !vdenc_context->framerate.num ||
+ !vdenc_context->framerate.den))
 vdenc_context->brc_enabled = 0;
 
 if (!vdenc_context->brc_enabled) {
@@ -1565,7 +1566,8 @@ gen9_vdenc_get_profile_level_max_frame(VADriverContextP 
ctx,
 tmpf = max_mbps / 172.0;
 
 max_byte_per_frame0 = (uint64_t)(tmpf * bits_per_mb);
-max_byte_per_frame1 = (uint64_t)(((double)max_mbps * 100) / 
vdenc_context->frames_per_100s *bits_per_mb);
+max_byte_per_frame1 = (uint64_t)(((double)max_mbps * 
vdenc_context->framerate.den) /
+ (double)vdenc_context->framerate.num * 
bits_per_mb);
 
 /* TODO: check VAEncMiscParameterTypeMaxFrameSize */
 ret = (unsigned int)MIN(max_byte_per_frame0, max_byte_per_frame1);
@@ -1586,12 +1588,12 @@ gen9_vdenc_calculate_initial_qp(VADriverContextP ctx,
 
 frame_size = (vdenc_context->frame_width * vdenc_context->frame_height * 3 
/ 2);
 qp = (int)(1.0 / 1.2 * pow(10.0,
-  

Re: [Libva] [PATCH 2/3] H.265 main 10 encoder supports only 10bpp render targets

[Mark Thompson]

On 05/12/16 17:54, Mark Thompson wrote:
> Signed-off-by: Mark Thompson <s...@jkqxz.net>
> ---
> The supported surface formats are already correct (i.e. only P010); this 
> makes the render target format right as well (before this, it declares that 
> it supports only YUV 4:2:0 8-bit).
> 
>  src/i965_drv_video.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/i965_drv_video.c b/src/i965_drv_video.c
> index d83427c..9e1c92a 100644
> --- a/src/i965_drv_video.c
> +++ b/src/i965_drv_video.c
> @@ -880,6 +880,8 @@ i965_get_default_chroma_formats(VADriverContextP ctx, 
> VAProfile profile,
>  break;
>  
>  case VAProfileHEVCMain10:
> +if (HAS_HEVC10_ENCODING(i965) && entrypoint == VAEntrypointEncSlice)
> +chroma_formats = VA_RT_FORMAT_YUV420_10BPP;
>  if (HAS_HEVC10_DECODING(i965) && entrypoint == VAEntrypointVLD)
>  chroma_formats |= i965->codec_info->hevc_dec_chroma_formats;
>  break;
> 

Ping.  (The other patches in this series have been considered separately.)

Thanks,

- Mark

___
Libva mailing list
Libva@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libva

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

[Mark Thompson]

On 15/12/16 08:54, Xiang, Haihao wrote:
> 
> Hi Mark,
> 
> Thanks for your patch, is there any benefit to use a fraction? using
> 100 as framerate_den works well in the driver.

Everything the driver interacts with directly, on both sides, uses a fraction:

* In VAAPI, VAEncMiscParameterFrameRate contains a 16 / 16 fraction.

* Newer hardware uses a fraction directly where it requires a framerate 
(gen9_vp9_encoder.c:1204, gen9_vdenc.c:1648).

* H.264 and H.265 streams contain num_units_in_tick / time_scale.

* gstreamer (GstVideoInfo.fps_[nd]), libyami 
(VideoFrameRate.frameRate{Num,Denom}) and libavcodec 
(AVCodecContext.{time_base,framerate} are fraction structures) all represent 
framerate as a fraction.

The driver should use a fraction as well to preserve the exact value and be 
consistent with everything it interacts with.

Do you have any other concerns about the patch?  If there are additional 
references outside the core driver code which I have missed somehow then I 
would be happy to update them as well.

Thanks,

- Mark

___
Libva mailing list
Libva@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libva

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

[Mark Thompson]

Also adds support for fractional framerate.

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
 src/gen9_vp9_encoder.c | 240 +
 src/gen9_vp9_encoder.h |  11 +--
 src/i965_drv_video.c   |  10 +--
 src/i965_encoder.c |  36 
 src/i965_encoder.h |   1 +
 5 files changed, 80 insertions(+), 218 deletions(-)

diff --git a/src/gen9_vp9_encoder.c b/src/gen9_vp9_encoder.c
index 3ea1537..533bf81 100644
--- a/src/gen9_vp9_encoder.c
+++ b/src/gen9_vp9_encoder.c
@@ -1201,8 +1201,8 @@ void gen9_vp9_set_curbe_brc(VADriverContextP ctx,
   VP9_BRC_KBPS;
 cmd->dw9.min_bit_rate   = (vp9_state->min_bit_rate  + 
VP9_BRC_KBPS - 1) / VP9_BRC_KBPS *
   VP9_BRC_KBPS;
-cmd->dw10.frame_ratem   = vp9_state->frame_rate;
-cmd->dw11.frame_rated   = 1;
+cmd->dw10.frame_ratem   = vp9_state->framerate_num;
+cmd->dw11.frame_rated   = vp9_state->framerate_den;
 
 cmd->dw14.avbr_accuracy = 30;
 cmd->dw14.avbr_convergence  = 150;
@@ -1235,8 +1235,8 @@ void gen9_vp9_set_curbe_brc(VADriverContextP ctx,
 cmd->dw17.enable_dynamic_scaling = vp9_state->dys_in_use;
 cmd->dw17.brc_overshoot_cbr_pct = 150;
 
-dInputBitsPerFrame = (double)(cmd->dw8.max_bit_rate) / 
(vp9_state->frame_rate);
-dbps_ratio = dInputBitsPerFrame / 
((double)(vp9_state->vbv_buffer_size_in_bit) / 30);
+dInputBitsPerFrame = (double)cmd->dw8.max_bit_rate * 
(double)vp9_state->framerate_den / (double)vp9_state->framerate_num;
+dbps_ratio = dInputBitsPerFrame / 
(double)vp9_state->vbv_buffer_size_in_bit / 30.0;
 if (dbps_ratio < 0.1)
 dbps_ratio = 0.1;
 if (dbps_ratio > 3.5)
@@ -1423,7 +1423,6 @@ gen9_vp9_brc_init_reset_kernel(VADriverContextP ctx,
 brc_initreset_curbe.initbrc= !vp9_state->brc_inited;
 brc_initreset_curbe.mbbrc_enabled  = 0;
 brc_initreset_curbe.ref_frame_flag  = vp9_state->ref_frame_flag;
-brc_initreset_curbe.frame_rate   = vp9_state->frame_rate;
 
 vme_context->pfn_set_curbe_brc(ctx, encode_state,
gpe_context,
@@ -1523,7 +1522,6 @@ gen9_vp9_brc_intra_dist_kernel(VADriverContextP ctx,
 brc_intra_dist_curbe.initbrc= !vp9_state->brc_inited;
 brc_intra_dist_curbe.mbbrc_enabled  = 0;
 brc_intra_dist_curbe.ref_frame_flag  = vp9_state->ref_frame_flag;
-brc_intra_dist_curbe.frame_rate   = vp9_state->frame_rate;
 
 vme_context->pfn_set_curbe_brc(ctx, encode_state,
gpe_context,
@@ -3926,168 +3924,51 @@ gen9_encode_vp9_check_parameter(VADriverContextP ctx,
 return VA_STATUS_ERROR_UNIMPLEMENTED;
 
 if (vp9_state->brc_enabled) {
-if (vp9_state->brc_flag_check & VP9_BRC_FAILURE) {
-WARN_ONCE("Rate control misc_parameter is required for BRC\n");
-return VA_STATUS_ERROR_INVALID_PARAMETER;
-}
-
-if (vp9_state->first_frame) {
-unsigned int brc_flag;
-VAEncMiscParameterBuffer *misc_param;
-
-brc_flag = VP9_BRC_SEQ | VP9_BRC_RC;
-if ((vp9_state->brc_flag_check & brc_flag) != brc_flag) {
-WARN_ONCE("SPS/RC misc is required for BRC\n");
-return VA_STATUS_ERROR_INVALID_PARAMETER;
-}
+if (vp9_state->first_frame || vp9_state->picture_coding_type == 
KEY_FRAME) {
+vp9_state->brc_reset = encoder_context->brc.need_reset || 
vp9_state->first_frame;
 
 /* 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;
-
-   

[Libva] [PATCH 3/4] VP8 encoder: use generic rate control parameters

[Mark Thompson]

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
 src/gen6_mfc.h |  6 --
 src/gen8_mfc.c | 64 --
 2 files changed, 13 insertions(+), 57 deletions(-)

diff --git a/src/gen6_mfc.h b/src/gen6_mfc.h
index 290922b..7a5d940 100644
--- a/src/gen6_mfc.h
+++ b/src/gen6_mfc.h
@@ -235,12 +235,6 @@ struct gen6_mfc_context
 double qpf_rounding_accumulator[MAX_TEMPORAL_LAYERS];
 int bits_prev_frame[MAX_TEMPORAL_LAYERS];
 int prev_slice_type[MAX_TEMPORAL_LAYERS];
-
-double saved_bps;
-double saved_fps;
-int saved_intra_period;
-int saved_ip_period;
-int saved_idr_period;
 } brc;
 
 struct {
diff --git a/src/gen8_mfc.c b/src/gen8_mfc.c
index d1de92c..b0e7b08 100644
--- a/src/gen8_mfc.c
+++ b/src/gen8_mfc.c
@@ -3324,12 +3324,8 @@ static void gen8_mfc_vp8_brc_init(struct encode_state 
*encode_state,
 {
 struct gen6_mfc_context *mfc_context = encoder_context->mfc_context;
 VAEncSequenceParameterBufferVP8 *seq_param = 
(VAEncSequenceParameterBufferVP8 *)encode_state->seq_param_ext->buffer;
-VAEncMiscParameterBuffer* misc_param_hrd = 
(VAEncMiscParameterBuffer*)encode_state->misc_param[VAEncMiscParameterTypeHRD][0]->buffer;
-VAEncMiscParameterHRD* param_hrd = 
(VAEncMiscParameterHRD*)misc_param_hrd->data;
-VAEncMiscParameterBuffer* misc_param_frame_rate_buffer = 
(VAEncMiscParameterBuffer*)encode_state->misc_param[VAEncMiscParameterTypeFrameRate][0]->buffer;
-VAEncMiscParameterFrameRate* param_frame_rate = 
(VAEncMiscParameterFrameRate*)misc_param_frame_rate_buffer->data;
 double bitrate = seq_param->bits_per_second;
-unsigned int frame_rate = param_frame_rate->framerate;
+double framerate = (double)encoder_context->brc.framerate_num[0] / 
(double)encoder_context->brc.framerate_den[0];
 int inum = 1, pnum = 0;
 int intra_period = seq_param->intra_period;
 int width_in_mbs = ALIGN(seq_param->frame_width, 16) / 16;
@@ -3340,14 +3336,14 @@ static void gen8_mfc_vp8_brc_init(struct encode_state 
*encode_state,
 
 mfc_context->brc.mode = encoder_context->rate_control_mode;
 
-mfc_context->brc.target_frame_size[0][SLICE_TYPE_I] = 
(int)((double)((bitrate * intra_period)/frame_rate) /
+mfc_context->brc.target_frame_size[0][SLICE_TYPE_I] = 
(int)((double)((bitrate * intra_period) / framerate) /
  (double)(inum + 
BRC_PWEIGHT * pnum ));
 mfc_context->brc.target_frame_size[0][SLICE_TYPE_P] = BRC_PWEIGHT * 
mfc_context->brc.target_frame_size[0][SLICE_TYPE_I];
 
 mfc_context->brc.gop_nums[0][SLICE_TYPE_I] = inum;
 mfc_context->brc.gop_nums[0][SLICE_TYPE_P] = pnum;
 
-mfc_context->brc.bits_per_frame[0] = bitrate/frame_rate;
+mfc_context->brc.bits_per_frame[0] = bitrate / framerate;
 
 mfc_context->brc.qp_prime_y[0][SLICE_TYPE_I] = 
gen8_mfc_vp8_qindex_estimate(encode_state,

 mfc_context,
@@ -3358,10 +3354,15 @@ static void gen8_mfc_vp8_brc_init(struct encode_state 
*encode_state,

 mfc_context->brc.target_frame_size[0][SLICE_TYPE_P],

 0);
 
-mfc_context->hrd.buffer_size[0] = (double)param_hrd->buffer_size;
-mfc_context->hrd.current_buffer_fullness[0] =
-(double)(param_hrd->initial_buffer_fullness < 
mfc_context->hrd.buffer_size[0])?
-param_hrd->initial_buffer_fullness: mfc_context->hrd.buffer_size[0]/2.;
+if (encoder_context->brc.hrd_buffer_size)
+mfc_context->hrd.buffer_size[0] = 
(double)encoder_context->brc.hrd_buffer_size;
+else
+mfc_context->hrd.buffer_size[0] = 2.0 * bitrate;
+if (encoder_context->brc.hrd_initial_buffer_fullness &&
+encoder_context->brc.hrd_initial_buffer_fullness < 
mfc_context->hrd.buffer_size[0])
+mfc_context->hrd.current_buffer_fullness[0] = 
(double)encoder_context->brc.hrd_initial_buffer_fullness;
+else
+mfc_context->hrd.current_buffer_fullness[0] = 
mfc_context->hrd.buffer_size[0] / 2.0;
 mfc_context->hrd.target_buffer_fullness[0] = 
(double)mfc_context->hrd.buffer_size[0]/2.;
 mfc_context->hrd.buffer_capacity[0] = 
(double)mfc_context->hrd.buffer_size[0]/max_frame_size;
 mfc_context->hrd.violation_noted = 0;
@@ -3509,45 +3510,6 @@ static void gen8_mfc_vp8_hrd_context_update(struct 
encode_state *encode_state,
 mfc_context->vui_hrd.i_frame_number++;
 }
 
-/*
- * Check whether the parameters related with CBR are updated and decide whether
- * it needs to reinitialize the configuration related with CBR.
- * Currently

[Libva] [PATCH 2/4] HEVC encoder: use generic rate control parameters

[Mark Thompson]

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
 src/gen9_mfc.h  |  6 -
 src/gen9_mfc_hevc.c | 67 +
 src/i965_encoder.c  | 78 +
 3 files changed, 80 insertions(+), 71 deletions(-)

diff --git a/src/gen9_mfc.h b/src/gen9_mfc.h
index f7dc572..8e7d5ad 100644
--- a/src/gen9_mfc.h
+++ b/src/gen9_mfc.h
@@ -153,12 +153,6 @@ struct gen9_hcpe_context {
 int target_frame_size[3]; // I,P,B
 double bits_per_frame;
 double qpf_rounding_accumulator;
-
-double saved_bps;
-double saved_fps;
-int saved_intra_period;
-int saved_ip_period;
-int saved_idr_period;
 } brc;
 
 struct {
diff --git a/src/gen9_mfc_hevc.c b/src/gen9_mfc_hevc.c
index be5666c..b4eca4f 100644
--- a/src/gen9_mfc_hevc.c
+++ b/src/gen9_mfc_hevc.c
@@ -2214,11 +2214,9 @@ static void intel_hcpe_brc_init(struct encode_state 
*encode_state,
 {
 struct gen9_hcpe_context *mfc_context = encoder_context->mfc_context;
 VAEncSequenceParameterBufferHEVC *pSequenceParameter = 
(VAEncSequenceParameterBufferHEVC *)encode_state->seq_param_ext->buffer;
-VAEncMiscParameterHRD* pParameterHRD = NULL;
-VAEncMiscParameterBuffer* pMiscParamHRD = NULL;
 
 double bitrate = pSequenceParameter->bits_per_second * 1.0;
-double framerate = (double)pSequenceParameter->vui_time_scale / 
(double)pSequenceParameter->vui_num_units_in_tick;
+double framerate = (double)encoder_context->brc.framerate_num[0] / 
(double)encoder_context->brc.framerate_den[0];
 int inum = 1, pnum = 0, bnum = 0; /* Gop structure: number of I, P, B 
frames in the Gop. */
 int intra_period = pSequenceParameter->intra_period;
 int ip_period = pSequenceParameter->ip_period;
@@ -2238,12 +2236,6 @@ static void intel_hcpe_brc_init(struct encode_state 
*encode_state,
 qp1_size = qp1_size * bpp;
 qp51_size = qp51_size * bpp;
 
-if (!encode_state->misc_param[VAEncMiscParameterTypeHRD][0] || 
!encode_state->misc_param[VAEncMiscParameterTypeHRD][0]->buffer)
-return;
-
-pMiscParamHRD = 
(VAEncMiscParameterBuffer*)encode_state->misc_param[VAEncMiscParameterTypeHRD][0]->buffer;
-pParameterHRD = (VAEncMiscParameterHRD*)pMiscParamHRD->data;
-
 if (pSequenceParameter->ip_period) {
 pnum = (intra_period + ip_period - 1) / ip_period - 1;
 bnum = intra_period - inum - pnum;
@@ -2262,7 +2254,7 @@ static void intel_hcpe_brc_init(struct encode_state 
*encode_state,
 
 bpf = mfc_context->brc.bits_per_frame = bitrate / framerate;
 
-if (!pParameterHRD || pParameterHRD->buffer_size <= 0)
+if (!encoder_context->brc.hrd_buffer_size)
 {
 mfc_context->hrd.buffer_size = bitrate * ratio;
 mfc_context->hrd.current_buffer_fullness =
@@ -2270,7 +2262,7 @@ static void intel_hcpe_brc_init(struct encode_state 
*encode_state,
 bitrate * ratio/2 : mfc_context->hrd.buffer_size / 2.;
 }else
 {
-buffer_size = (double)pParameterHRD->buffer_size ;
+buffer_size = (double)encoder_context->brc.hrd_buffer_size;
 if(buffer_size < bitrate * ratio_min)
 {
 buffer_size = bitrate * ratio_min;
@@ -2279,11 +2271,11 @@ static void intel_hcpe_brc_init(struct encode_state 
*encode_state,
 buffer_size = bitrate * ratio_max ;
 }
 mfc_context->hrd.buffer_size =buffer_size;
-if(pParameterHRD->initial_buffer_fullness > 0)
+if(encoder_context->brc.hrd_initial_buffer_fullness)
 {
 mfc_context->hrd.current_buffer_fullness =
-(double)(pParameterHRD->initial_buffer_fullness < 
mfc_context->hrd.buffer_size) ?
-pParameterHRD->initial_buffer_fullness : 
mfc_context->hrd.buffer_size / 2.;
+(double)(encoder_context->brc.hrd_initial_buffer_fullness < 
mfc_context->hrd.buffer_size) ?
+encoder_context->brc.hrd_initial_buffer_fullness : 
mfc_context->hrd.buffer_size / 2.;
 }else
 {
 mfc_context->hrd.current_buffer_fullness = 
mfc_context->hrd.buffer_size / 2.;
@@ -2519,51 +2511,6 @@ int intel_hcpe_interlace_check(VADriverContextP ctx,
 return 1;
 }
 
-/*
- * Check whether the parameters related with CBR are updated and decide whether
- * it needs to reinitialize the configuration related with CBR.
- * Currently it will check the following parameters:
- *  bits_per_second
- *  frame_rate
- *  gop_configuration(intra_period, ip_period, intra_idr_period)
- */
-static bool intel_hcpe_brc_updated_check(struct encode_state *encode_state,
-struct intel_encoder_context *encoder_context)
-{
-/* to do */
-unsigned int rate_control_mode = encoder_context->rate_control_mode;
-struct gen9_hcpe_con

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

[Mark Thompson]

Update references in both H.264 encoders (gen6_mfc and gen9_vdenc).

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
 src/gen6_mfc_common.c | 11 ++
 src/gen9_vdenc.c  | 29 ++--
 src/gen9_vdenc.h  |  3 ++-
 src/i965_encoder.c| 61 +--
 src/i965_encoder.h|  3 ++-
 5 files changed, 72 insertions(+), 35 deletions(-)

diff --git a/src/gen6_mfc_common.c b/src/gen6_mfc_common.c
index 68d030e..c29817e 100644
--- a/src/gen6_mfc_common.c
+++ b/src/gen6_mfc_common.c
@@ -119,16 +119,19 @@ static void intel_mfc_brc_init(struct encode_state 
*encode_state,
 
 if (i == 0) {
 bitrate = encoder_context->brc.bits_per_second[0];
-framerate = (double)encoder_context->brc.framerate_per_100s[0] / 
100.0;
+framerate = (double)encoder_context->brc.framerate_num[0] / 
(double)encoder_context->brc.framerate_den[0];
 } else {
 bitrate = (encoder_context->brc.bits_per_second[i] - 
encoder_context->brc.bits_per_second[i - 1]);
-framerate = (double)(encoder_context->brc.framerate_per_100s[i] - 
encoder_context->brc.framerate_per_100s[i - 1]) / 100.0;
+framerate = ((double)encoder_context->brc.framerate_num[i] / 
(double)encoder_context->brc.framerate_den[i]) -
+((double)encoder_context->brc.framerate_num[i - 1] / 
(double)encoder_context->brc.framerate_den[i - 1]);
 }
 
 if (i == encoder_context->layer.num_layers - 1)
 factor = 1.0;
-else
-factor = (double)encoder_context->brc.framerate_per_100s[i] / 
encoder_context->brc.framerate_per_100s[encoder_context->layer.num_layers - 1];
+else {
+factor = ((double)encoder_context->brc.framerate_num[i] / 
(double)encoder_context->brc.framerate_den[i]) /
+((double)encoder_context->brc.framerate_num[i - 1] / 
(double)encoder_context->brc.framerate_den[i - 1]);
+}
 
 hrd_factor = (double)bitrate / 
encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1];
 
diff --git a/src/gen9_vdenc.c b/src/gen9_vdenc.c
index 8009b31..b2d0b21 100644
--- a/src/gen9_vdenc.c
+++ b/src/gen9_vdenc.c
@@ -851,7 +851,8 @@ gen9_vdenc_update_misc_parameters(VADriverContextP ctx,
 if (vdenc_context->internal_rate_mode != I965_BRC_CQP &&
 encoder_context->brc.need_reset) {
 /* So far, vdenc doesn't support temporal layer */
-vdenc_context->frames_per_100s = 
encoder_context->brc.framerate_per_100s[0];
+vdenc_context->framerate_num = encoder_context->brc.framerate_num[0];
+vdenc_context->framerate_den = encoder_context->brc.framerate_den[0];
 
 vdenc_context->vbv_buffer_size_in_bit = 
encoder_context->brc.hrd_buffer_size;
 vdenc_context->init_vbv_buffer_fullness_in_bit = 
encoder_context->brc.hrd_initial_buffer_fullness;
@@ -927,7 +928,8 @@ gen9_vdenc_update_parameters(VADriverContextP ctx,
  !vdenc_context->vbv_buffer_size_in_bit ||
  !vdenc_context->max_bit_rate ||
  !vdenc_context->target_bit_rate ||
- !vdenc_context->frames_per_100s))
+ !vdenc_context->framerate_num ||
+ !vdenc_context->framerate_den))
 vdenc_context->brc_enabled = 0;
 
 if (!vdenc_context->brc_enabled) {
@@ -1565,7 +1567,8 @@ gen9_vdenc_get_profile_level_max_frame(VADriverContextP 
ctx,
 tmpf = max_mbps / 172.0;
 
 max_byte_per_frame0 = (uint64_t)(tmpf * bits_per_mb);
-max_byte_per_frame1 = (uint64_t)(((double)max_mbps * 100) / 
vdenc_context->frames_per_100s *bits_per_mb);
+max_byte_per_frame1 = (uint64_t)(((double)max_mbps * 
vdenc_context->framerate_den) /
+ (double)vdenc_context->framerate_num * 
bits_per_mb);
 
 /* TODO: check VAEncMiscParameterTypeMaxFrameSize */
 ret = (unsigned int)MIN(max_byte_per_frame0, max_byte_per_frame1);
@@ -1586,12 +1589,12 @@ gen9_vdenc_calculate_initial_qp(VADriverContextP ctx,
 
 frame_size = (vdenc_context->frame_width * vdenc_context->frame_height * 3 
/ 2);
 qp = (int)(1.0 / 1.2 * pow(10.0,
-   (log10(frame_size * 2.0 / 3.0 * 
((float)vdenc_context->frames_per_100s) /
-  ((float)(vdenc_context->target_bit_rate 
* 1000) * 100)) - x0) *
+   (log10(frame_size * 2.0 / 3.0 * 
vdenc_context->framerate_num /
+  ((double)vdenc_context->target_bit_rate 
* 1000.0 * vdenc_context->framerate_den)) - x0) *
(y1 - y0) / (x1 - x0) + y0) + 0.5);
 qp += 2;
-delat_qp = (int)(9 - (vdenc_context->vbv_buffer_size_in_bit * 
((float)vdenc_context->frames_per_100s) /
-  

Re: [Libva] [PATCH 3/3] VP9 encode: support fractional framerate

[Mark Thompson]

On 08/12/16 01:13, Zhao Yakui wrote:
> On 12/07/2016 01:48 PM, Xiang, Haihao wrote:
>> Now we have the same way for frame rate for each codec in i965_encoder.c, it 
>> would be better to use the result directly
> 
> Hi, Mark
> 
> Can you refresh your patch based on Haihao's suggestion?
> Now the frame rate is parsed in the function of 
> intel_encoder_check_framerate_parameter. (It is defined in i965_encoder.c) 
> and it is stored as framerate_per_100s.
> So the vp9 can use the framerate_per_100s for the frame_rate_m. The 
> frame_rate_d can be 100.

Right, I went a bit further than that.  The following removes all use of any of 
the RC parameter buffers (RC, HRD, framerate) from the specific encoders, 
moving top-level RC code into i965_encoder.c.

Tested with VP8, VP9, H.264 and H.265 on Kaby Lake.  I haven't tested the 
low-power H.264 encoder (gen9_vdenc.c) because it is only enabled for Skylake 
using CQP at the moment, though there are some edits there.

Thanks,

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

Re: [Libva] [PATCH 1/3] Declare I010 support for QueryImageFormats

[Mark Thompson]

On 06/12/16 01:37, Zhao Yakui wrote:
> On 12/06/2016 01:49 AM, Mark Thompson wrote:
>> Signed-off-by: Mark Thompson<s...@jkqxz.net>
>> ---
>> It's already returned as a usable surface format for the video processor, 
>> but is missing from the list returned by vaQueryImageFormats().
>>
>>   src/i965_drv_video.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/src/i965_drv_video.c b/src/i965_drv_video.c
>> index 79a2aec..d83427c 100644
>> --- a/src/i965_drv_video.c
>> +++ b/src/i965_drv_video.c
>> @@ -268,6 +268,8 @@ i965_image_formats_map[I965_MAX_IMAGE_FORMATS + 1] = {
>> { VA_FOURCC_BGRX, VA_LSB_FIRST, 32, 24, 0x00ff, 0xff00, 
>> 0x00ff } },
>>   { I965_SURFACETYPE_YUV,
>> { VA_FOURCC_P010, VA_LSB_FIRST, 24, } },
>> +{ I965_SURFACETYPE_YUV,
>> +  { VA_FOURCC_I010, VA_LSB_FIRST, 24, } },
>>   };
>>
> 
> The format returned by vaQueryImageFormats is mainly used for calling 
> vaCreateImage. That is to say: if we want to add the I010 to the image_format 
> list, we should firstly add the support in i965_CreateImage.
> 
> For the 10-bit surfaces(P010, I010), it will be preferred that it is created 
> by using vaCreateSurfaces. If the image is needed, the vaDeriveImage can be 
> used instead.
> 
> Is it OK to you?

Right, testing a bit more I can see there is more support missing than I 
initially thought.  It needs support throughout the VPP processing chain for 
vaGetImage() in particular to work, so the cases I was hoping for aren't yet 
possible.

The use-case I'm thinking of here is to be able to move data back and forth 
between software (typically LSB-packed three-plane YUV 4:2:0, here being called 
I010: e.g. libx264, libvpx, libavcodec) and hardware (all MSB-packed two-plane 
YUV 4:2:0, i.e. P010) without an additional shift/(de)interleave step in 
software.  Since we want to avoid CPU involvement where possible, full image 
support is helpful because we can get the GPU to deal with the 
copying/conversion between normal memory and surfaces - vaDeriveImage() is not 
really sufficient because the CPU then has to do the copy (and also has to deal 
with the uncached memory requiring the streaming operations, making it even 
more awkward to use).

I'll drop this patch for now, since the changes required to make that work are 
somewhat more involved than I was hoping.

Thanks,

- Mark

___
Libva mailing list
Libva@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libva

Re: [Libva] [PATCH] Don't automatically destroy the buffer(s) passed to vaRenderPicture

[Mark Thompson]

On 01/12/16 08:55, Xiang, Haihao wrote:
> Instead the user must call vaDestroyBuffer() to destroy a buffer explicitly.
> 
> If following the previous API specification,
> 1. Violate "who allocate who release" principle
> 2. The user cannot re-use VA buffer flexibly
> 3. The user still has to call vaDestroyBuffer() to destroy the buffers which
>are not going to be passed to vaRenderPicture()
> 
> We discussed the change at https://bugs.freedesktop.org/show_bug.cgi?id=97970
> 
> Signed-off-by: Xiang, Haihao 
> ---
>  va/va.h  |  9 +++--
>  va/va_enc_h264.h |  3 +--
>  va/va_vpp.h  | 12 +++-
>  3 files changed, 11 insertions(+), 13 deletions(-)

This is a totally incompatible change to VAAPI, so please make libva before and 
after this change clearly distinguishable.

Probably you should increment the major version number and change the SONAME of 
libva, though I realise commercial concerns will likely block that.  At least 
increment the minor version number if nothing else can be done.

Thanks,

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

[Libva] [PATCH 3/3] VP9 encode: support fractional framerate

[Mark Thompson]

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
Tested on Kaby Lake.  Someone who has access to the manuals should make sure 
that the framerate numerator/denominator actually works there as I am guessing 
it does.

Removes the frame_rate field in struct gen9_vp9_brc_curbe_param, because there 
is no longer a sensible value to set it to (and also it's not read anywhere).  
If it is needed, it wants to be replaced by the two parts of the fraction.

 src/gen9_vp9_encoder.c | 45 ++---
 src/gen9_vp9_encoder.h |  4 ++--
 2 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/src/gen9_vp9_encoder.c b/src/gen9_vp9_encoder.c
index 3ea1537..8ff1b9b 100644
--- a/src/gen9_vp9_encoder.c
+++ b/src/gen9_vp9_encoder.c
@@ -1201,8 +1201,8 @@ void gen9_vp9_set_curbe_brc(VADriverContextP ctx,
   VP9_BRC_KBPS;
 cmd->dw9.min_bit_rate   = (vp9_state->min_bit_rate  + 
VP9_BRC_KBPS - 1) / VP9_BRC_KBPS *
   VP9_BRC_KBPS;
-cmd->dw10.frame_ratem   = vp9_state->frame_rate;
-cmd->dw11.frame_rated   = 1;
+cmd->dw10.frame_ratem   = vp9_state->frame_rate_num;
+cmd->dw11.frame_rated   = vp9_state->frame_rate_den;
 
 cmd->dw14.avbr_accuracy = 30;
 cmd->dw14.avbr_convergence  = 150;
@@ -1235,7 +1235,7 @@ void gen9_vp9_set_curbe_brc(VADriverContextP ctx,
 cmd->dw17.enable_dynamic_scaling = vp9_state->dys_in_use;
 cmd->dw17.brc_overshoot_cbr_pct = 150;
 
-dInputBitsPerFrame = (double)(cmd->dw8.max_bit_rate) / 
(vp9_state->frame_rate);
+dInputBitsPerFrame = ((double)cmd->dw8.max_bit_rate * 
vp9_state->frame_rate_den) / (vp9_state->frame_rate_num);
 dbps_ratio = dInputBitsPerFrame / 
((double)(vp9_state->vbv_buffer_size_in_bit) / 30);
 if (dbps_ratio < 0.1)
 dbps_ratio = 0.1;
@@ -1423,7 +1423,6 @@ gen9_vp9_brc_init_reset_kernel(VADriverContextP ctx,
 brc_initreset_curbe.initbrc= !vp9_state->brc_inited;
 brc_initreset_curbe.mbbrc_enabled  = 0;
 brc_initreset_curbe.ref_frame_flag  = vp9_state->ref_frame_flag;
-brc_initreset_curbe.frame_rate   = vp9_state->frame_rate;
 
 vme_context->pfn_set_curbe_brc(ctx, encode_state,
gpe_context,
@@ -1523,7 +1522,6 @@ gen9_vp9_brc_intra_dist_kernel(VADriverContextP ctx,
 brc_intra_dist_curbe.initbrc= !vp9_state->brc_inited;
 brc_intra_dist_curbe.mbbrc_enabled  = 0;
 brc_intra_dist_curbe.ref_frame_flag  = vp9_state->ref_frame_flag;
-brc_intra_dist_curbe.frame_rate   = vp9_state->frame_rate;
 
 vme_context->pfn_set_curbe_brc(ctx, encode_state,
gpe_context,
@@ -3964,10 +3962,17 @@ gen9_encode_vp9_check_parameter(VADriverContextP ctx,
 
encode_state->misc_param[VAEncMiscParameterTypeFrameRate][0]->buffer;
 misc_param_fr = (VAEncMiscParameterFrameRate 
*)misc_param->data;
 
-vp9_state->frame_rate = misc_param_fr->framerate;
+if (misc_param_fr->framerate & 0x) {
+vp9_state->frame_rate_num = misc_param_fr->framerate 
>> 16 & 0x;
+vp9_state->frame_rate_den = misc_param_fr->framerate   
& 0x;
+} else {
+vp9_state->frame_rate_num = misc_param_fr->framerate;
+vp9_state->frame_rate_den = 1;
+}
 } else {
 /* Assign the default frame rate */
-vp9_state->frame_rate = 30;
+vp9_state->frame_rate_num = 30;
+vp9_state->frame_rate_den = 1;
 }
 
 /* RC misc will override HRD parameter */
@@ -3999,10 +4004,17 @@ gen9_encode_vp9_check_parameter(VADriverContextP ctx,
 
encode_state->misc_param[VAEncMiscParameterTypeFrameRate][0]->buffer;
 misc_param_fr = (VAEncMiscParameterFrameRate 
*)misc_param->data;
 
-vp9_state->frame_rate = misc_param_fr->framerate;
+if (misc_param_fr->framerate & 0x) {
+vp9_state->frame_rate_num = misc_param_fr->framerate 
>> 16 & 0x;
+vp9_state->frame_rate_den = misc_param_fr->framerate   
& 0x;
+} else {
+vp9_state->frame_rate_num = misc_param_fr->framerate;
+vp9_state->frame_rate

[Libva] [PATCH 1/3] Declare I010 support for QueryImageFormats

[Mark Thompson]

Signed-off-by: Mark Thompson <s...@jkqxz.net>
---
It's already returned as a usable surface format for the video processor, but 
is missing from the list returned by vaQueryImageFormats().

 src/i965_drv_video.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/i965_drv_video.c b/src/i965_drv_video.c
index 79a2aec..d83427c 100644
--- a/src/i965_drv_video.c
+++ b/src/i965_drv_video.c
@@ -268,6 +268,8 @@ i965_image_formats_map[I965_MAX_IMAGE_FORMATS + 1] = {
   { VA_FOURCC_BGRX, VA_LSB_FIRST, 32, 24, 0x00ff, 0xff00, 
0x00ff } },
 { I965_SURFACETYPE_YUV,
   { VA_FOURCC_P010, VA_LSB_FIRST, 24, } },
+{ I965_SURFACETYPE_YUV,
+  { VA_FOURCC_I010, VA_LSB_FIRST, 24, } },
 };
 
 /* List of supported subpicture formats */
-- 
2.10.2

___
Libva mailing list
Libva@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libva