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

[mypopy at gmail.com]

On Fri, Aug 21, 2020 at 7:03 AM Mark Thompson <sw at jkqxz.net> wrote:
>
> 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.
>
I believe  the  parameter  int quality in function int
ff_side_data_set_encoder_stats(AVPacket *pkt, int quality, int64_t
*error, int error_count, int pict_type) is  inconsistent, I think need
to more check/clarification in  different codec