Re: [FFmpeg-devel] [PATCH] avcodec/svt-av1: Set pic_type only when gop_size == 1 - Vignesh Venkat via ffmpeg-devel

From: Vignesh Venkat via ffmpeg-devel <ffmpeg-devel@ffmpeg.org>
To: "Ronald S. Bultje" <rsbultje@gmail.com>
Cc: Vignesh Venkat <vigneshv@google.com>,
	FFmpeg development discussions and patches
	<ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH] avcodec/svt-av1: Set pic_type only when gop_size == 1
Date: Tue, 3 Oct 2023 15:51:29 -0700
Message-ID: <CAOJaEPLXu4hHMpVoVt1PD69QA=8=U2RmPHGqcD4M6cZ_KBhotQ@mail.gmail.com> (raw)
In-Reply-To: <CAEEMt2=6fCC_UARWn5-O8Xxc7aGVp947tS1_RYSjK7epxwx8Fw@mail.gmail.com>

On Thu, Sep 28, 2023 at 2:05 AM Ronald S. Bultje <rsbultje@gmail.com> wrote:
>
> Hi Vignesh,
>
> On Thu, Sep 28, 2023 at 12:14 AM Vignesh Venkatasubramanian via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote:
>>
>> SVT-AV1 does not support requesting keyframes at arbitrary points
>> by setting pic_type to EB_AV1_KEY_PICTURE.
>>
>> This patch changes the following:
>>  * Set pic_type to EB_AV1_KEY_PICTURE only when gop_size == 1. This
>>    only has an effect in this case (combined with force_key_frames).
>>    In all other cases, setting this has no effect.
>>  * Set force_key_frames to 1 only when gop_size == 1, this is
>>    needed for pic_type request above to work.
>>
>> Please see the comments in
>> https://gitlab.com/AOMediaCodec/SVT-AV1/-/issues/2076 for a bit more
>> details.
>
>
> Right. So, if I put my archeologist hat on, is it fair to say that what probably happened is that force_key_frames used to not exist, and pic_type worked as per above code. Then force_key_frames was required (because of quality implications), breaking the above code. And now we're removing the broken code because otherwise there's a quality penalty. Is that fair?
>

Hi Ronald, Yes, i believe your assessment is correct.

> I agree it's probably unfair to ask you to fix the broken use case that was previously possible (I suppose this comes down to exposing an option to set force_key_frames by having the user indicate s/he'll be using pic_type). However, maybe we should not remove the dead code, because it's functionally OK. Maybe we even need a FIXME above the line where we set force_key_frames only if gop_size==1, explaining this is a bug. WDYT?
>
> (It feels a bit weird to remove it because it's broken, is what I'm trying to say, especially because we know how to fix it.)
>

Makes sense, i have kept the old code in the pic_type setting part and
have updated this patch to only set force_key_frames when gop_size ==
1.

PTAL.

> Ronald

-- 
Vignesh
_______________________________________________
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".