[FFmpeg-devel] [PATCH] avcodec/svt-av1: Set force_key_frames only when gop_size == 1

[Ronald S. Bultje]

Hi,

On Wed, Oct 4, 2023 at 5:05 PM Vignesh Venkat <vigneshv at google.com> wrote:

> On Wed, Oct 4, 2023 at 10:23 AM Ronald S. Bultje <rsbultje at gmail.com>
> wrote:
> >
> > Hi,
> >
> > On Tue, Oct 3, 2023 at 6:53 PM Vignesh Venkatasubramanian via
> ffmpeg-devel <ffmpeg-devel at ffmpeg.org> wrote:
> >>
> >> SVT-AV1 does not support requesting keyframes at arbitrary points
> >> by setting pic_type to EB_AV1_KEY_PICTURE. So set force_key_frames
> >> to 1 only when gop_size == 1.
> >>
> >> Please see the comments in
> >> https://gitlab.com/AOMediaCodec/SVT-AV1/-/issues/2076 for a bit more
> >> details.
> >>
> >> Signed-off-by: Vignesh Venkatasubramanian <vigneshv at google.com>
> >> ---
> >>  libavcodec/libsvtav1.c | 9 +++++++--
> >>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> >> index 5015169244..8d2c7f3be4 100644
> >> --- a/libavcodec/libsvtav1.c
> >> +++ b/libavcodec/libsvtav1.c
> >> @@ -253,8 +253,13 @@ static int
> config_enc_params(EbSvtAv1EncConfiguration *param,
> >>      // In order for SVT-AV1 to force keyframes by setting pic_type to
> >>      // EB_AV1_KEY_PICTURE on any frame, force_key_frames has to be
> set. Note
> >>      // that this does not force all frames to be keyframes (it only
> forces a
> >> -    // keyframe with pic_type is set to EB_AV1_KEY_PICTURE).
> >> -    param->force_key_frames = 1;
> >> +    // keyframe with pic_type is set to EB_AV1_KEY_PICTURE). As of
> now, SVT-AV1
> >> +    // does not support arbitrary keyframe requests by setting
> pic_type to
> >> +    // EB_AV1_KEY_PICTURE, so it is done only when gop_size == 1.
> >> +    // FIXME: When SVT-AV1 supports arbitrary keyframe requests, this
> code needs
> >> +    // to be updated to set force_key_frames accordingly.
> >> +    if (avctx->gop_size == 1)
> >> +        param->force_key_frames = 1;
> >>
> >>      if (avctx->framerate.num > 0 && avctx->framerate.den > 0) {
> >>          param->frame_rate_numerator   = avctx->framerate.num;
> >> --
> >> 2.42.0.582.g8ccd20d70d-goog
> >
> >
> > Seems reasonable to me.
> >
> > Can you merge, or should I merge for you?
> >
>
> Thanks Ronald. I don't have commit access. Can you please merge it?
>

Done.

Ronald