diff mbox series

[FFmpeg-devel,v2,6/6] avcodec/libsvtav1: support constant quality mode

Message ID 1631928405-26935-6-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/5] avcodec/libsvtav1: Fix redundant setting of caps_internal | expand

Checks

Context Check Description
andriy/configurex86 warning Failed to apply patch
andriy/configureppc warning Failed to apply patch

Commit Message

Lance Wang Sept. 18, 2021, 1:26 a.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 doc/encoders.texi      | 10 ++++++++--
 libavcodec/libsvtav1.c | 10 +++++++++-
 libavcodec/version.h   |  2 +-
 3 files changed, 18 insertions(+), 4 deletions(-)

Comments

Christopher Degawa Sept. 19, 2021, 6:02 p.m. UTC | #1 
On Fri, Sep 17, 2021 at 9:28 PM <lance.lmwang@gmail.com> wrote:

> From: Limin Wang <lance.lmwang@gmail.com>
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>
As a note, I personally favor this patch over mine since I don't think
`enable_tpl_la` is worth exposing to the user if its main role is to switch
between crf and cqp
Christopher Degawa Sept. 23, 2021, 1:45 p.m. UTC | #2 
On Sun, Sep 19, 2021 at 2:02 PM Christopher Degawa <ccom@randomderp.com>
wrote:

>
>
> On Fri, Sep 17, 2021 at 9:28 PM <lance.lmwang@gmail.com> wrote:
>
>> From: Limin Wang <lance.lmwang@gmail.com>
>>
>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>>
> As a note, I personally favor this patch over mine since I don't think
> `enable_tpl_la` is worth exposing to the user if its main role is to switch
> between crf and cqp
>

Ping on this patch, this has been requested on SVT-AV1's side as well
Jan Ekström Sept. 23, 2021, 2:11 p.m. UTC | #3 
On Thu, Sep 23, 2021 at 4:46 PM Christopher Degawa <ccom@randomderp.com> wrote:
>
> On Sun, Sep 19, 2021 at 2:02 PM Christopher Degawa <ccom@randomderp.com>
> wrote:
>
> >
> >
> > On Fri, Sep 17, 2021 at 9:28 PM <lance.lmwang@gmail.com> wrote:
> >
> >> From: Limin Wang <lance.lmwang@gmail.com>
> >>
> >> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >>
> > As a note, I personally favor this patch over mine since I don't think
> > `enable_tpl_la` is worth exposing to the user if its main role is to switch
> > between crf and cqp
> >
>
> Ping on this patch, this has been requested on SVT-AV1's side as well

I think my further comments on the previous version were mostly to
move off from the rc option for rate control, which has become more
and more seemingly unnecessary (and different from most other encoders
for no obvious reason).

The only option that with a quick look doesn't match just setting
quantizer|bit rate|CRF is "cvbr", but it doesn't seem to be impossible
to either rework "rc" for it, or to utilize a separate option for it.

Basically the idea is to get it more or less to match other encoders,
so that the wrapper doesn't feel completely alien to the rest of
things.

Jan
Lance Wang Sept. 23, 2021, 3:08 p.m. UTC | #4 
On Thu, Sep 23, 2021 at 05:11:49PM +0300, Jan Ekström wrote:
> On Thu, Sep 23, 2021 at 4:46 PM Christopher Degawa <ccom@randomderp.com> wrote:
> >
> > On Sun, Sep 19, 2021 at 2:02 PM Christopher Degawa <ccom@randomderp.com>
> > wrote:
> >
> > >
> > >
> > > On Fri, Sep 17, 2021 at 9:28 PM <lance.lmwang@gmail.com> wrote:
> > >
> > >> From: Limin Wang <lance.lmwang@gmail.com>
> > >>
> > >> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > >>
> > > As a note, I personally favor this patch over mine since I don't think
> > > `enable_tpl_la` is worth exposing to the user if its main role is to switch
> > > between crf and cqp
> > >
> >
> > Ping on this patch, this has been requested on SVT-AV1's side as well
> 
> I think my further comments on the previous version were mostly to
> move off from the rc option for rate control, which has become more
> and more seemingly unnecessary (and different from most other encoders
> for no obvious reason).
> 
> The only option that with a quick look doesn't match just setting
> quantizer|bit rate|CRF is "cvbr", but it doesn't seem to be impossible
> to either rework "rc" for it, or to utilize a separate option for it.

Do you think it's OK to remove the rc option? to match other encoders, 
We can choose the rate control by the related parameters like:

if (crf >= 0)
    choose crf rate control;
else if (bitrate > 0)
    choose cvbr rate control;
else if (cqp >=0 )
    choose cqp rate control;

> 
> Basically the idea is to get it more or less to match other encoders,
> so that the wrapper doesn't feel completely alien to the rest of
> things.
> 
> 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".
Lance Wang Sept. 25, 2021, 1:06 a.m. UTC | #5 
On Thu, Sep 23, 2021 at 09:45:48AM -0400, Christopher Degawa wrote:
> On Sun, Sep 19, 2021 at 2:02 PM Christopher Degawa <ccom@randomderp.com>
> wrote:
> 
> >
> >
> > On Fri, Sep 17, 2021 at 9:28 PM <lance.lmwang@gmail.com> wrote:
> >
> >> From: Limin Wang <lance.lmwang@gmail.com>
> >>
> >> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >>
> > As a note, I personally favor this patch over mine since I don't think
> > `enable_tpl_la` is worth exposing to the user if its main role is to switch
> > between crf and cqp
> >
> 
> Ping on this patch, this has been requested on SVT-AV1's side as well

For Jan haven't further comments, I Will apply the patchset tomorrow unless 
there are objections.
Jan Ekström Sept. 25, 2021, 9:51 a.m. UTC | #6 
On Sat, Sep 25, 2021 at 4:06 AM <lance.lmwang@gmail.com> wrote:
>
> On Thu, Sep 23, 2021 at 09:45:48AM -0400, Christopher Degawa wrote:
> > On Sun, Sep 19, 2021 at 2:02 PM Christopher Degawa <ccom@randomderp.com>
> > wrote:
> >
> > >
> > >
> > > On Fri, Sep 17, 2021 at 9:28 PM <lance.lmwang@gmail.com> wrote:
> > >
> > >> From: Limin Wang <lance.lmwang@gmail.com>
> > >>
> > >> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > >>
> > > As a note, I personally favor this patch over mine since I don't think
> > > `enable_tpl_la` is worth exposing to the user if its main role is to switch
> > > between crf and cqp
> > >
> >
> > Ping on this patch, this has been requested on SVT-AV1's side as well
>
> For Jan haven't further comments, I Will apply the patchset tomorrow unless
> there are objections.

There are probably patches in this set which can be applied (like the
open/closed GOP thing, IIRC), but please do not apply the whole set.

It's kind of important we don't make this wrapper even more of a mess
than it is right now.

Jan
Jan Ekström Sept. 25, 2021, 12:35 p.m. UTC | #7 
On Thu, Sep 23, 2021 at 6:08 PM <lance.lmwang@gmail.com> wrote:
>
> On Thu, Sep 23, 2021 at 05:11:49PM +0300, Jan Ekström wrote:
> > On Thu, Sep 23, 2021 at 4:46 PM Christopher Degawa <ccom@randomderp.com> wrote:
> > >
> > > On Sun, Sep 19, 2021 at 2:02 PM Christopher Degawa <ccom@randomderp.com>
> > > wrote:
> > >
> > > >
> > > >
> > > > On Fri, Sep 17, 2021 at 9:28 PM <lance.lmwang@gmail.com> wrote:
> > > >
> > > >> From: Limin Wang <lance.lmwang@gmail.com>
> > > >>
> > > >> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > >>
> > > > As a note, I personally favor this patch over mine since I don't think
> > > > `enable_tpl_la` is worth exposing to the user if its main role is to switch
> > > > between crf and cqp
> > > >
> > >
> > > Ping on this patch, this has been requested on SVT-AV1's side as well
> >
> > I think my further comments on the previous version were mostly to
> > move off from the rc option for rate control, which has become more
> > and more seemingly unnecessary (and different from most other encoders
> > for no obvious reason).
> >
> > The only option that with a quick look doesn't match just setting
> > quantizer|bit rate|CRF is "cvbr", but it doesn't seem to be impossible
> > to either rework "rc" for it, or to utilize a separate option for it.
>
> Do you think it's OK to remove the rc option? to match other encoders,
> We can choose the rate control by the related parameters like:
>
> if (crf >= 0)
>     choose crf rate control;
> else if (bitrate > 0)
>     choose cvbr rate control;
> else if (cqp >=0 )
>     choose cqp rate control;
>

In general that is the idea, yes (except bit rate would mean either
vbr or cvbr). It would be nice to follow whatever is the default on
SVT-AV1's side by default, and then if the user specifies a rate
control mode that is not the default, his selection is honored. Not
sure what the best way for that is to be honest. Possibly the style of
setting AVCodecDefault a la libx265?

For the rc option, given how much SVT-AV1 itself is in flux, I would
be OK with removing the option, or making it an option that decides
between cvbr and vbr (essentially making it "VBR bit rate control
mode"). I wish cvbr would instead be something like VBV/HRD elsewhere,
so we could just utilize maxrate&bufsize for controlling it, instead
of having it as another discrete rate control mode.

As these things change, I also hope that SVT-AV1 will soon get a
key=value style of option API, that way the wrapper will not have to
be updated constantly according to the changes in the library :) . As
I feel SVT-AV1 will be changed quite a bit in the future (due to its
recent age).

Jan
Lance Wang Sept. 25, 2021, 1:35 p.m. UTC | #8 
On Sat, Sep 25, 2021 at 03:35:53PM +0300, Jan Ekström wrote:
> On Thu, Sep 23, 2021 at 6:08 PM <lance.lmwang@gmail.com> wrote:
> >
> > On Thu, Sep 23, 2021 at 05:11:49PM +0300, Jan Ekström wrote:
> > > On Thu, Sep 23, 2021 at 4:46 PM Christopher Degawa <ccom@randomderp.com> wrote:
> > > >
> > > > On Sun, Sep 19, 2021 at 2:02 PM Christopher Degawa <ccom@randomderp.com>
> > > > wrote:
> > > >
> > > > >
> > > > >
> > > > > On Fri, Sep 17, 2021 at 9:28 PM <lance.lmwang@gmail.com> wrote:
> > > > >
> > > > >> From: Limin Wang <lance.lmwang@gmail.com>
> > > > >>
> > > > >> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > >>
> > > > > As a note, I personally favor this patch over mine since I don't think
> > > > > `enable_tpl_la` is worth exposing to the user if its main role is to switch
> > > > > between crf and cqp
> > > > >
> > > >
> > > > Ping on this patch, this has been requested on SVT-AV1's side as well
> > >
> > > I think my further comments on the previous version were mostly to
> > > move off from the rc option for rate control, which has become more
> > > and more seemingly unnecessary (and different from most other encoders
> > > for no obvious reason).
> > >
> > > The only option that with a quick look doesn't match just setting
> > > quantizer|bit rate|CRF is "cvbr", but it doesn't seem to be impossible
> > > to either rework "rc" for it, or to utilize a separate option for it.
> >
> > Do you think it's OK to remove the rc option? to match other encoders,
> > We can choose the rate control by the related parameters like:
> >
> > if (crf >= 0)
> >     choose crf rate control;
> > else if (bitrate > 0)
> >     choose cvbr rate control;
> > else if (cqp >=0 )
> >     choose cqp rate control;
> >
> 
> In general that is the idea, yes (except bit rate would mean either
> vbr or cvbr). It would be nice to follow whatever is the default on
> SVT-AV1's side by default, and then if the user specifies a rate
> control mode that is not the default, his selection is honored. Not
> sure what the best way for that is to be honest. Possibly the style of
> setting AVCodecDefault a la libx265?
> 
> For the rc option, given how much SVT-AV1 itself is in flux, I would
> be OK with removing the option, or making it an option that decides
> between cvbr and vbr (essentially making it "VBR bit rate control
> mode"). I wish cvbr would instead be something like VBV/HRD elsewhere,
> so we could just utilize maxrate&bufsize for controlling it, instead
> of having it as another discrete rate control mode.

Yes, I think it's preferable to use utilize maxrate&bufsize to control
cbr and vbr as most of encoder. Then I'll follow this direction to update
the patch.

> 
> As these things change, I also hope that SVT-AV1 will soon get a
> key=value style of option API, that way the wrapper will not have to
> be updated constantly according to the changes in the library :) . As
> I feel SVT-AV1 will be changed quite a bit in the future (due to its
> recent age).
>

Yes, it's good to add svtav1-opts so that we can utilize the update options
directly. 

> 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".
Jan Ekström Sept. 25, 2021, 1:51 p.m. UTC | #9 
On Sat, Sep 25, 2021 at 4:35 PM <lance.lmwang@gmail.com> wrote:
>
> On Sat, Sep 25, 2021 at 03:35:53PM +0300, Jan Ekström wrote:
> > On Thu, Sep 23, 2021 at 6:08 PM <lance.lmwang@gmail.com> wrote:
> > >
> > > On Thu, Sep 23, 2021 at 05:11:49PM +0300, Jan Ekström wrote:
> > > > On Thu, Sep 23, 2021 at 4:46 PM Christopher Degawa <ccom@randomderp.com> wrote:
> > > > >
> > > > > On Sun, Sep 19, 2021 at 2:02 PM Christopher Degawa <ccom@randomderp.com>
> > > > > wrote:
> > > > >
> > > > > >
> > > > > >
> > > > > > On Fri, Sep 17, 2021 at 9:28 PM <lance.lmwang@gmail.com> wrote:
> > > > > >
> > > > > >> From: Limin Wang <lance.lmwang@gmail.com>
> > > > > >>
> > > > > >> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > > >>
> > > > > > As a note, I personally favor this patch over mine since I don't think
> > > > > > `enable_tpl_la` is worth exposing to the user if its main role is to switch
> > > > > > between crf and cqp
> > > > > >
> > > > >
> > > > > Ping on this patch, this has been requested on SVT-AV1's side as well
> > > >
> > > > I think my further comments on the previous version were mostly to
> > > > move off from the rc option for rate control, which has become more
> > > > and more seemingly unnecessary (and different from most other encoders
> > > > for no obvious reason).
> > > >
> > > > The only option that with a quick look doesn't match just setting
> > > > quantizer|bit rate|CRF is "cvbr", but it doesn't seem to be impossible
> > > > to either rework "rc" for it, or to utilize a separate option for it.
> > >
> > > Do you think it's OK to remove the rc option? to match other encoders,
> > > We can choose the rate control by the related parameters like:
> > >
> > > if (crf >= 0)
> > >     choose crf rate control;
> > > else if (bitrate > 0)
> > >     choose cvbr rate control;
> > > else if (cqp >=0 )
> > >     choose cqp rate control;
> > >
> >
> > In general that is the idea, yes (except bit rate would mean either
> > vbr or cvbr). It would be nice to follow whatever is the default on
> > SVT-AV1's side by default, and then if the user specifies a rate
> > control mode that is not the default, his selection is honored. Not
> > sure what the best way for that is to be honest. Possibly the style of
> > setting AVCodecDefault a la libx265?
> >
> > For the rc option, given how much SVT-AV1 itself is in flux, I would
> > be OK with removing the option, or making it an option that decides
> > between cvbr and vbr (essentially making it "VBR bit rate control
> > mode"). I wish cvbr would instead be something like VBV/HRD elsewhere,
> > so we could just utilize maxrate&bufsize for controlling it, instead
> > of having it as another discrete rate control mode.
>
> Yes, I think it's preferable to use utilize maxrate&bufsize to control
> cbr and vbr as most of encoder. Then I'll follow this direction to update
> the patch.
>

Unfortunately that was just wishful thinking :) .

Unless you can set the buffer size and max rate in SVT-AV1, of course.
With the limited looking I've done CVBR just uses the bit rate for
each GOP instead of average over the whole clip.

> >
> > As these things change, I also hope that SVT-AV1 will soon get a
> > key=value style of option API, that way the wrapper will not have to
> > be updated constantly according to the changes in the library :) . As
> > I feel SVT-AV1 will be changed quite a bit in the future (due to its
> > recent age).
> >
>
> Yes, it's good to add svtav1-opts so that we can utilize the update options
> directly.
>

Let's go with -params ;) Since that is what modules seem to have
standardized on (x264|x265|rav1e|aom) :)

But that is anyways in the future, since SVT-AV1 doesn't have such an
API yet (as far as I know).

Jan
Lance Wang Sept. 25, 2021, 2:20 p.m. UTC | #10 
On Sat, Sep 25, 2021 at 04:51:58PM +0300, Jan Ekström wrote:
> On Sat, Sep 25, 2021 at 4:35 PM <lance.lmwang@gmail.com> wrote:
> >
> > On Sat, Sep 25, 2021 at 03:35:53PM +0300, Jan Ekström wrote:
> > > On Thu, Sep 23, 2021 at 6:08 PM <lance.lmwang@gmail.com> wrote:
> > > >
> > > > On Thu, Sep 23, 2021 at 05:11:49PM +0300, Jan Ekström wrote:
> > > > > On Thu, Sep 23, 2021 at 4:46 PM Christopher Degawa <ccom@randomderp.com> wrote:
> > > > > >
> > > > > > On Sun, Sep 19, 2021 at 2:02 PM Christopher Degawa <ccom@randomderp.com>
> > > > > > wrote:
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On Fri, Sep 17, 2021 at 9:28 PM <lance.lmwang@gmail.com> wrote:
> > > > > > >
> > > > > > >> From: Limin Wang <lance.lmwang@gmail.com>
> > > > > > >>
> > > > > > >> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > > > >>
> > > > > > > As a note, I personally favor this patch over mine since I don't think
> > > > > > > `enable_tpl_la` is worth exposing to the user if its main role is to switch
> > > > > > > between crf and cqp
> > > > > > >
> > > > > >
> > > > > > Ping on this patch, this has been requested on SVT-AV1's side as well
> > > > >
> > > > > I think my further comments on the previous version were mostly to
> > > > > move off from the rc option for rate control, which has become more
> > > > > and more seemingly unnecessary (and different from most other encoders
> > > > > for no obvious reason).
> > > > >
> > > > > The only option that with a quick look doesn't match just setting
> > > > > quantizer|bit rate|CRF is "cvbr", but it doesn't seem to be impossible
> > > > > to either rework "rc" for it, or to utilize a separate option for it.
> > > >
> > > > Do you think it's OK to remove the rc option? to match other encoders,
> > > > We can choose the rate control by the related parameters like:
> > > >
> > > > if (crf >= 0)
> > > >     choose crf rate control;
> > > > else if (bitrate > 0)
> > > >     choose cvbr rate control;
> > > > else if (cqp >=0 )
> > > >     choose cqp rate control;
> > > >
> > >
> > > In general that is the idea, yes (except bit rate would mean either
> > > vbr or cvbr). It would be nice to follow whatever is the default on
> > > SVT-AV1's side by default, and then if the user specifies a rate
> > > control mode that is not the default, his selection is honored. Not
> > > sure what the best way for that is to be honest. Possibly the style of
> > > setting AVCodecDefault a la libx265?
> > >
> > > For the rc option, given how much SVT-AV1 itself is in flux, I would
> > > be OK with removing the option, or making it an option that decides
> > > between cvbr and vbr (essentially making it "VBR bit rate control
> > > mode"). I wish cvbr would instead be something like VBV/HRD elsewhere,
> > > so we could just utilize maxrate&bufsize for controlling it, instead
> > > of having it as another discrete rate control mode.
> >
> > Yes, I think it's preferable to use utilize maxrate&bufsize to control
> > cbr and vbr as most of encoder. Then I'll follow this direction to update
> > the patch.
> >
> 
> Unfortunately that was just wishful thinking :) .
> 
> Unless you can set the buffer size and max rate in SVT-AV1, of course.
> With the limited looking I've done CVBR just uses the bit rate for
> each GOP instead of average over the whole clip.

As it's not very clear yet, so I'll not update this patch anymore.

> 
> > >
> > > As these things change, I also hope that SVT-AV1 will soon get a
> > > key=value style of option API, that way the wrapper will not have to
> > > be updated constantly according to the changes in the library :) . As
> > > I feel SVT-AV1 will be changed quite a bit in the future (due to its
> > > recent age).
> > >
> >
> > Yes, it's good to add svtav1-opts so that we can utilize the update options
> > directly.
> >
> 
> Let's go with -params ;) Since that is what modules seem to have
> standardized on (x264|x265|rav1e|aom) :)
> 
> But that is anyways in the future, since SVT-AV1 doesn't have such an
> API yet (as far as I know).

I think I have tried to add new options half year again, there are willing to
hold and let to wait for the params API.

> 
> 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".
Jun Zhao Sept. 26, 2021, 11:43 a.m. UTC | #11 
On Sat, Sep 25, 2021 at 8:36 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Thu, Sep 23, 2021 at 6:08 PM <lance.lmwang@gmail.com> wrote:
> >
> > On Thu, Sep 23, 2021 at 05:11:49PM +0300, Jan Ekström wrote:
> > > On Thu, Sep 23, 2021 at 4:46 PM Christopher Degawa <ccom@randomderp.com> wrote:
> > > >
> > > > On Sun, Sep 19, 2021 at 2:02 PM Christopher Degawa <ccom@randomderp.com>
> > > > wrote:
> > > >
> > > > >
> > > > >
> > > > > On Fri, Sep 17, 2021 at 9:28 PM <lance.lmwang@gmail.com> wrote:
> > > > >
> > > > >> From: Limin Wang <lance.lmwang@gmail.com>
> > > > >>
> > > > >> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > >>
> > > > > As a note, I personally favor this patch over mine since I don't think
> > > > > `enable_tpl_la` is worth exposing to the user if its main role is to switch
> > > > > between crf and cqp
> > > > >
> > > >
> > > > Ping on this patch, this has been requested on SVT-AV1's side as well
> > >
> > > I think my further comments on the previous version were mostly to
> > > move off from the rc option for rate control, which has become more
> > > and more seemingly unnecessary (and different from most other encoders
> > > for no obvious reason).
> > >
> > > The only option that with a quick look doesn't match just setting
> > > quantizer|bit rate|CRF is "cvbr", but it doesn't seem to be impossible
> > > to either rework "rc" for it, or to utilize a separate option for it.
> >
> > Do you think it's OK to remove the rc option? to match other encoders,
> > We can choose the rate control by the related parameters like:
> >
> > if (crf >= 0)
> >     choose crf rate control;
> > else if (bitrate > 0)
> >     choose cvbr rate control;
> > else if (cqp >=0 )
> >     choose cqp rate control;
> >
>
> In general that is the idea, yes (except bit rate would mean either
> vbr or cvbr). It would be nice to follow whatever is the default on
> SVT-AV1's side by default, and then if the user specifies a rate
> control mode that is not the default, his selection is honored. Not
> sure what the best way for that is to be honest. Possibly the style of
> setting AVCodecDefault a la libx265?
>
> For the rc option, given how much SVT-AV1 itself is in flux, I would
> be OK with removing the option, or making it an option that decides
> between cvbr and vbr (essentially making it "VBR bit rate control
> mode"). I wish cvbr would instead be something like VBV/HRD elsewhere,
> so we could just utilize maxrate&bufsize for controlling it, instead
> of having it as another discrete rate control mode.
>
> As these things change, I also hope that SVT-AV1 will soon get a
> key=value style of option API, that way the wrapper will not have to
I agree with this part , now FFmpeg SVT-AV1 wrapper needs to update
with the SVT-AV1's structural mapping if want to enable a new feature
> be updated constantly according to the changes in the library :) . As
> I feel SVT-AV1 will be changed quite a bit in the future (due to its
> recent age).
>
diff mbox series

Patch

diff --git a/doc/encoders.texi b/doc/encoders.texi
index 64d604e..e7f61ff 100644
--- a/doc/encoders.texi
+++ b/doc/encoders.texi
@@ -1807,8 +1807,11 @@  Set the rate control mode to use.
 Possible modes:
 @table @option
 @item cqp
-Constant quantizer: use fixed values of qindex (dependent on the frame type)
-throughout the stream.  This mode is the default.
+Constant quantizer(crf: -1): use fixed values of qindex (dependent on the frame type)
+throughout the stream.
+
+Constant quality(crf > 0): maintain a constant QP throughout the stream. This mode is
+the default.
 
 @item vbr
 Variable bitrate: use a target bitrate for the whole stream.
@@ -1826,6 +1829,9 @@  Set the minimum quantizer to use when using a bitrate mode.
 @item qp
 Set the quantizer used in cqp rate control mode (0-63).
 
+@item crf
+Select the quality for constant quality mode (0-63).
+
 @item sc_detection
 Enable scene change detection.
 
diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
index 509d92d..f982bb6 100644
--- a/libavcodec/libsvtav1.c
+++ b/libavcodec/libsvtav1.c
@@ -66,6 +66,7 @@  typedef struct SvtContext {
     int rc_mode;
     int scd;
     int qp;
+    int crf;
 
     int tier;
 
@@ -210,6 +211,10 @@  static int config_enc_params(EbSvtAv1EncConfiguration *param,
         param->min_qp_allowed       = avctx->qmin;
     } else {
         param->enable_tpl_la = 0; /* CQP need turn off enable_tp_la */
+        if ( svt_enc->crf > 0) {
+            param->qp = svt_enc->crf;
+            param->enable_tpl_la = 1;
+        }
     }
 
     /* 2 = IDR, closed GOP, 1 = CRA, open GOP */
@@ -523,13 +528,16 @@  static const AVOption options[] = {
 
     { "rc", "Bit rate control mode", OFFSET(rc_mode),
       AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 2, VE , "rc"},
-        { "cqp", "Constant quantizer", 0, AV_OPT_TYPE_CONST, { .i64 = 0 },  INT_MIN, INT_MAX, VE, "rc" },
+        { "cqp", "Constant quantizer(crf: -1), Constant quality(crf > 0)", 0, AV_OPT_TYPE_CONST, { .i64 = 0 },  INT_MIN, INT_MAX, VE, "rc" },
         { "vbr", "Variable Bit Rate, use a target bitrate for the entire stream", 0, AV_OPT_TYPE_CONST, { .i64 = 1 },  INT_MIN, INT_MAX, VE, "rc" },
         { "cvbr", "Constrained Variable Bit Rate, use a target bitrate for each GOP", 0, AV_OPT_TYPE_CONST,{ .i64 = 2 },  INT_MIN, INT_MAX, VE, "rc" },
 
     { "qp", "Quantizer to use with cqp rate control mode", OFFSET(qp),
       AV_OPT_TYPE_INT, { .i64 = 50 }, 0, 63, VE },
 
+    { "crf", "Select the quality for constant quality mode, set to -1 for cqp rate control mode", OFFSET(crf),
+      AV_OPT_TYPE_INT, { .i64 = 50 }, -1, 63, VE },
+
     { "sc_detection", "Scene change detection", OFFSET(scd),
       AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE },
 
diff --git a/libavcodec/version.h b/libavcodec/version.h
index 4b4fe54..b0a741b 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -29,7 +29,7 @@ 
 
 #define LIBAVCODEC_VERSION_MAJOR  59
 #define LIBAVCODEC_VERSION_MINOR   7
-#define LIBAVCODEC_VERSION_MICRO 103
+#define LIBAVCODEC_VERSION_MICRO 104
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
                                                LIBAVCODEC_VERSION_MINOR, \