On 27/01/17 22:06, Andy Furniss wrote:
> Mark Thompson wrote:
>> On 26/01/17 18:26, Andy Furniss wrote:
>>> Tested with ffmpeg and gst-vaapi. Without this bits per
>>> frame is set way too low.
>>>
>>> Signed-off-by: Andy Furniss <adf.li...@gmail.com>
>>> ---
>>> src/gallium/state_trackers/va/picture.c | 32
>>> ++++++++++++++++++++++++--------
>>> 1 file changed, 24 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/gallium/state_trackers/va/picture.c
>>> b/src/gallium/state_trackers/va/picture.c
>>> index 82584ea..a024437 100644
>>> --- a/src/gallium/state_trackers/va/picture.c
>>> +++ b/src/gallium/state_trackers/va/picture.c
>>> @@ -119,14 +119,30 @@ getEncParamPreset(vlVaContext *context)
>>> context->desc.h264enc.rate_ctrl.fill_data_enable = 1;
>>> context->desc.h264enc.rate_ctrl.enforce_hrd = 1;
>>> context->desc.h264enc.enable_vui = false;
>>> - if (context->desc.h264enc.rate_ctrl.frame_rate_num == 0)
>>> - context->desc.h264enc.rate_ctrl.frame_rate_num = 30;
>>> - context->desc.h264enc.rate_ctrl.target_bits_picture =
>>> - context->desc.h264enc.rate_ctrl.target_bitrate /
>>> context->desc.h264enc.rate_ctrl.frame_rate_num;
>>> - context->desc.h264enc.rate_ctrl.peak_bits_picture_integer =
>>> - context->desc.h264enc.rate_ctrl.peak_bitrate /
>>> context->desc.h264enc.rate_ctrl.frame_rate_num;
>>> - context->desc.h264enc.rate_ctrl.peak_bits_picture_fraction = 0;
>>> + if (context->desc.h264enc.rate_ctrl.frame_rate_num == 0 ||
>>> + context->desc.h264enc.rate_ctrl.frame_rate_den == 0) {
>>> + context->desc.h264enc.rate_ctrl.frame_rate_num = 30;
>>> + context->desc.h264enc.rate_ctrl.frame_rate_den = 1;
>>> + }
>>> + if (context->desc.h264enc.rate_ctrl.frame_rate_den > 1) {
>>> + context->desc.h264enc.rate_ctrl.target_bits_picture =
>>> + context->desc.h264enc.rate_ctrl.target_bitrate /
>>> + (context->desc.h264enc.rate_ctrl.frame_rate_num /
>>> + context->desc.h264enc.rate_ctrl.frame_rate_den + 1);
>>
>> This is still doing more rounding than necessary? (Consider 0.5fps as 1/2,
>> say.) Don't you want:
>
> Hmm, sub 1 fps is going to be treated as 1 fps with my code but -
>
>>
>> context->desc.h264enc.rate_ctrl.target_bits_picture =
>> (context->desc.h264enc.rate_ctrl.target_bitrate *
>> context->desc.h264enc.rate_ctrl.frame_rate_den) /
>> context->desc.h264enc.rate_ctrl.frame_rate_num;
>>
>> (And no need for any extra +1 or conditional, because you've already made
>> sure that neither of them are zero.)
>
> This will overflow, with high bitrate plus ntsc denom = 1001
Urgh, yes, so it will. Apologies for missing that.
> Could use floats I guess like I did here.
>
> https://cgit.freedesktop.org/mesa/mesa/commit/?id=a5993022275c20061ac025d9adc26c5f9d02afee
>
> I don't know what the preference is between float and int.
I think float is fine here? I don't know what the preference is either...
(uint64_t would also be big enough, I think.)
As an alternative source of inspiration,
<https://cgit.freedesktop.org/vaapi/intel-driver/tree/src/gen6_mfc_common.c#n93>
is full of doubles ;)
>>> + context->desc.h264enc.rate_ctrl.peak_bits_picture_integer =
>>> + context->desc.h264enc.rate_ctrl.peak_bitrate /
>>> + (context->desc.h264enc.rate_ctrl.frame_rate_num /
>>> + context->desc.h264enc.rate_ctrl.frame_rate_den + 1);
>>
>> Similarly this one.
>>
>>> + } else {
>>> + context->desc.h264enc.rate_ctrl.target_bits_picture =
>>> + context->desc.h264enc.rate_ctrl.target_bitrate /
>>> + context->desc.h264enc.rate_ctrl.frame_rate_num;
>>> + context->desc.h264enc.rate_ctrl.peak_bits_picture_integer =
>>> + context->desc.h264enc.rate_ctrl.peak_bitrate /
>>> + context->desc.h264enc.rate_ctrl.frame_rate_num;
>>> + }
>>
>> Doing that would also avoid needing the if at all.
>>
>>>
>>> + context->desc.h264enc.rate_ctrl.peak_bits_picture_fraction = 0;
>>> context->desc.h264enc.ref_pic_mode = 0x00000201;
>>> }
>>>
>>> @@ -362,7 +378,7 @@ handleVAEncSequenceParameterBufferType(vlVaDriver *drv,
>>> vlVaContext *context, vl
>>> context->gop_coeff = VL_VA_ENC_GOP_COEFF;
>>> context->desc.h264enc.gop_size = h264->intra_idr_period *
>>> context->gop_coeff;
>>> context->desc.h264enc.rate_ctrl.frame_rate_num = h264->time_scale / 2;
>>> - context->desc.h264enc.rate_ctrl.frame_rate_den = 1;
>>> + context->desc.h264enc.rate_ctrl.frame_rate_den =
>>> h264->num_units_in_tick;
>>> return VA_STATUS_SUCCESS;
>>> }
>>
>> Otherwise LGTM :)
>
> Thanks.
>
>>
>> Thanks,
>>
>> - Mark
>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
