| Message ID | 20230626174715.2213497-1-vigneshv@google.com |
|---|---|
| State | Accepted |
| Commit | 7bcc1b4eb8534fce66e53d0fc2d66a899bbad8a2 |
| Headers | show |
| Series | [FFmpeg-devel] libsvtav1: Add workaround for gop_size == 1 | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Hi, On Mon, Jun 26, 2023 at 1:47 PM Vignesh Venkatasubramanian < vigneshv-at-google.com@ffmpeg.org> wrote: > In some versions of libsvtav1, setting intra_period_length to 0 > does not produce the intended result (i.e.) all frames produced > are not keyframes. > > Instead handle the gop_size == 1 as a special case by setting > the pic_type to EB_AV1_KEY_PICTURE when encoding each frame so > that all the output frames are keyframes. > > SVT-AV1 Bug: https://gitlab.com/AOMediaCodec/SVT-AV1/-/issues/2076 > > Example command: ffmpeg -f lavfi -i testsrc=duration=1:size=64x64:rate=30 > -c:v libsvtav1 -g 1 -y test.webm > > Before: Only first frame is keyframe. > After: All frames are keyframes. > > Signed-off-by: Vignesh Venkatasubramanian <vigneshv@google.com> > --- > libavcodec/libsvtav1.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > It feels a bit dirty to add workarounds in ffmpeg library wrappers for bugs in these libraries. Not 100% against it, but it's not particularly great. Ronald
On Mon, Jun 26, 2023 at 3:17 PM Ronald S. Bultje <rsbultje@gmail.com> wrote: > > Hi, > > On Mon, Jun 26, 2023 at 1:47 PM Vignesh Venkatasubramanian <vigneshv-at-google.com@ffmpeg.org> wrote: >> >> In some versions of libsvtav1, setting intra_period_length to 0 >> does not produce the intended result (i.e.) all frames produced >> are not keyframes. >> >> Instead handle the gop_size == 1 as a special case by setting >> the pic_type to EB_AV1_KEY_PICTURE when encoding each frame so >> that all the output frames are keyframes. >> >> SVT-AV1 Bug: https://gitlab.com/AOMediaCodec/SVT-AV1/-/issues/2076 >> >> Example command: ffmpeg -f lavfi -i testsrc=duration=1:size=64x64:rate=30 -c:v libsvtav1 -g 1 -y test.webm >> >> Before: Only first frame is keyframe. >> After: All frames are keyframes. >> >> Signed-off-by: Vignesh Venkatasubramanian <vigneshv@google.com> >> --- >> libavcodec/libsvtav1.c | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) > > > It feels a bit dirty to add workarounds in ffmpeg library wrappers for bugs in these libraries. Not 100% against it, but it's not particularly great. > Yeah I agree that it's not ideal. But minor changes like this that are commented/documented is okay i believe. > Ronald
On 6/26/2023 7:32 PM, Vignesh Venkat wrote: > On Mon, Jun 26, 2023 at 3:17 PM Ronald S. Bultje <rsbultje@gmail.com> wrote: >> >> Hi, >> >> On Mon, Jun 26, 2023 at 1:47 PM Vignesh Venkatasubramanian <vigneshv-at-google.com@ffmpeg.org> wrote: >>> >>> In some versions of libsvtav1, setting intra_period_length to 0 >>> does not produce the intended result (i.e.) all frames produced >>> are not keyframes. >>> >>> Instead handle the gop_size == 1 as a special case by setting >>> the pic_type to EB_AV1_KEY_PICTURE when encoding each frame so >>> that all the output frames are keyframes. >>> >>> SVT-AV1 Bug: https://gitlab.com/AOMediaCodec/SVT-AV1/-/issues/2076 >>> >>> Example command: ffmpeg -f lavfi -i testsrc=duration=1:size=64x64:rate=30 -c:v libsvtav1 -g 1 -y test.webm >>> >>> Before: Only first frame is keyframe. >>> After: All frames are keyframes. >>> >>> Signed-off-by: Vignesh Venkatasubramanian <vigneshv@google.com> >>> --- >>> libavcodec/libsvtav1.c | 16 +++++++++++++++- >>> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> >> It feels a bit dirty to add workarounds in ffmpeg library wrappers for bugs in these libraries. Not 100% against it, but it's not particularly great. >> > > Yeah I agree that it's not ideal. But minor changes like this that are > commented/documented is okay i believe. What are the buggy svt-av1 versions? Can this be wrapped in a preprocessor check, so that if we bump the minimum required version in the future and it's newer, this change can be removed/undone?
On Mon, Jun 26, 2023 at 4:53 PM James Almer <jamrial@gmail.com> wrote: > > On 6/26/2023 7:32 PM, Vignesh Venkat wrote: > > On Mon, Jun 26, 2023 at 3:17 PM Ronald S. Bultje <rsbultje@gmail.com> wrote: > >> > >> Hi, > >> > >> On Mon, Jun 26, 2023 at 1:47 PM Vignesh Venkatasubramanian <vigneshv-at-google.com@ffmpeg.org> wrote: > >>> > >>> In some versions of libsvtav1, setting intra_period_length to 0 > >>> does not produce the intended result (i.e.) all frames produced > >>> are not keyframes. > >>> > >>> Instead handle the gop_size == 1 as a special case by setting > >>> the pic_type to EB_AV1_KEY_PICTURE when encoding each frame so > >>> that all the output frames are keyframes. > >>> > >>> SVT-AV1 Bug: https://gitlab.com/AOMediaCodec/SVT-AV1/-/issues/2076 > >>> > >>> Example command: ffmpeg -f lavfi -i testsrc=duration=1:size=64x64:rate=30 -c:v libsvtav1 -g 1 -y test.webm > >>> > >>> Before: Only first frame is keyframe. > >>> After: All frames are keyframes. > >>> > >>> Signed-off-by: Vignesh Venkatasubramanian <vigneshv@google.com> > >>> --- > >>> libavcodec/libsvtav1.c | 16 +++++++++++++++- > >>> 1 file changed, 15 insertions(+), 1 deletion(-) > >> > >> > >> It feels a bit dirty to add workarounds in ffmpeg library wrappers for bugs in these libraries. Not 100% against it, but it's not particularly great. > >> > > > > Yeah I agree that it's not ideal. But minor changes like this that are > > commented/documented is okay i believe. > > What are the buggy svt-av1 versions? Can this be wrapped in a > preprocessor check, so that if we bump the minimum required version in > the future and it's newer, this change can be removed/undone? > From what i tested, it seems to be there on the tip of tree SVT-AV1 as well. So we can't guard it with macros until the issue is fixed. _______________________________________________ > 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".
Hi Vignesh, On Tue, Jun 27, 2023 at 1:55 PM Vignesh Venkat < vigneshv-at-google.com@ffmpeg.org> wrote: > > >>> In some versions of libsvtav1, setting intra_period_length to 0 > [..] > > >>> SVT-AV1 Bug: https://gitlab.com/AOMediaCodec/SVT-AV1/-/issues/2076 > > >>> > > >>> Example command: ffmpeg -f lavfi -i > testsrc=duration=1:size=64x64:rate=30 -c:v libsvtav1 -g 1 -y test.webm > > >>> > > >>> Before: Only first frame is keyframe. > > >>> After: All frames are keyframes. > [..] > From what i tested, it seems to be there on the tip of tree SVT-AV1 as > well. So we can't guard it with macros until the issue is fixed. > Isn't this the expected behaviour, btw? See https://gitlab.com/AOMediaCodec/SVT-AV1/-/blob/master/Docs/Parameters.md "Keyint --keyint [-2-(2^31)-1] -2 GOP size (frames), use s suffix for seconds (SvtAv1EncApp only) [-2: ~5 seconds, -1: "infinite" only for CRF, 0: == -1] " note the final parts: 0=-1, and -1 means "infinite" keyint (i.e. never insert a new KF). Since FFmpeg's wrapper sets keyint to "gopsize value minus 1", "-g 1" seems to become equivalent to "--keyint 0" on the SVT-AV1 commandline, unless there's some discrepancy between API and CLI interface. (I admit that if I set it to 1, I don't actually get keyframes, but rather intraonly frames for all except the first frame. --keyint 2 correctly uses key - inter pairs. But your "before" statement above appeared to suggest that the first keyframe is followed by inter frames, not intraonly frames. If you meant "intraonly after key" instead of "inter after key", you should probably note that more explicitly. This may also help upstream reproduce in #2076 so they can fix it going forward.) Ronald
On Tue, Jun 27, 2023 at 12:07 PM Ronald S. Bultje <rsbultje@gmail.com> wrote: > > Hi Vignesh, > > On Tue, Jun 27, 2023 at 1:55 PM Vignesh Venkat < > vigneshv-at-google.com@ffmpeg.org> wrote: > > > > >>> In some versions of libsvtav1, setting intra_period_length to 0 > > > [..] > > > > >>> SVT-AV1 Bug: https://gitlab.com/AOMediaCodec/SVT-AV1/-/issues/2076 > > > >>> > > > >>> Example command: ffmpeg -f lavfi -i > > testsrc=duration=1:size=64x64:rate=30 -c:v libsvtav1 -g 1 -y test.webm > > > >>> > > > >>> Before: Only first frame is keyframe. > > > >>> After: All frames are keyframes. > > > [..] > > > From what i tested, it seems to be there on the tip of tree SVT-AV1 as > > well. So we can't guard it with macros until the issue is fixed. > > > > Isn't this the expected behaviour, btw? > > See https://gitlab.com/AOMediaCodec/SVT-AV1/-/blob/master/Docs/Parameters.md > > "Keyint --keyint [-2-(2^31)-1] -2 GOP size (frames), use s suffix for > seconds (SvtAv1EncApp only) [-2: ~5 seconds, -1: "infinite" only for CRF, > 0: == -1] " > > note the final parts: 0=-1, and -1 means "infinite" keyint (i.e. never > insert a new KF). Since FFmpeg's wrapper sets keyint to "gopsize value > minus 1", "-g 1" seems to become equivalent to "--keyint 0" on the SVT-AV1 > commandline, unless there's some discrepancy between API and CLI interface. > Yes, it might be working as documented in the SVT-AV1 API. But the point is that we would need a way to force all key frames (as i think that is what a user would mean when they set "-g 1" in ffmpeg?). > (I admit that if I set it to 1, I don't actually get keyframes, but rather > intraonly frames for all except the first frame. --keyint 2 correctly uses > key - inter pairs. But your "before" statement above appeared to suggest > that the first keyframe is followed by inter frames, not intraonly frames. > If you meant "intraonly after key" instead of "inter after key", you should > probably note that more explicitly. This may also help upstream reproduce > in #2076 so they can fix it going forward.) yeah i wasn't really paying attention to whether the subsequent frames were intra-only or inter frames. i was just looking at whether or not they were keyframes. :) i will update the upstream bug to clarify this part. but in the meantime, i think this patch is reasonable to replicate the behavior of other AV1 encoders with -g 1. > > 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".
Hi, On Wed, Jun 28, 2023 at 12:48 PM Vignesh Venkat < vigneshv-at-google.com@ffmpeg.org> wrote: > i will update the upstream bug to clarify this part. but in the > meantime, i think this patch is reasonable to replicate the behavior > of other AV1 encoders with -g 1. > Sounds reasonable, I think. James, objections? Ronald
On 6/28/2023 8:25 PM, Ronald S. Bultje wrote: > Hi, > > On Wed, Jun 28, 2023 at 12:48 PM Vignesh Venkat < > vigneshv-at-google.com@ffmpeg.org> wrote: > >> i will update the upstream bug to clarify this part. but in the >> meantime, i think this patch is reasonable to replicate the behavior >> of other AV1 encoders with -g 1. >> > > Sounds reasonable, I think. James, objections? > > Ronald No, fine by me.
On Wed, Jun 28, 2023 at 4:27 PM James Almer <jamrial@gmail.com> wrote: > > On 6/28/2023 8:25 PM, Ronald S. Bultje wrote: > > Hi, > > > > On Wed, Jun 28, 2023 at 12:48 PM Vignesh Venkat < > > vigneshv-at-google.com@ffmpeg.org> wrote: > > > >> i will update the upstream bug to clarify this part. but in the > >> meantime, i think this patch is reasonable to replicate the behavior > >> of other AV1 encoders with -g 1. > >> > > > > Sounds reasonable, I think. James, objections? > > > > Ronald > > No, fine by me. > _______________________________________________ > 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". Ronald or James, would you be able to merge this please? :)
Hi, On Thu, Jul 13, 2023 at 5:13 PM Vignesh Venkat < vigneshv-at-google.com@ffmpeg.org> wrote: > On Wed, Jun 28, 2023 at 4:27 PM James Almer <jamrial@gmail.com> wrote: > > > > On 6/28/2023 8:25 PM, Ronald S. Bultje wrote: > > > Hi, > > > > > > On Wed, Jun 28, 2023 at 12:48 PM Vignesh Venkat < > > > vigneshv-at-google.com@ffmpeg.org> wrote: > > > > > >> i will update the upstream bug to clarify this part. but in the > > >> meantime, i think this patch is reasonable to replicate the behavior > > >> of other AV1 encoders with -g 1. > > >> > > > > > > Sounds reasonable, I think. James, objections? > > > > > > Ronald > > > > No, fine by me. > > _______________________________________________ > > 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". > > Ronald or James, would you be able to merge this please? :) > Done. Ronald
diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c index 718a04d221..f2b73361d8 100644 --- a/libavcodec/libsvtav1.c +++ b/libavcodec/libsvtav1.c @@ -242,9 +242,20 @@ static int config_enc_params(EbSvtAv1EncConfiguration *param, if (avctx->level != FF_LEVEL_UNKNOWN) param->level = avctx->level; - if (avctx->gop_size > 0) + // gop_size == 1 case is handled when encoding each frame by setting + // pic_type to EB_AV1_KEY_PICTURE. For gop_size > 1, set the + // intra_period_length. Even though setting intra_period_length to 0 should + // work in this case, it does not. + // See: https://gitlab.com/AOMediaCodec/SVT-AV1/-/issues/2076 + if (avctx->gop_size > 1) param->intra_period_length = avctx->gop_size - 1; + // 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; + if (avctx->framerate.num > 0 && avctx->framerate.den > 0) { param->frame_rate_numerator = avctx->framerate.num; param->frame_rate_denominator = avctx->framerate.den; @@ -462,6 +473,9 @@ static int eb_send_frame(AVCodecContext *avctx, const AVFrame *frame) break; } + if (avctx->gop_size == 1) + headerPtr->pic_type = EB_AV1_KEY_PICTURE; + svt_av1_enc_send_picture(svt_enc->svt_handle, headerPtr); return 0;
In some versions of libsvtav1, setting intra_period_length to 0 does not produce the intended result (i.e.) all frames produced are not keyframes. Instead handle the gop_size == 1 as a special case by setting the pic_type to EB_AV1_KEY_PICTURE when encoding each frame so that all the output frames are keyframes. SVT-AV1 Bug: https://gitlab.com/AOMediaCodec/SVT-AV1/-/issues/2076 Example command: ffmpeg -f lavfi -i testsrc=duration=1:size=64x64:rate=30 -c:v libsvtav1 -g 1 -y test.webm Before: Only first frame is keyframe. After: All frames are keyframes. Signed-off-by: Vignesh Venkatasubramanian <vigneshv@google.com> --- libavcodec/libsvtav1.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)