diff mbox series

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

Message ID 1595766367-8861-3-git-send-email-mypopydev@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/5] lavc/libkvazaar: fix framerate setting | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Jun Zhao July 26, 2020, 12:26 p.m. UTC 
From: Jun Zhao <barryjzhao@tencent.com>

Export choosen pict_type and qp.

Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
---
 libavcodec/libkvazaar.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Comments

Mark Thompson Aug. 8, 2020, 10:02 p.m. UTC | #1 
On 26/07/2020 13:26, Jun Zhao wrote:
> From: Jun Zhao <barryjzhao@tencent.com>
> 
> Export choosen pict_type and qp.
> 
> Signed-off-by: Jun Zhao <barryjzhao@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.

> +
> +        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...

(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].

> +
> +#if FF_API_CODED_FRAME
> +FF_DISABLE_DEPRECATION_WARNINGS
> +        avctx->coded_frame->quality = frame_info.qp * FF_QP2LAMBDA;
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
> +
>           *got_packet_ptr = 1;
>       }
>   
>
- Mark
Jun Zhao Aug. 15, 2020, 8:48 a.m. UTC | #2 
On Sun, Aug 9, 2020 at 6:07 AM Mark Thompson <sw@jkqxz.net> wrote:
>
> On 26/07/2020 13:26, Jun Zhao wrote:
> > From: Jun Zhao <barryjzhao@tencent.com>
> >
> > Export choosen pict_type and qp.
> >
> > Signed-off-by: Jun Zhao <barryjzhao@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
> > +
> > +#if FF_API_CODED_FRAME
> > +FF_DISABLE_DEPRECATION_WARNINGS
> > +        avctx->coded_frame->quality = frame_info.qp * FF_QP2LAMBDA;
> > +FF_ENABLE_DEPRECATION_WARNINGS
> > +#endif
> > +
> >           *got_packet_ptr = 1;
> >       }
> >
> >
> - Mark
Mark Thompson Aug. 20, 2020, 11:02 p.m. UTC | #3 
On 15/08/2020 09:48, mypopy@gmail.com wrote:
> On Sun, Aug 9, 2020 at 6:07 AM Mark Thompson <sw@jkqxz.net> wrote:
>>
>> On 26/07/2020 13:26, Jun Zhao wrote:
>>> From: Jun Zhao <barryjzhao@tencent.com>
>>>
>>> Export choosen pict_type and qp.
>>>
>>> Signed-off-by: Jun Zhao <barryjzhao@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
James Almer Aug. 20, 2020, 11:07 p.m. UTC | #4 
On 8/15/2020 5:48 AM, mypopy@gmail.com wrote:
> On Sun, Aug 9, 2020 at 6:07 AM Mark Thompson <sw@jkqxz.net> wrote:
>>
>> On 26/07/2020 13:26, Jun Zhao wrote:
>>> From: Jun Zhao <barryjzhao@tencent.com>
>>>
>>> Export choosen pict_type and qp.
>>>
>>> Signed-off-by: Jun Zhao <barryjzhao@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

No, the usage of deprecated features should not be expanded. It's kept
in old encoders for compatibility reasons, but new ones should not use
it at all. So please remove it.

>>> +
>>> +        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
>>> +
>>> +#if FF_API_CODED_FRAME
>>> +FF_DISABLE_DEPRECATION_WARNINGS
>>> +        avctx->coded_frame->quality = frame_info.qp * FF_QP2LAMBDA;
>>> +FF_ENABLE_DEPRECATION_WARNINGS
>>> +#endif
>>> +
>>>           *got_packet_ptr = 1;
>>>       }
>>>
>>>
>> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Jun Zhao Aug. 21, 2020, 11:09 a.m. UTC | #5 
On Fri, Aug 21, 2020 at 7:15 AM James Almer <jamrial@gmail.com> wrote:
>
> On 8/15/2020 5:48 AM, mypopy@gmail.com wrote:
> > On Sun, Aug 9, 2020 at 6:07 AM Mark Thompson <sw@jkqxz.net> wrote:
> >>
> >> On 26/07/2020 13:26, Jun Zhao wrote:
> >>> From: Jun Zhao <barryjzhao@tencent.com>
> >>>
> >>> Export choosen pict_type and qp.
> >>>
> >>> Signed-off-by: Jun Zhao <barryjzhao@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
>
> No, the usage of deprecated features should not be expanded. It's kept
> in old encoders for compatibility reasons, but new ones should not use
> it at all. So please remove it.
>
Will remove it, thx
> >>> +
> >>> +        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
> >>> +
> >>> +#if FF_API_CODED_FRAME
> >>> +FF_DISABLE_DEPRECATION_WARNINGS
> >>> +        avctx->coded_frame->quality = frame_info.qp * FF_QP2LAMBDA;
> >>> +FF_ENABLE_DEPRECATION_WARNINGS
> >>> +#endif
> >>> +
> >>>           *got_packet_ptr = 1;
> >>>       }
> >>>
> >>>
> >> - Mark
Jun Zhao Aug. 21, 2020, 11:12 a.m. UTC | #6 
On Fri, Aug 21, 2020 at 7:03 AM Mark Thompson <sw@jkqxz.net> wrote:
>
> On 15/08/2020 09:48, mypopy@gmail.com wrote:
> > On Sun, Aug 9, 2020 at 6:07 AM Mark Thompson <sw@jkqxz.net> wrote:
> >>
> >> On 26/07/2020 13:26, Jun Zhao wrote:
> >>> From: Jun Zhao <barryjzhao@tencent.com>
> >>>
> >>> Export choosen pict_type and qp.
> >>>
> >>> Signed-off-by: Jun Zhao <barryjzhao@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
diff mbox series

Patch

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
+
+        ff_side_data_set_encoder_stats(avpkt, frame_info.qp * FF_QP2LAMBDA, NULL, 0, pict_type);
+
+#if FF_API_CODED_FRAME
+FF_DISABLE_DEPRECATION_WARNINGS
+        avctx->coded_frame->quality = frame_info.qp * FF_QP2LAMBDA;
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+
         *got_packet_ptr = 1;
     }