[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 = 1000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 2000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 3000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 4000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 5000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 6000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 7000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 8000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 9000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 10000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000


Thanks,

- Mark