diff mbox series

[FFmpeg-devel] avcodec/libvpxenc: return quantizer parameter for an encoded frame

Message ID 20220405121539.1773869-1-danilchap@google.com
State New
Headers show
Series [FFmpeg-devel] avcodec/libvpxenc: return quantizer parameter for an encoded frame | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_aarch64_jetson success Make finished
andriy/make_fate_aarch64_jetson success Make fate finished
andriy/make_armv7_RPi4 success Make finished
andriy/make_fate_armv7_RPi4 success Make fate finished

Commit Message

Danil Chapovalov April 5, 2022, 12:15 p.m. UTC 
---
 libavcodec/libvpxenc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Lynne April 5, 2022, 1:55 p.m. UTC | #1 
5 Apr 2022, 14:15 by danilchap-at-google.com@ffmpeg.org:

> ---
>  libavcodec/libvpxenc.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
> index dff1d06b0e..0705863450 100644
> --- a/libavcodec/libvpxenc.c
> +++ b/libavcodec/libvpxenc.c
> @@ -1250,6 +1250,8 @@ static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
>  int ret = ff_get_encode_buffer(avctx, pkt, cx_frame->sz, 0);
>  uint8_t *side_data;
>  int pict_type;
> +    int quality;
> +    VPxContext *ctx = avctx->priv_data;
>  
>  if (ret < 0)
>  return ret;
> @@ -1264,7 +1266,12 @@ static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
>  pict_type = AV_PICTURE_TYPE_P;
>  }
>  
> -    ff_side_data_set_encoder_stats(pkt, 0, cx_frame->sse + 1,
> +    ret = vpx_codec_control(&ctx->encoder, VP8E_GET_LAST_QUANTIZER_64, &quality);
> +    if (ret != VPX_CODEC_OK) {
> +        quality = 0;
> +    }
>

Fix the coding style.
James Almer April 5, 2022, 1:57 p.m. UTC | #2 
On 4/5/2022 10:55 AM, Lynne wrote:
> 5 Apr 2022, 14:15 by danilchap-at-google.com@ffmpeg.org:
> 
>> ---
>>   libavcodec/libvpxenc.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
>> index dff1d06b0e..0705863450 100644
>> --- a/libavcodec/libvpxenc.c
>> +++ b/libavcodec/libvpxenc.c
>> @@ -1250,6 +1250,8 @@ static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
>>   int ret = ff_get_encode_buffer(avctx, pkt, cx_frame->sz, 0);
>>   uint8_t *side_data;
>>   int pict_type;
>> +    int quality;
>> +    VPxContext *ctx = avctx->priv_data;
>>   
>>   if (ret < 0)
>>   return ret;
>> @@ -1264,7 +1266,12 @@ static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
>>   pict_type = AV_PICTURE_TYPE_P;
>>   }
>>   
>> -    ff_side_data_set_encoder_stats(pkt, 0, cx_frame->sse + 1,
>> +    ret = vpx_codec_control(&ctx->encoder, VP8E_GET_LAST_QUANTIZER_64, &quality);
>> +    if (ret != VPX_CODEC_OK) {
>> +        quality = 0;
>> +    }
>>
> 
> Fix the coding style.

You could just say to remove the brackets for one line statements.
James Almer April 5, 2022, 1:59 p.m. UTC | #3 
On 4/5/2022 9:15 AM, Danil Chapovalov wrote:
> ---
>   libavcodec/libvpxenc.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
> index dff1d06b0e..0705863450 100644
> --- a/libavcodec/libvpxenc.c
> +++ b/libavcodec/libvpxenc.c
> @@ -1250,6 +1250,8 @@ static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
>       int ret = ff_get_encode_buffer(avctx, pkt, cx_frame->sz, 0);
>       uint8_t *side_data;
>       int pict_type;
> +    int quality;
> +    VPxContext *ctx = avctx->priv_data;
>   
>       if (ret < 0)
>           return ret;
> @@ -1264,7 +1266,12 @@ static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
>           pict_type = AV_PICTURE_TYPE_P;
>       }
>   
> -    ff_side_data_set_encoder_stats(pkt, 0, cx_frame->sse + 1,
> +    ret = vpx_codec_control(&ctx->encoder, VP8E_GET_LAST_QUANTIZER_64, &quality);
> +    if (ret != VPX_CODEC_OK) {

Is the last argument guaranteed to be untouched when the return value of 
vpx_codec_control() is not VPX_CODEC_OK? If so, you can initialize 
quality to 0 above, and remove this part.

> +        quality = 0;
> +    }
> +
> +    ff_side_data_set_encoder_stats(pkt, quality * FF_QP2LAMBDA, cx_frame->sse + 1,
>                                      cx_frame->have_sse ? 3 : 0, pict_type);
>   
>       if (cx_frame->have_sse) {
Danil Chapovalov April 5, 2022, 2:22 p.m. UTC | #4 
I've rechecked documentation for the "vpx_codec_control" - can't find
any guarantee about the last parameter when the function fails, thus
prefer to be on the safe side.

On Tue, Apr 5, 2022 at 3:59 PM James Almer <jamrial@gmail.com> wrote:
>
> On 4/5/2022 9:15 AM, Danil Chapovalov wrote:
> > ---
> >   libavcodec/libvpxenc.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
> > index dff1d06b0e..0705863450 100644
> > --- a/libavcodec/libvpxenc.c
> > +++ b/libavcodec/libvpxenc.c
> > @@ -1250,6 +1250,8 @@ static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
> >       int ret = ff_get_encode_buffer(avctx, pkt, cx_frame->sz, 0);
> >       uint8_t *side_data;
> >       int pict_type;
> > +    int quality;
> > +    VPxContext *ctx = avctx->priv_data;
> >
> >       if (ret < 0)
> >           return ret;
> > @@ -1264,7 +1266,12 @@ static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
> >           pict_type = AV_PICTURE_TYPE_P;
> >       }
> >
> > -    ff_side_data_set_encoder_stats(pkt, 0, cx_frame->sse + 1,
> > +    ret = vpx_codec_control(&ctx->encoder, VP8E_GET_LAST_QUANTIZER_64, &quality);
> > +    if (ret != VPX_CODEC_OK) {
>
> Is the last argument guaranteed to be untouched when the return value of
> vpx_codec_control() is not VPX_CODEC_OK? If so, you can initialize
> quality to 0 above, and remove this part.
>
> > +        quality = 0;
> > +    }
> > +
> > +    ff_side_data_set_encoder_stats(pkt, quality * FF_QP2LAMBDA, cx_frame->sse + 1,
> >                                      cx_frame->have_sse ? 3 : 0, pict_type);
> >
> >       if (cx_frame->have_sse) {
> _______________________________________________
> 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".
James Zern April 6, 2022, 12:16 a.m. UTC | #5 
On Tue, Apr 5, 2022 at 7:23 AM Danil Chapovalov
<danilchap-at-google.com@ffmpeg.org> wrote:
>
> I've rechecked documentation for the "vpx_codec_control" - can't find
> any guarantee about the last parameter when the function fails, thus
> prefer to be on the safe side.
>

The check is fine, though the function can only fail due to invalid
arguments and won't modify the value in that case.
As an aside, try to avoid top posting on this list:
https://ffmpeg.org/contact.html#MailingLists

> On Tue, Apr 5, 2022 at 3:59 PM James Almer <jamrial@gmail.com> wrote:
> >
> > On 4/5/2022 9:15 AM, Danil Chapovalov wrote:
> > > ---
> > >   libavcodec/libvpxenc.c | 9 ++++++++-
> > >   1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
> > > index dff1d06b0e..0705863450 100644
> > > --- a/libavcodec/libvpxenc.c
> > > +++ b/libavcodec/libvpxenc.c
> > > @@ -1250,6 +1250,8 @@ static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
> > >       int ret = ff_get_encode_buffer(avctx, pkt, cx_frame->sz, 0);
> > >       uint8_t *side_data;
> > >       int pict_type;
> > > +    int quality;
> > > +    VPxContext *ctx = avctx->priv_data;
> > >
> > >       if (ret < 0)
> > >           return ret;
> > > @@ -1264,7 +1266,12 @@ static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
> > >           pict_type = AV_PICTURE_TYPE_P;
> > >       }
> > >
> > > -    ff_side_data_set_encoder_stats(pkt, 0, cx_frame->sse + 1,
> > > +    ret = vpx_codec_control(&ctx->encoder, VP8E_GET_LAST_QUANTIZER_64, &quality);
> > > +    if (ret != VPX_CODEC_OK) {
> >
> > Is the last argument guaranteed to be untouched when the return value of
> > vpx_codec_control() is not VPX_CODEC_OK? If so, you can initialize
> > quality to 0 above, and remove this part.
> >
> > > +        quality = 0;
> > > +    }
> > > +
> > > +    ff_side_data_set_encoder_stats(pkt, quality * FF_QP2LAMBDA, cx_frame->sse + 1,
> > >                                      cx_frame->have_sse ? 3 : 0, pict_type);
> > >
> > >       if (cx_frame->have_sse) {
> > _______________________________________________
> > 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".
> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
index dff1d06b0e..0705863450 100644
--- a/libavcodec/libvpxenc.c
+++ b/libavcodec/libvpxenc.c
@@ -1250,6 +1250,8 @@  static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
     int ret = ff_get_encode_buffer(avctx, pkt, cx_frame->sz, 0);
     uint8_t *side_data;
     int pict_type;
+    int quality;
+    VPxContext *ctx = avctx->priv_data;
 
     if (ret < 0)
         return ret;
@@ -1264,7 +1266,12 @@  static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
         pict_type = AV_PICTURE_TYPE_P;
     }
 
-    ff_side_data_set_encoder_stats(pkt, 0, cx_frame->sse + 1,
+    ret = vpx_codec_control(&ctx->encoder, VP8E_GET_LAST_QUANTIZER_64, &quality);
+    if (ret != VPX_CODEC_OK) {
+        quality = 0;
+    }
+
+    ff_side_data_set_encoder_stats(pkt, quality * FF_QP2LAMBDA, cx_frame->sse + 1,
                                    cx_frame->have_sse ? 3 : 0, pict_type);
 
     if (cx_frame->have_sse) {