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

[Vignesh Venkat]

On Thu, Sep 28, 2023 at 2:05 AM Ronald S. Bultje <rsbultje at gmail.com> wrote:
>
> Hi Vignesh,
>
> On Thu, Sep 28, 2023 at 12:14 AM 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.
>>
>> 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