diff mbox series

[FFmpeg-devel] Add enable_keyframe_filtering option for libaom-av1 encoder.

Message ID 20201027203815.131648-1-bohanli@google.com
State New
Headers show
Series [FFmpeg-devel] Add enable_keyframe_filtering option for libaom-av1 encoder.
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Bohan Li Oct. 27, 2020, 8:38 p.m. UTC
Add the option to use -enable-keyframe-filtering with libaom-av1
codec.  The option controls the encoder behavior on performing
temporal filtering on keyframes.

Signed-off-by: Bohan Li <bohanli@google.com>
---
 doc/encoders.texi      | 17 +++++++++++++++++
 libavcodec/libaomenc.c |  5 +++++
 2 files changed, 22 insertions(+)

Comments

Jan Ekström Oct. 27, 2020, 9:18 p.m. UTC | #1
On Tue, Oct 27, 2020 at 10:38 PM Bohan Li
<bohanli-at-google.com@ffmpeg.org> wrote:
>
> Add the option to use -enable-keyframe-filtering with libaom-av1
> codec.  The option controls the encoder behavior on performing
> temporal filtering on keyframes.
>

Hi,

Looking at the amount of options etc which you want to expose to
FFmpeg users (or just utilize yourselves via FFmpeg), and by the
feeling that we're duplicating a lot of option parsing that aomenc CLI
is already doing, I wonder if it would be more useful for libaom to
provide a key=value API? This then could be exposed to FFmpeg users
with an option similar to {x264,x265,rav1e}-params.

This way each time libaom adds new options, they don't explicitly need
to be added to the wrapper. They can just be utilized.

Best regards,
Jan
Bohan Li Oct. 27, 2020, 11:13 p.m. UTC | #2
Hi Jan,

Thanks for the suggestion! I believe that is a good idea. I am not super
familiar with this part, though, so will probably need to propose this to
the Aomedia issue tracker.
Meanwhile I think it is also useful to expose useful options (like this
one, enable-keyframe-filtering, which is discussed quite a few times in the
community as far as I know) to the users as well. So I think we should
still apply this patch, and if once the mentioned API is working, we could
maybe re-consider which ones are more needed.

Best,
Bohan

On Tue, Oct 27, 2020 at 2:19 PM Jan Ekström <jeebjp@gmail.com> wrote:

> On Tue, Oct 27, 2020 at 10:38 PM Bohan Li
> <bohanli-at-google.com@ffmpeg.org> wrote:
> >
> > Add the option to use -enable-keyframe-filtering with libaom-av1
> > codec.  The option controls the encoder behavior on performing
> > temporal filtering on keyframes.
> >
>
> Hi,
>
> Looking at the amount of options etc which you want to expose to
> FFmpeg users (or just utilize yourselves via FFmpeg), and by the
> feeling that we're duplicating a lot of option parsing that aomenc CLI
> is already doing, I wonder if it would be more useful for libaom to
> provide a key=value API? This then could be exposed to FFmpeg users
> with an option similar to {x264,x265,rav1e}-params.
>
> This way each time libaom adds new options, they don't explicitly need
> to be added to the wrapper. They can just be utilized.
>
> Best regards,
> 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".
Bohan Li Oct. 30, 2020, 9:43 p.m. UTC | #3
Gentle ping :)

Bohan

On Tue, Oct 27, 2020 at 4:13 PM Bohan Li <bohanli@google.com> wrote:

> Hi Jan,
>
> Thanks for the suggestion! I believe that is a good idea. I am not super
> familiar with this part, though, so will probably need to propose this to
> the Aomedia issue tracker.
> Meanwhile I think it is also useful to expose useful options (like this
> one, enable-keyframe-filtering, which is discussed quite a few times in the
> community as far as I know) to the users as well. So I think we should
> still apply this patch, and if once the mentioned API is working, we could
> maybe re-consider which ones are more needed.
>
> Best,
> Bohan
>
> On Tue, Oct 27, 2020 at 2:19 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
>> On Tue, Oct 27, 2020 at 10:38 PM Bohan Li
>> <bohanli-at-google.com@ffmpeg.org> wrote:
>> >
>> > Add the option to use -enable-keyframe-filtering with libaom-av1
>> > codec.  The option controls the encoder behavior on performing
>> > temporal filtering on keyframes.
>> >
>>
>> Hi,
>>
>> Looking at the amount of options etc which you want to expose to
>> FFmpeg users (or just utilize yourselves via FFmpeg), and by the
>> feeling that we're duplicating a lot of option parsing that aomenc CLI
>> is already doing, I wonder if it would be more useful for libaom to
>> provide a key=value API? This then could be exposed to FFmpeg users
>> with an option similar to {x264,x265,rav1e}-params.
>>
>> This way each time libaom adds new options, they don't explicitly need
>> to be added to the wrapper. They can just be utilized.
>>
>> Best regards,
>> 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".
>
>
Bohan Li Nov. 5, 2020, 6:29 p.m. UTC | #4
Another ping :)

Bohan

On Fri, Oct 30, 2020 at 2:43 PM Bohan Li <bohanli@google.com> wrote:

> Gentle ping :)
>
> Bohan
>
> On Tue, Oct 27, 2020 at 4:13 PM Bohan Li <bohanli@google.com> wrote:
>
>> Hi Jan,
>>
>> Thanks for the suggestion! I believe that is a good idea. I am not super
>> familiar with this part, though, so will probably need to propose this to
>> the Aomedia issue tracker.
>> Meanwhile I think it is also useful to expose useful options (like this
>> one, enable-keyframe-filtering, which is discussed quite a few times in the
>> community as far as I know) to the users as well. So I think we should
>> still apply this patch, and if once the mentioned API is working, we could
>> maybe re-consider which ones are more needed.
>>
>> Best,
>> Bohan
>>
>> On Tue, Oct 27, 2020 at 2:19 PM Jan Ekström <jeebjp@gmail.com> wrote:
>>
>>> On Tue, Oct 27, 2020 at 10:38 PM Bohan Li
>>> <bohanli-at-google.com@ffmpeg.org> wrote:
>>> >
>>> > Add the option to use -enable-keyframe-filtering with libaom-av1
>>> > codec.  The option controls the encoder behavior on performing
>>> > temporal filtering on keyframes.
>>> >
>>>
>>> Hi,
>>>
>>> Looking at the amount of options etc which you want to expose to
>>> FFmpeg users (or just utilize yourselves via FFmpeg), and by the
>>> feeling that we're duplicating a lot of option parsing that aomenc CLI
>>> is already doing, I wonder if it would be more useful for libaom to
>>> provide a key=value API? This then could be exposed to FFmpeg users
>>> with an option similar to {x264,x265,rav1e}-params.
>>>
>>> This way each time libaom adds new options, they don't explicitly need
>>> to be added to the wrapper. They can just be utilized.
>>>
>>> Best regards,
>>> 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".
>>
>>
Steven Liu Nov. 6, 2020, 2:33 a.m. UTC | #5
> 2020年11月6日 上午2:29,Bohan Li <bohanli-at-google.com@ffmpeg.org> 写道:
> 
> Another ping :)
I think maybe you should fix JEEB’s review comment, then will get responses comments.
Not only apply a middle version.
> 
> Bohan
> 
> On Fri, Oct 30, 2020 at 2:43 PM Bohan Li <bohanli@google.com> wrote:
> 
>> Gentle ping :)
>> 
>> Bohan
>> 
>> On Tue, Oct 27, 2020 at 4:13 PM Bohan Li <bohanli@google.com> wrote:
>> 
>>> Hi Jan,
>>> 
>>> Thanks for the suggestion! I believe that is a good idea. I am not super
>>> familiar with this part, though, so will probably need to propose this to
>>> the Aomedia issue tracker.
>>> Meanwhile I think it is also useful to expose useful options (like this
>>> one, enable-keyframe-filtering, which is discussed quite a few times in the
>>> community as far as I know) to the users as well. So I think we should
>>> still apply this patch, and if once the mentioned API is working, we could
>>> maybe re-consider which ones are more needed.
>>> 
>>> Best,
>>> Bohan
>>> 
>>> On Tue, Oct 27, 2020 at 2:19 PM Jan Ekström <jeebjp@gmail.com> wrote:
>>> 
>>>> On Tue, Oct 27, 2020 at 10:38 PM Bohan Li
>>>> <bohanli-at-google.com@ffmpeg.org> wrote:
>>>>> 
>>>>> Add the option to use -enable-keyframe-filtering with libaom-av1
>>>>> codec.  The option controls the encoder behavior on performing
>>>>> temporal filtering on keyframes.
>>>>> 
>>>> 
>>>> Hi,
>>>> 
>>>> Looking at the amount of options etc which you want to expose to
>>>> FFmpeg users (or just utilize yourselves via FFmpeg), and by the
>>>> feeling that we're duplicating a lot of option parsing that aomenc CLI
>>>> is already doing, I wonder if it would be more useful for libaom to
>>>> provide a key=value API? This then could be exposed to FFmpeg users
>>>> with an option similar to {x264,x265,rav1e}-params.
>>>> 
>>>> This way each time libaom adds new options, they don't explicitly need
>>>> to be added to the wrapper. They can just be utilized.
>>>> 
>>>> Best regards,
>>>> 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".
>>> 
>>> 
> _______________________________________________
> 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".

Thanks

Steven Liu
Bohan Li Nov. 6, 2020, 8:42 a.m. UTC | #6
Thanks for the reply, Steven!

Regarding JEEB’s comment, the suggestion was to add an api to the *libaom*
library, not to ffmpeg. I do agree with the rationale, but when such an api
would be available for ffmpeg to use is quite uncertain. In the meanwhile,
I believe it is reasonable to add in this patch to expose the option to the
users. I don’t think this is a “middle version”, since as mentioned, this
option could be quite useful in certain scenarios and IMHO should be added
in.

Please let me know if there are any concerns on this. Thanks!

Best,
Bohan

On Thu, Nov 5, 2020 at 6:35 PM Steven Liu <lq@chinaffmpeg.org> wrote:

>
>
> > 2020年11月6日 上午2:29,Bohan Li <bohanli-at-google.com@ffmpeg.org> 写道:
> >
> > Another ping :)
> I think maybe you should fix JEEB’s review comment, then will get
> responses comments.
> Not only apply a middle version.
> >
> > Bohan
> >
> > On Fri, Oct 30, 2020 at 2:43 PM Bohan Li <bohanli@google.com> wrote:
> >
> >> Gentle ping :)
> >>
> >> Bohan
> >>
> >> On Tue, Oct 27, 2020 at 4:13 PM Bohan Li <bohanli@google.com> wrote:
> >>
> >>> Hi Jan,
> >>>
> >>> Thanks for the suggestion! I believe that is a good idea. I am not
> super
> >>> familiar with this part, though, so will probably need to propose this
> to
> >>> the Aomedia issue tracker.
> >>> Meanwhile I think it is also useful to expose useful options (like this
> >>> one, enable-keyframe-filtering, which is discussed quite a few times
> in the
> >>> community as far as I know) to the users as well. So I think we should
> >>> still apply this patch, and if once the mentioned API is working, we
> could
> >>> maybe re-consider which ones are more needed.
> >>>
> >>> Best,
> >>> Bohan
> >>>
> >>> On Tue, Oct 27, 2020 at 2:19 PM Jan Ekström <jeebjp@gmail.com> wrote:
> >>>
> >>>> On Tue, Oct 27, 2020 at 10:38 PM Bohan Li
> >>>> <bohanli-at-google.com@ffmpeg.org> wrote:
> >>>>>
> >>>>> Add the option to use -enable-keyframe-filtering with libaom-av1
> >>>>> codec.  The option controls the encoder behavior on performing
> >>>>> temporal filtering on keyframes.
> >>>>>
> >>>>
> >>>> Hi,
> >>>>
> >>>> Looking at the amount of options etc which you want to expose to
> >>>> FFmpeg users (or just utilize yourselves via FFmpeg), and by the
> >>>> feeling that we're duplicating a lot of option parsing that aomenc CLI
> >>>> is already doing, I wonder if it would be more useful for libaom to
> >>>> provide a key=value API? This then could be exposed to FFmpeg users
> >>>> with an option similar to {x264,x265,rav1e}-params.
> >>>>
> >>>> This way each time libaom adds new options, they don't explicitly need
> >>>> to be added to the wrapper. They can just be utilized.
> >>>>
> >>>> Best regards,
> >>>> 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".
> >>>
> >>>
> > _______________________________________________
> > 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".
>
> Thanks
>
> Steven Liu
>
> _______________________________________________
> 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".
Steven Liu Nov. 6, 2020, 9:45 a.m. UTC | #7
> 2020年11月6日 下午4:42,Bohan Li <bohanli@google.com> 写道:
> 
> Thanks for the reply, Steven! 
> 
> Regarding JEEB’s comment, the suggestion was to add an api to the *libaom* library, not to ffmpeg. I do agree with the rationale, but when such an api would be available for ffmpeg to use is quite uncertain. In the meanwhile, I believe it is reasonable to add in this patch to expose the option to the users. I don’t think this is a “middle version”, since as mentioned, this option could be quite useful in certain scenarios and IMHO should be added in. 
What about use av1_param arguments? As x264opts, x265opts?
> 
> Please let me know if there are any concerns on this. Thanks!
> 
> Best,
> Bohan
> 
> On Thu, Nov 5, 2020 at 6:35 PM Steven Liu <lq@chinaffmpeg.org> wrote:
> 
> 
> > 2020年11月6日 上午2:29,Bohan Li <bohanli-at-google.com@ffmpeg.org> 写道:
> > 
> > Another ping :)
> I think maybe you should fix JEEB’s review comment, then will get responses comments.
> Not only apply a middle version.
> > 
> > Bohan
> > 
> > On Fri, Oct 30, 2020 at 2:43 PM Bohan Li <bohanli@google.com> wrote:
> > 
> >> Gentle ping :)
> >> 
> >> Bohan
> >> 
> >> On Tue, Oct 27, 2020 at 4:13 PM Bohan Li <bohanli@google.com> wrote:
> >> 
> >>> Hi Jan,
> >>> 
> >>> Thanks for the suggestion! I believe that is a good idea. I am not super
> >>> familiar with this part, though, so will probably need to propose this to
> >>> the Aomedia issue tracker.
> >>> Meanwhile I think it is also useful to expose useful options (like this
> >>> one, enable-keyframe-filtering, which is discussed quite a few times in the
> >>> community as far as I know) to the users as well. So I think we should
> >>> still apply this patch, and if once the mentioned API is working, we could
> >>> maybe re-consider which ones are more needed.
> >>> 
> >>> Best,
> >>> Bohan
> >>> 
> >>> On Tue, Oct 27, 2020 at 2:19 PM Jan Ekström <jeebjp@gmail.com> wrote:
> >>> 
> >>>> On Tue, Oct 27, 2020 at 10:38 PM Bohan Li
> >>>> <bohanli-at-google.com@ffmpeg.org> wrote:
> >>>>> 
> >>>>> Add the option to use -enable-keyframe-filtering with libaom-av1
> >>>>> codec.  The option controls the encoder behavior on performing
> >>>>> temporal filtering on keyframes.
> >>>>> 
> >>>> 
> >>>> Hi,
> >>>> 
> >>>> Looking at the amount of options etc which you want to expose to
> >>>> FFmpeg users (or just utilize yourselves via FFmpeg), and by the
> >>>> feeling that we're duplicating a lot of option parsing that aomenc CLI
> >>>> is already doing, I wonder if it would be more useful for libaom to
> >>>> provide a key=value API? This then could be exposed to FFmpeg users
> >>>> with an option similar to {x264,x265,rav1e}-params.
> >>>> 
> >>>> This way each time libaom adds new options, they don't explicitly need
> >>>> to be added to the wrapper. They can just be utilized.
> >>>> 
> >>>> Best regards,
> >>>> 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".
> >>> 
> >>> 
> > _______________________________________________
> > 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".
> 
> Thanks
> 
> Steven Liu
> 
> _______________________________________________
> 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".

Thanks

Steven Liu
Jan Ekström Nov. 6, 2020, 11:10 a.m. UTC | #8
On Fri, Nov 6, 2020 at 11:46 AM Steven Liu <lq@chinaffmpeg.org> wrote:
>
>
>
> > 2020年11月6日 下午4:42,Bohan Li <bohanli@google.com> 写道:
> >
> > Thanks for the reply, Steven!
> >
> > Regarding JEEB’s comment, the suggestion was to add an api to the *libaom* library, not to ffmpeg. I do agree with the rationale, but when such an api would be available for ffmpeg to use is quite uncertain. In the meanwhile, I believe it is reasonable to add in this patch to expose the option to the users. I don’t think this is a “middle version”, since as mentioned, this option could be quite useful in certain scenarios and IMHO should be added in.
> What about use av1_param arguments? As x264opts, x265opts?

The problem with this is that libaom as far as I can tell doesn't have
an API like that. Thus each and every API user that needs to have
options defined as strings will have to *duplicate* those mappings
(which already are defined in the aomenc command line application, but
not exposed through the API).

This has been the butt of jokes and reason for lamentations on IRC for
quite a while, but unfortunately nobody actually seems to have voiced
an opinion on this on the mailing list until now?

Thus there generally speaking are only two ways forward for this:
1. This is really spammy and unfortunate (and getting these removed
will be a PITA after we actually do get a key & value based API), but
we take the additional option in.
2. This is apparently a low usage option and the amount of AVOptions
in this encoder is getting higher and higher, thus we deny the patch
until a key, value based API is provided.

Personally I lean somewhat towards 2, but if the option is
needed/useful (use cases can be provided), then begrudgingly I could
go for 1.

So far the lack of comments has mostly been regarding people not
having high enough stakes/care regarding this module, methinks?

Jan
Bohan Li Nov. 6, 2020, 5:31 p.m. UTC | #9
Thanks a lot for the comment, Jan and Steven! Yes I very much agree that
the number of options is quite large and this is not a great path to go
for. But for the moment I still suggest that we apply this patch, while
proposing to libaom for a key & value api.

This option gives users the option to control the keyframe filtering
method, which is quite essential to both objective and subjective quality.
When turned on (default) the objective quality/compression efficiency is
greatly improved, but at the cost of potential subjective quality loss at
keyframes (because they are filtered and can appear blurry). Therefore
users who do not want to have such artifacts may want to disable the
option. This is actually a long-standing trade-off that libaom users have
to (sometimes painfully) choose, and they cannot do so with ffmpeg right
now. Also recently a new experimental option (=2) was added to libaom in
order to mitigate the subjective loss. It would be nice to expose this
option for people to be able to test it using ffmpeg with various decoders
too. I've observed a few related discussions in the AV1 community (e.g. the
reddit thread on the experimental option:
https://www.reddit.com/r/AV1/comments/ic7ktu/aom_fixed_its_filtered_reference_frame_issues/
).

That said, I do understand the rationale and the reason why people might be
opposed to adding another option, though I still suggest that we add the
option for now, and when the API is available, we can make decisions and
deprecate certain ones that are not necessary. Please let me know your
thoughts on this, meanwhile I'll file a feature request to the aomedia bug
tracker for the API.

Thank you again for the comments!
Best,
Bohan

On Fri, Nov 6, 2020 at 3:16 AM Jan Ekström <jeebjp@gmail.com> wrote:

> On Fri, Nov 6, 2020 at 11:46 AM Steven Liu <lq@chinaffmpeg.org> wrote:
> >
> >
> >
> > > 2020年11月6日 下午4:42,Bohan Li <bohanli@google.com> 写道:
> > >
> > > Thanks for the reply, Steven!
> > >
> > > Regarding JEEB’s comment, the suggestion was to add an api to the
> *libaom* library, not to ffmpeg. I do agree with the rationale, but when
> such an api would be available for ffmpeg to use is quite uncertain. In the
> meanwhile, I believe it is reasonable to add in this patch to expose the
> option to the users. I don’t think this is a “middle version”, since as
> mentioned, this option could be quite useful in certain scenarios and IMHO
> should be added in.
> > What about use av1_param arguments? As x264opts, x265opts?
>
> The problem with this is that libaom as far as I can tell doesn't have
> an API like that. Thus each and every API user that needs to have
> options defined as strings will have to *duplicate* those mappings
> (which already are defined in the aomenc command line application, but
> not exposed through the API).
>
> This has been the butt of jokes and reason for lamentations on IRC for
> quite a while, but unfortunately nobody actually seems to have voiced
> an opinion on this on the mailing list until now?
>
> Thus there generally speaking are only two ways forward for this:
> 1. This is really spammy and unfortunate (and getting these removed
> will be a PITA after we actually do get a key & value based API), but
> we take the additional option in.
> 2. This is apparently a low usage option and the amount of AVOptions
> in this encoder is getting higher and higher, thus we deny the patch
> until a key, value based API is provided.
>
> Personally I lean somewhat towards 2, but if the option is
> needed/useful (use cases can be provided), then begrudgingly I could
> go for 1.
>
> So far the lack of comments has mostly been regarding people not
> having high enough stakes/care regarding this module, methinks?
>
> 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".
Bohan Li Nov. 9, 2020, 7 p.m. UTC | #10
I have added a feature request in the libaom issue tracker for the key &
value api, although it may take some time for the api to land. Meanwhile I
still suggest that we apply this patch to ffmpeg since as mentioned this
option can be quite important for subjective quality control.
Please let me know if there are any concerns.

Thanks!
Bohan

On Fri, Nov 6, 2020 at 9:31 AM Bohan Li <bohanli@google.com> wrote:

> Thanks a lot for the comment, Jan and Steven! Yes I very much agree that
> the number of options is quite large and this is not a great path to go
> for. But for the moment I still suggest that we apply this patch, while
> proposing to libaom for a key & value api.
>
> This option gives users the option to control the keyframe filtering
> method, which is quite essential to both objective and subjective quality.
> When turned on (default) the objective quality/compression efficiency is
> greatly improved, but at the cost of potential subjective quality loss at
> keyframes (because they are filtered and can appear blurry). Therefore
> users who do not want to have such artifacts may want to disable the
> option. This is actually a long-standing trade-off that libaom users have
> to (sometimes painfully) choose, and they cannot do so with ffmpeg right
> now. Also recently a new experimental option (=2) was added to libaom in
> order to mitigate the subjective loss. It would be nice to expose this
> option for people to be able to test it using ffmpeg with various decoders
> too. I've observed a few related discussions in the AV1 community (e.g. the
> reddit thread on the experimental option:
> https://www.reddit.com/r/AV1/comments/ic7ktu/aom_fixed_its_filtered_reference_frame_issues/
> ).
>
> That said, I do understand the rationale and the reason why people might
> be opposed to adding another option, though I still suggest that we add the
> option for now, and when the API is available, we can make decisions and
> deprecate certain ones that are not necessary. Please let me know your
> thoughts on this, meanwhile I'll file a feature request to the aomedia bug
> tracker for the API.
>
> Thank you again for the comments!
> Best,
> Bohan
>
> On Fri, Nov 6, 2020 at 3:16 AM Jan Ekström <jeebjp@gmail.com> wrote:
>
>> On Fri, Nov 6, 2020 at 11:46 AM Steven Liu <lq@chinaffmpeg.org> wrote:
>> >
>> >
>> >
>> > > 2020年11月6日 下午4:42,Bohan Li <bohanli@google.com> 写道:
>> > >
>> > > Thanks for the reply, Steven!
>> > >
>> > > Regarding JEEB’s comment, the suggestion was to add an api to the
>> *libaom* library, not to ffmpeg. I do agree with the rationale, but when
>> such an api would be available for ffmpeg to use is quite uncertain. In the
>> meanwhile, I believe it is reasonable to add in this patch to expose the
>> option to the users. I don’t think this is a “middle version”, since as
>> mentioned, this option could be quite useful in certain scenarios and IMHO
>> should be added in.
>> > What about use av1_param arguments? As x264opts, x265opts?
>>
>> The problem with this is that libaom as far as I can tell doesn't have
>> an API like that. Thus each and every API user that needs to have
>> options defined as strings will have to *duplicate* those mappings
>> (which already are defined in the aomenc command line application, but
>> not exposed through the API).
>>
>> This has been the butt of jokes and reason for lamentations on IRC for
>> quite a while, but unfortunately nobody actually seems to have voiced
>> an opinion on this on the mailing list until now?
>>
>> Thus there generally speaking are only two ways forward for this:
>> 1. This is really spammy and unfortunate (and getting these removed
>> will be a PITA after we actually do get a key & value based API), but
>> we take the additional option in.
>> 2. This is apparently a low usage option and the amount of AVOptions
>> in this encoder is getting higher and higher, thus we deny the patch
>> until a key, value based API is provided.
>>
>> Personally I lean somewhat towards 2, but if the option is
>> needed/useful (use cases can be provided), then begrudgingly I could
>> go for 1.
>>
>> So far the lack of comments has mostly been regarding people not
>> having high enough stakes/care regarding this module, methinks?
>>
>> 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".
>
>
Bohan Li Nov. 16, 2020, 3:38 p.m. UTC | #11
Gentle ping :)

On Mon, Nov 9, 2020 at 11:00 AM Bohan Li <bohanli@google.com> wrote:

> I have added a feature request in the libaom issue tracker for the key &
> value api, although it may take some time for the api to land. Meanwhile I
> still suggest that we apply this patch to ffmpeg since as mentioned this
> option can be quite important for subjective quality control.
> Please let me know if there are any concerns.
>
> Thanks!
> Bohan
>
> On Fri, Nov 6, 2020 at 9:31 AM Bohan Li <bohanli@google.com> wrote:
>
>> Thanks a lot for the comment, Jan and Steven! Yes I very much agree that
>> the number of options is quite large and this is not a great path to go
>> for. But for the moment I still suggest that we apply this patch, while
>> proposing to libaom for a key & value api.
>>
>> This option gives users the option to control the keyframe filtering
>> method, which is quite essential to both objective and subjective quality.
>> When turned on (default) the objective quality/compression efficiency is
>> greatly improved, but at the cost of potential subjective quality loss at
>> keyframes (because they are filtered and can appear blurry). Therefore
>> users who do not want to have such artifacts may want to disable the
>> option. This is actually a long-standing trade-off that libaom users have
>> to (sometimes painfully) choose, and they cannot do so with ffmpeg right
>> now. Also recently a new experimental option (=2) was added to libaom in
>> order to mitigate the subjective loss. It would be nice to expose this
>> option for people to be able to test it using ffmpeg with various decoders
>> too. I've observed a few related discussions in the AV1 community (e.g. the
>> reddit thread on the experimental option:
>> https://www.reddit.com/r/AV1/comments/ic7ktu/aom_fixed_its_filtered_reference_frame_issues/
>> ).
>>
>> That said, I do understand the rationale and the reason why people might
>> be opposed to adding another option, though I still suggest that we add the
>> option for now, and when the API is available, we can make decisions and
>> deprecate certain ones that are not necessary. Please let me know your
>> thoughts on this, meanwhile I'll file a feature request to the aomedia bug
>> tracker for the API.
>>
>> Thank you again for the comments!
>> Best,
>> Bohan
>>
>> On Fri, Nov 6, 2020 at 3:16 AM Jan Ekström <jeebjp@gmail.com> wrote:
>>
>>> On Fri, Nov 6, 2020 at 11:46 AM Steven Liu <lq@chinaffmpeg.org> wrote:
>>> >
>>> >
>>> >
>>> > > 2020年11月6日 下午4:42,Bohan Li <bohanli@google.com> 写道:
>>> > >
>>> > > Thanks for the reply, Steven!
>>> > >
>>> > > Regarding JEEB’s comment, the suggestion was to add an api to the
>>> *libaom* library, not to ffmpeg. I do agree with the rationale, but when
>>> such an api would be available for ffmpeg to use is quite uncertain. In the
>>> meanwhile, I believe it is reasonable to add in this patch to expose the
>>> option to the users. I don’t think this is a “middle version”, since as
>>> mentioned, this option could be quite useful in certain scenarios and IMHO
>>> should be added in.
>>> > What about use av1_param arguments? As x264opts, x265opts?
>>>
>>> The problem with this is that libaom as far as I can tell doesn't have
>>> an API like that. Thus each and every API user that needs to have
>>> options defined as strings will have to *duplicate* those mappings
>>> (which already are defined in the aomenc command line application, but
>>> not exposed through the API).
>>>
>>> This has been the butt of jokes and reason for lamentations on IRC for
>>> quite a while, but unfortunately nobody actually seems to have voiced
>>> an opinion on this on the mailing list until now?
>>>
>>> Thus there generally speaking are only two ways forward for this:
>>> 1. This is really spammy and unfortunate (and getting these removed
>>> will be a PITA after we actually do get a key & value based API), but
>>> we take the additional option in.
>>> 2. This is apparently a low usage option and the amount of AVOptions
>>> in this encoder is getting higher and higher, thus we deny the patch
>>> until a key, value based API is provided.
>>>
>>> Personally I lean somewhat towards 2, but if the option is
>>> needed/useful (use cases can be provided), then begrudgingly I could
>>> go for 1.
>>>
>>> So far the lack of comments has mostly been regarding people not
>>> having high enough stakes/care regarding this module, methinks?
>>>
>>> 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".
>>
>>
diff mbox series

Patch

diff --git a/doc/encoders.texi b/doc/encoders.texi
index 0b1c69e982..c80a520a20 100644
--- a/doc/encoders.texi
+++ b/doc/encoders.texi
@@ -1685,6 +1685,23 @@  Enable interintra compound. Default is true.
 @item enable-smooth-interintra (@emph{boolean}) (Requires libaom >= v2.0.0)
 Enable smooth interintra mode. Default is true.
 
+@item enable-keyframe-filtering (Requires libaom >= v2.0.0)
+Filtering type for key frames. Temporal filtering of key frames improves
+compression efficiency, but may introduce key frames that are blurred at times.
+Adding an overlay to filtered frames mitigates the blurring issue, but could
+potentially confuse video players when performing random access.
+Possible values:
+@table @samp
+@item @emph{-1}
+Use the default in libaom (default).
+@item @emph{0}
+Do not filter key frames. 
+@item @emph{1}
+Filter key frames but do not apply overlays. 
+@item @emph{2}
+Filter key frames and apply overlays to them (experimental).
+@end table
+
 @end table
 
 @section libsvtav1
diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
index 2b0581b15a..77c25770a4 100644
--- a/libavcodec/libaomenc.c
+++ b/libavcodec/libaomenc.c
@@ -124,6 +124,7 @@  typedef struct AOMEncoderContext {
     int enable_diff_wtd_comp;
     int enable_dist_wtd_comp;
     int enable_dual_filter;
+    int enable_keyframe_filtering;
 } AOMContext;
 
 static const char *const ctlidstr[] = {
@@ -192,6 +193,7 @@  static const char *const ctlidstr[] = {
     [AV1E_SET_REDUCED_REFERENCE_SET]    = "AV1E_SET_REDUCED_REFERENCE_SET",
     [AV1E_SET_ENABLE_SMOOTH_INTERINTRA] = "AV1E_SET_ENABLE_SMOOTH_INTERINTRA",
     [AV1E_SET_ENABLE_REF_FRAME_MVS]     = "AV1E_SET_ENABLE_REF_FRAME_MVS",
+    [AV1E_SET_ENABLE_KEYFRAME_FILTERING] = "AV1E_SET_ENABLE_KEYFRAME_FILTERING"
 #endif
 };
 
@@ -812,6 +814,8 @@  static av_cold int aom_init(AVCodecContext *avctx,
         codecctl_int(avctx, AV1E_SET_ENABLE_ONESIDED_COMP, ctx->enable_onesided_comp);
     if (ctx->enable_smooth_interintra >= 0)
         codecctl_int(avctx, AV1E_SET_ENABLE_SMOOTH_INTERINTRA, ctx->enable_smooth_interintra);
+    if (ctx->enable_keyframe_filtering >= 0)
+        codecctl_int(avctx, AV1E_SET_ENABLE_KEYFRAME_FILTERING, ctx->enable_keyframe_filtering);
 #endif
 
     codecctl_int(avctx, AOME_SET_STATIC_THRESHOLD, ctx->static_thresh);
@@ -1261,6 +1265,7 @@  static const AVOption options[] = {
     { "enable-masked-comp",           "Enable masked compound",                            OFFSET(enable_masked_comp),           AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
     { "enable-interintra-comp",       "Enable interintra compound",                        OFFSET(enable_interintra_comp),       AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
     { "enable-smooth-interintra",     "Enable smooth interintra mode",                     OFFSET(enable_smooth_interintra),     AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
+    { "enable-keyframe-filtering",    "Keyframe filtering type",                           OFFSET(enable_keyframe_filtering),    AV_OPT_TYPE_INT,  {.i64 = -1}, -1, 3, VE},
     { NULL },
 };