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

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

[Vignesh Venkatasubramanian via ffmpeg-devel]

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.

Signed-off-by: Vignesh Venkatasubramanian <vigneshv@google.com>
---
 libavcodec/libsvtav1.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
index 5015169244..3247c30f60 100644
--- a/libavcodec/libsvtav1.c
+++ b/libavcodec/libsvtav1.c
@@ -253,8 +253,11 @@ 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.
+    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;
@@ -462,19 +465,8 @@ static int eb_send_frame(AVCodecContext *avctx, const AVFrame *frame)
     headerPtr->flags         = 0;
     headerPtr->p_app_private = NULL;
     headerPtr->pts           = frame->pts;
-
-    switch (frame->pict_type) {
-    case AV_PICTURE_TYPE_I:
-        headerPtr->pic_type = EB_AV1_KEY_PICTURE;
-        break;
-    default:
-        // Actually means auto, or default.
-        headerPtr->pic_type = EB_AV1_INVALID_PICTURE;
-        break;
-    }
-
-    if (avctx->gop_size == 1)
-        headerPtr->pic_type = EB_AV1_KEY_PICTURE;
+    // EB_AV1_INVALID_PICTURE means auto, or default.
+    headerPtr->pic_type = (avctx->gop_size == 1) ? EB_AV1_KEY_PICTURE : EB_AV1_INVALID_PICTURE;
 
     svt_av1_enc_send_picture(svt_enc->svt_handle, headerPtr);
 
-- 
2.42.0.515.g380fc7ccd1-goog

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

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

[Ronald S. Bultje]

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?

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.)

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

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

[Vignesh Venkat via ffmpeg-devel]

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

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

[Vignesh Venkatasubramanian via ffmpeg-devel]

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@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

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

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

[Ronald S. Bultje]

Hi,

On Tue, Oct 3, 2023 at 6:53 PM 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. 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@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?

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

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

[Vignesh Venkat via ffmpeg-devel]

On Wed, Oct 4, 2023 at 10:23 AM Ronald S. Bultje <rsbultje@gmail.com> wrote:
>
> Hi,
>
> On Tue, Oct 3, 2023 at 6:53 PM 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. 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@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?

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

Re: [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@google.com> wrote:

> On Wed, Oct 4, 2023 at 10:23 AM Ronald S. Bultje <rsbultje@gmail.com>
> wrote:
> >
> > Hi,
> >
> > On Tue, Oct 3, 2023 at 6:53 PM 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. 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@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
_______________________________________________
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".