Thanks for the update.

From just reading the code seems fine. For testing we'll need to re-compile the libraries.

To my understanding there are three encoder implementations:

1. aom-av1 (Reference implementation. Has the most features but is generally considered the slowest of the three)
2. rav1e (Rust implementation, has more features than SVT1 but seems to be a bit slower generally)
3. SVT-AV1 (The fastest but from my understanding doesn't have all the features that the reference implementation or rav1e has)

From what I can see, it seems like there are still some cases where someone would want to use one of the other encoders besides SVT, for example quality if more important than speed.
Perhaps my information is outdated, but it seems like rav1e and aom is able to compress video at a higher quality than SVT because SVT doesn't have all features implemented in the standard yet.

To me it would make sense to be able to choose which encoder to use, if it is available on the system.
Now, all of these three are optional dependencies two ffmpeg, so if we wanted to we could only ship one (or more) of them with Blender.

However I don't know if this is something we want to support or not (the ability to choose the specific encoder)

Within Blender the important things are to be able to read AV1 stream, and be able to output an AV1. I would not expose the setting as a codec as it is rather messy to explain what the difference is to users. The way I see it if we want to support multiple codecs then it needs to be hooked up into a preset system somehow: when the best quality requested from options it could imply one codec or another.

When you're talking about missing features, does it mean that, for example, SVT-AV1 will be unable to read certain files?

In D14920#401249, @Sergey Sharybin (sergey) wrote:

Within Blender the important things are to be able to read AV1 stream, and be able to output an AV1. I would not expose the setting as a codec as it is rather messy to explain what the difference is to users. The way I see it if we want to support multiple codecs then it needs to be hooked up into a preset system somehow: when the best quality requested from options it could imply one codec or another.

When you're talking about missing features, does it mean that, for example, SVT-AV1 will be unable to read certain files?

From my understanding of it, SVT-AV1 is purely an AV1 encoder, and FFMpeg only uses dav1d for decoding AV1.

EDIT: It looks like aom is the only one that's both an encoder and decoder. Everything else appears to be only one or the other.

I tested rendering with both libaom-av1 and libsvtav1, and libsvtav1 is usually faster for most cases. For now I will leave the code that prioritizes libsvtav1 first when encoding to AV1, but I may change this if I find a better alternative. The three AV1 encoders have some quirks for encoding, and I have added some sanity checks account for them, but there may be more things to fix. For example, libaom-av1 may have better performance if another parameter(s) is set (such as "-tiles" for more parallelism when encoding) and I haven't added/tested this yet. I may add more changes after some testing.

Whichever codecs are ultimately decided, please update intern/ffmpeg/tests/ffmpeg_codecs.cc as appropriate so that we can be sure that ffmpeg has been built properly now and in the future when updates are made.

In D14920#401523, @Jesse Yurkovich (deadpin) wrote:

Whichever codecs are ultimately decided, please update intern/ffmpeg/tests/ffmpeg_codecs.cc as appropriate so that we can be sure that ffmpeg has been built properly now and in the future when updates are made.

If libaom-av1 is used:

FFMPEG_TEST_VCODEC_NAME(libaom-av1, AV_PIX_FMT_YUV420P)

will not work, because there is a dash in the encoder's name.
If aom will be included, I will use the alternative:

TEST(ffmpeg, libaom_av1_AV_PIX_FMT_YUV420P)
{
  EXPECT_TRUE(test_codec_video_by_name("libaom-av1", AV_PIX_FMT_YUV420P));
}

I think I'm ready for review again, so please test away if needed.

I must note that because the patch adds a test for "libaom-av1" as the encoder, this implicitly requires the "aom" package (at least it's called this in ArchLinux), which also provides an AV1 decoder. So by including the "libaom-av1" encoder, the decoder should also be available, which should implicitly ensure that Blender can open AV1 videos (by leveraging FFMpeg).

After some testing, I've found that the rav1e encoder has been under-performing with the current settings (using VMAF). I will update soon with tweaked settings.

Can't test this with current libs, but code looks fine to me as well, so no objections here. I don't really have strong opinion on particular codecs/details as long as they are sensible.

Just to see if understand this correctly there's 3 AV1 libraries libaom-av1, librav1e, and libsvtav1. is it suggested we enable all 3 in the build we ship on blender.org? The updated unit test only seems to add a test for libaom-av1 can anyone clarify this? (asking since i'm the one most likely doing the work of updating our ffmpeg build)

In D14920#404605, @Ray Molenkamp (LazyDodo) wrote:

Just to see if understand this correctly there's 3 AV1 libraries libaom-av1, librav1e, and libsvtav1. is it suggested we enable all 3 in the build we ship on blender.org? The updated unit test only seems to add a test for libaom-av1 can anyone clarify this? (asking since i'm the one most likely doing the work of updating our ffmpeg build)

I set up the encoder searching such that it should not fail if only one of the three encoders are available (which is depending on avcodec_find_encoder(AV_CODEC_ID_AV1) such that FFMpeg should return any available encoder). You can see mention of such in line 487 of writeffmpeg.c .

EDIT: So yes, I think only having libaom-av1 enabled is fine for the bare minimum.

@Sebastian Parborg (zeddb) / @Sergey Sharybin (sergey) can you two get to a consensus what to ship?

Besides my comments, I think this generally looks fine.

We will have to discuss a bit more which codecs we want to ship per default.
I think we could probably ship with only libaom and it should be fine.

@Stephen Seo (seodisparate) Perhaps for a later patch you could contribute a VMAF test script?
I think it would be really interesting in perhaps later down the line have some tests using it to ensure that expected video quality hasn't degraded between releases.

I'm also very interested in your findings where rav1e produced lower scores in Blender than when using ffmpeg via the command line.
Do you have any hunch on why this is the case?

Ok, from discussions it sounds like just leaving the test for AOM is fine. If there are any misunderstandings, please let me know. (I will continue reviewing comments/discussions.)

I have been using a simple script using defaults to check vmaf scores between "original" and "reencoded" (which is noted as "reference" and "distorted" respectively in the script).

In D14920#410101, @Sebastian Parborg (zeddb) wrote:

...
I'm also very interested in your findings where rav1e produced lower scores in Blender than when using ffmpeg via the command line.
Do you have any hunch on why this is the case?

I think it may have to do with the default bitrate settings being applied in Blender, whereas with ffmpeg, I made no such specification when using ffmpeg in the terminal. I probably will do more tests again since I changed an encoding parameter with AOM, but since the parameters for rav1e have not changed, those results will be the same for me.

EDIT: Actually, it seems that bitrate is set to zero when CRF is used (which I have set up AV1 to use). So if that's not the case, it should be some other setting being applied somewhere, but I'm not too familiar with Blender's code base..

Due to the use of power_of_2_min_i(), a situation will arise where a non-power-of-2 amount of threads will not be fully utilized. However, if I switch the function calls to power_of_2_max_i(), then the "next power-of-two value" will be used in the "tiles" parameter setting for AOM AV1 encoding. So is it better to potentially under-utilize the specified/requested amount of threads, or to over-utilize? This probably isn't a problem if the thread-count is just a power-of-two value such as 8, 16, or 32, but values in between will currently under-utilize the specified amount of threads.

I've talked to Sergey about this and it is best if we under commit the amount of thread.
Usually (at least in my experience) it is not too bad to over commit, however for render farms and situations where you want Blender to never use more cores than specified.
(IE if we tell Blender to only use 6 thread, it doesn't use 8 threads)

Just to make sure:
I think this should be good to go now, right?

I think this should be good to go now, right?

We'd need to land an updated ffmpeg in svn first, otherwise you'll have a failing test on your hands

In D14920#411238, @Sebastian Parborg (zeddb) wrote:

I've talked to Sergey about this and it is best if we under commit the amount of thread.
Usually (at least in my experience) it is not too bad to over commit, however for render farms and situations where you want Blender to never use more cores than specified.
(IE if we tell Blender to only use 6 thread, it doesn't use 8 threads)

Actually, when using libsvtav1, it prints to stdout the number of threads it will use. There is no clear option to manipulate the number of threads used, and when tested on my 16-core system, the number of threads printed was about 617.

Just to make sure:
I think this should be good to go now, right?

I currently see no need to add further changes unless someone notices something.

The libs to support this should have landed now (the ffmpeg test is passing for me locally), but this diff may needs a rebase to latest master for the bots to pass, as they appear to be checking out an older library set without aom when i tested just now

https://builder.blender.org/admin/#/builders/18/builds/575

Someone on chat asked about this diff, thought it was a fair question, platform wise the libs are ready, are we landing this for 3.4?

It should be fine to land the patch if it is ready.

Bump 🔥

I landed it, i admit imho we could have done better here, i'm not entirely sure why this dragged on as long as it did after the libs landed.

This this is probably the usual causes causes.
IIRC we postponed this as it was too late to merge for 3.3.
Then at least I forgot about it at the start of the 3.4 period.

I guess we might be able to queue up things like this in a better way with gitea.
Perhaps we could add postponed merges like this into a "milestone" list for the appropriate bcon in the next release cycle?
The we would have a clearer "this feature is good to go for this release, but not merged yet" list the people can go over and poke devs about.