diff mbox series

[FFmpeg-devel] Allow to modify max qp configuration parameter in libvpx without reseting the encoder

Message ID 20220314130459.88494-1-danilchap@google.com
State New
Headers show
Series [FFmpeg-devel] Allow to modify max qp configuration parameter in libvpx without reseting the encoder | expand

Checks

Context Check Description
yinshiyou/commit_msg_loongarch64 warning The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ".
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/commit_msg_x86 warning The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ".
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/commit_msg_aarch64_jetson warning The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ".
andriy/make_aarch64_jetson success Make finished
andriy/make_fate_aarch64_jetson success Make fate finished
andriy/commit_msg_armv7_RPi4 warning The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ".
andriy/make_armv7_RPi4 success Make finished
andriy/make_fate_armv7_RPi4 success Make fate finished

Commit Message

Danil Chapovalov March 14, 2022, 1:04 p.m. UTC 
---
 libavcodec/libvpxenc.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Jan Ekström March 14, 2022, 3:27 p.m. UTC | #1 
On Mon, Mar 14, 2022 at 3:05 PM Danil Chapovalov
<danilchap-at-google.com@ffmpeg.org> wrote:
>
> ---

Probably something a la

avcodec/libvpxenc: enable dynamic quantizer reconfiguration

?

>  libavcodec/libvpxenc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
> index 8f94ba15dc..45baeed435 100644
> --- a/libavcodec/libvpxenc.c
> +++ b/libavcodec/libvpxenc.c
> @@ -1658,6 +1658,13 @@ static int vpx_encode(AVCodecContext *avctx, AVPacket *pkt,
>                  flags |= strtoul(en->value, NULL, 10);
>              }
>
> +            en = av_dict_get(frame->metadata, "max-quantizer", NULL, 0);
> +            if (en) {
> +                struct vpx_codec_enc_cfg cfg = *enccfg;
> +                cfg.rc_max_quantizer = strtoul(en->value, NULL, 10);
> +                vpx_codec_enc_config_set(&ctx->encoder, &cfg);
> +            }
> +

There is side data already defined for quantizers, AVVideoEncParams /
AV_FRAME_DATA_VIDEO_ENC_PARAMS .

In other words, this should be handled in a similar manner to ROI, not
as an ad-hoc metadata key in the AVFrame.

Cheers,
Jan
Danil Chapovalov March 16, 2022, 12:13 p.m. UTC | #2 
On Mon, Mar 14, 2022 at 4:28 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Mon, Mar 14, 2022 at 3:05 PM Danil Chapovalov
> <danilchap-at-google.com@ffmpeg.org> wrote:
> >
> > ---
>
> Probably something a la
>
> avcodec/libvpxenc: enable dynamic quantizer reconfiguration
>
> ?

Thank you, resubmitted with new title

>
> >  libavcodec/libvpxenc.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
> > index 8f94ba15dc..45baeed435 100644
> > --- a/libavcodec/libvpxenc.c
> > +++ b/libavcodec/libvpxenc.c
> > @@ -1658,6 +1658,13 @@ static int vpx_encode(AVCodecContext *avctx, AVPacket *pkt,
> >                  flags |= strtoul(en->value, NULL, 10);
> >              }
> >
> > +            en = av_dict_get(frame->metadata, "max-quantizer", NULL, 0);
> > +            if (en) {
> > +                struct vpx_codec_enc_cfg cfg = *enccfg;
> > +                cfg.rc_max_quantizer = strtoul(en->value, NULL, 10);
> > +                vpx_codec_enc_config_set(&ctx->encoder, &cfg);
> > +            }
> > +
>
> There is side data already defined for quantizers, AVVideoEncParams /
> AV_FRAME_DATA_VIDEO_ENC_PARAMS .
>
> In other words, this should be handled in a similar manner to ROI, not
> as an ad-hoc metadata key in the AVFrame.

I've checked struct AVVideoEncParams, it doesn't look fitting: it
contains exact qp (plus qp per plane), while my patch suggests
changing max-qp limit for current and following frames.
AVVideoEncParams also has some extra fields that I'm unsure how to
handle (they are unrelated to what I'm trying to do),
I haven't found any other struct that would contain something like max qp.
What is ROI? What code can I use as an example of your suggestion?


>
> Cheers,
> Jan
> _______________________________________________
> 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".
Lynne March 16, 2022, 12:37 p.m. UTC | #3 
16 Mar 2022, 13:13 by danilchap-at-google.com@ffmpeg.org:

> On Mon, Mar 14, 2022 at 4:28 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
>>
>> On Mon, Mar 14, 2022 at 3:05 PM Danil Chapovalov
>> <danilchap-at-google.com@ffmpeg.org> wrote:
>> >
>> > ---
>>
>> Probably something a la
>>
>> avcodec/libvpxenc: enable dynamic quantizer reconfiguration
>>
>> ?
>>
>
> Thank you, resubmitted with new title
>
>>
>> >  libavcodec/libvpxenc.c | 7 +++++++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
>> > index 8f94ba15dc..45baeed435 100644
>> > --- a/libavcodec/libvpxenc.c
>> > +++ b/libavcodec/libvpxenc.c
>> > @@ -1658,6 +1658,13 @@ static int vpx_encode(AVCodecContext *avctx, AVPacket *pkt,
>> >                  flags |= strtoul(en->value, NULL, 10);
>> >              }
>> >
>> > +            en = av_dict_get(frame->metadata, "max-quantizer", NULL, 0);
>> > +            if (en) {
>> > +                struct vpx_codec_enc_cfg cfg = *enccfg;
>> > +                cfg.rc_max_quantizer = strtoul(en->value, NULL, 10);
>> > +                vpx_codec_enc_config_set(&ctx->encoder, &cfg);
>> > +            }
>> > +
>>
>> There is side data already defined for quantizers, AVVideoEncParams /
>> AV_FRAME_DATA_VIDEO_ENC_PARAMS .
>>
>> In other words, this should be handled in a similar manner to ROI, not
>> as an ad-hoc metadata key in the AVFrame.
>>
>
> I've checked struct AVVideoEncParams, it doesn't look fitting: it
> contains exact qp (plus qp per plane), while my patch suggests
> changing max-qp limit for current and following frames.
> AVVideoEncParams also has some extra fields that I'm unsure how to
> handle (they are unrelated to what I'm trying to do),
> I haven't found any other struct that would contain something like max qp.
> What is ROI? What code can I use as an example of your suggestion?
>

If the encoder is operating under bitrate mode, then I think it would
be fine to interpret the qp field as a max qp field.
IIRC Youtube run libvpx and libaom under constant bitrate mode and
adjust the max quantizer per frame, because no one can fix the
rate control system of either library.
Anton Khirnov March 19, 2022, 2:25 p.m. UTC | #4 
Quoting Lynne (2022-03-16 13:37:55)
> 16 Mar 2022, 13:13 by danilchap-at-google.com@ffmpeg.org:
> 
> > On Mon, Mar 14, 2022 at 4:28 PM Jan Ekström <jeebjp@gmail.com> wrote:
> >
> >>
> >> On Mon, Mar 14, 2022 at 3:05 PM Danil Chapovalov
> >> <danilchap-at-google.com@ffmpeg.org> wrote:
> >> >
> >> > ---
> >>
> >> Probably something a la
> >>
> >> avcodec/libvpxenc: enable dynamic quantizer reconfiguration
> >>
> >> ?
> >>
> >
> > Thank you, resubmitted with new title
> >
> >>
> >> >  libavcodec/libvpxenc.c | 7 +++++++
> >> >  1 file changed, 7 insertions(+)
> >> >
> >> > diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
> >> > index 8f94ba15dc..45baeed435 100644
> >> > --- a/libavcodec/libvpxenc.c
> >> > +++ b/libavcodec/libvpxenc.c
> >> > @@ -1658,6 +1658,13 @@ static int vpx_encode(AVCodecContext *avctx, AVPacket *pkt,
> >> >                  flags |= strtoul(en->value, NULL, 10);
> >> >              }
> >> >
> >> > +            en = av_dict_get(frame->metadata, "max-quantizer", NULL, 0);
> >> > +            if (en) {
> >> > +                struct vpx_codec_enc_cfg cfg = *enccfg;
> >> > +                cfg.rc_max_quantizer = strtoul(en->value, NULL, 10);
> >> > +                vpx_codec_enc_config_set(&ctx->encoder, &cfg);
> >> > +            }
> >> > +
> >>
> >> There is side data already defined for quantizers, AVVideoEncParams /
> >> AV_FRAME_DATA_VIDEO_ENC_PARAMS .
> >>
> >> In other words, this should be handled in a similar manner to ROI, not
> >> as an ad-hoc metadata key in the AVFrame.
> >>
> >
> > I've checked struct AVVideoEncParams, it doesn't look fitting: it
> > contains exact qp (plus qp per plane), while my patch suggests
> > changing max-qp limit for current and following frames.
> > AVVideoEncParams also has some extra fields that I'm unsure how to
> > handle (they are unrelated to what I'm trying to do),
> > I haven't found any other struct that would contain something like max qp.
> > What is ROI? What code can I use as an example of your suggestion?
> >
> 
> If the encoder is operating under bitrate mode, then I think it would
> be fine to interpret the qp field as a max qp field.
> IIRC Youtube run libvpx and libaom under constant bitrate mode and
> adjust the max quantizer per frame, because no one can fix the
> rate control system of either library.

Why not just update AVCodecContext.qmax?
diff mbox series

Patch

diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
index 8f94ba15dc..45baeed435 100644
--- a/libavcodec/libvpxenc.c
+++ b/libavcodec/libvpxenc.c
@@ -1658,6 +1658,13 @@  static int vpx_encode(AVCodecContext *avctx, AVPacket *pkt,
                 flags |= strtoul(en->value, NULL, 10);
             }
 
+            en = av_dict_get(frame->metadata, "max-quantizer", NULL, 0);
+            if (en) {
+                struct vpx_codec_enc_cfg cfg = *enccfg;
+                cfg.rc_max_quantizer = strtoul(en->value, NULL, 10);
+                vpx_codec_enc_config_set(&ctx->encoder, &cfg);
+            }
+
             memset(&layer_id, 0, sizeof(layer_id));
 
             en = av_dict_get(frame->metadata, "temporal_id", NULL, 0);