| Message ID | 20230927221403.3277953-1-vigneshv@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/svt-av1: Set pic_type only when gop_size == 1 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | fail | Make failed |
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
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
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);
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(-)