[FFmpeg-devel] [PATCH 3/5] lavc/libkvazaar: export encoded frame stats

[Mark Thompson]

On 15/08/2020 09:48, mypopy at gmail.com wrote:
> On Sun, Aug 9, 2020 at 6:07 AM Mark Thompson <sw at jkqxz.net> wrote:
>>
>> On 26/07/2020 13:26, Jun Zhao wrote:
>>> From: Jun Zhao <barryjzhao at tencent.com>
>>>
>>> Export choosen pict_type and qp.
>>>
>>> Signed-off-by: Jun Zhao <barryjzhao at tencent.com>
>>> ---
>>>    libavcodec/libkvazaar.c | 30 ++++++++++++++++++++++++++++++
>>>    1 file changed, 30 insertions(+)
>>>
>>> diff --git a/libavcodec/libkvazaar.c b/libavcodec/libkvazaar.c
>>> index 71c9c8f..9032547 100644
>>> --- a/libavcodec/libkvazaar.c
>>> +++ b/libavcodec/libkvazaar.c
>>> @@ -37,6 +37,7 @@
>>>
>>>    #include "avcodec.h"
>>>    #include "internal.h"
>>> +#include "packet_internal.h"
>>>
>>>    typedef struct LibkvazaarContext {
>>>        const AVClass *class;
>>> @@ -170,6 +171,7 @@ static int libkvazaar_encode(AVCodecContext *avctx,
>>>        kvz_data_chunk *data_out = NULL;
>>>        uint32_t len_out = 0;
>>>        int retval = 0;
>>> +    int pict_type;
>>>
>>>        *got_packet_ptr = 0;
>>>
>>> @@ -257,6 +259,34 @@ static int libkvazaar_encode(AVCodecContext *avctx,
>>>                avpkt->flags |= AV_PKT_FLAG_KEY;
>>>            }
>>>
>>> +        switch (frame_info.slice_type) {
>>> +        case KVZ_SLICE_I:
>>> +            pict_type = AV_PICTURE_TYPE_I;
>>> +            break;
>>> +        case KVZ_SLICE_P:
>>> +            pict_type = AV_PICTURE_TYPE_P;
>>> +            break;
>>> +        case KVZ_SLICE_B:
>>> +            pict_type = AV_PICTURE_TYPE_B;
>>> +            break;
>>> +        default:
>>> +            av_log(avctx, AV_LOG_ERROR, "Unknown picture type encountered.\n");
>>> +            return AVERROR_EXTERNAL;
>>> +        }
>>> +#if FF_API_CODED_FRAME
>>> +FF_DISABLE_DEPRECATION_WARNINGS
>>> +        avctx->coded_frame->pict_type = pict_type;
>>> +FF_ENABLE_DEPRECATION_WARNINGS
>>> +#endif
>>
>> Is there some value to setting the deprecated fields?  They will disappear on the next version bump, which probably isn't very far away.
>>
> I think we can keep this part, if we want to remove the
> avctx->coded_frame->pict_type from next version bump, we can drop this
> part from all codec one-time
>>> +
>>> +        ff_side_data_set_encoder_stats(avpkt, frame_info.qp * FF_QP2LAMBDA, NULL, 0, pict_type);
>>
>> I'm not familiar with kvazaar - is the QP value here actually reflective of the global quality in a useful way?  The documentation is not very good...
>>
> Yes, it's a global quality based on Frame in kvazaar
> 
>> (Your following patch for OpenH264 uses a field "AverageFrameQP", which has an much more usefully-suggestive name.)
>>
>> Zero is a valid QP value, but you shouldn't be passing it here.  Possibly it needs some more scaling to get the range to [1, FF_LAMDBA_MAX].
>>
> I know vpx encoder wrapper used the zero QP value in side data, maybe
> we can keep the same action

So the libvpx wrapper is also broken?  This all seems very inconsistent - the documentation requires that the number be in [1, FF_LAMBDA_MAX], but codecs are using random numbers which include zero.

libx264 is applying some sort of offset so that the returned qpplus1 field is actually qpplussomewherebetween1and30 (with 10-bit input qp == -12 gives qpplus1 == 1 and qp == 51 gives qpplus1 == 81), but we still subtract 1 so the result includes zero.

libx265 doesn't seem to support negative QPs so it's always in the 8-bit range, but that still includes zero.

nvenc has frameAvgQP - 1, which seems bizarre.  I guess it would work if it doesn't support QP < 2, but given that it allows 12-bit inputs (range -24 to 51) that seems unlikely.

Scaling to match what the documentation actually says seems like the sanest way to fix this - we can't change the range because zero is the "unused" value.

- Mark