Configure max_threads max value to 64

[yit-b]

Add upper bound of 64 to max_threads in init_max_threads() so that encoding does not crash on machines with high core counts.

See the repro and discussion in issue #23

Before this change (on a machine with 96 cores):
Script:

from PIL import Image
import pillow_avif
import numpy as np
from pathlib import Path

def main():
    arr = np.ones((128, 128, 3), dtype=np.uint8)
    img_orig = Image.fromarray(arr)
    out_path = Path("/tmp") / f"foobar.avif"
    print(f"Saving to {out_path}")
    img_orig.save(out_path)

if __name__ == "__main__":
    main()

Output:

RuntimeError: Failed to encode image: Encoding of color planes failed

After this change: program works as expected.

[fdintino]

It looks like the line in codec_aom.c linked from #23 (comment) was added in libavif 1.0.2 and the most recent wheel builds are for libavif 1.0.1. It might be advantageous to have 64+ threads with some codecs—SVT-AV1 in particular—so I'm not sure it's a great idea to limit this for all codecs. However, it's tricky to know which encoder is being used by default, so it also wouldn't be straightforward to limit maxThreads for only libaom. How would you feel about wrapping this code in an #if AVIF_VERSION < 1000200 directive? That way, users with newer versions of libavif will be able to use more threads for rav1e and SVT-AV1, while users with libavif 1.0.1 and below won't encounter this issue.

[yit-b]

It looks like this is an issue for both avm and aom:
https://github.com/search?q=repo%3AAOMediaCodec%2Flibavif+%2264+threads%22&type=code

With that check, we will still have the issue when using newer libavif versions with aom or avm. Maybe it would be better to allow users to specify the maxThreads argument manually, but default to min(cpus, 64)? If users are using SVT and have more than 64 cores, then they can set it higher if they want.

The library is currently not usable at all (with aom or avm) on large machines due to the current default behavior.

[yit-b]

Also I don't see any evidence that aom and avm didn't crash before that check was added. Maybe the check was added to protect against it because people were seeing crashes when setting it higher (like we are now).

[yit-b]

Alternatively: if the "codec" arg is provided and is "aom" or "avm", then limit maxThreads to 64.

This arg here:
https://github.com/fdintino/pillow-avif-plugin/blob/main/src/pillow_avif/_avif.c#L347

[fdintino]

The check was added at the same time that the --jobs default argument to the avifenc utility changed from 1 to all, which caused it to use as many threads as there are cores. Presumably whoever introduced that change also had 64+ cores and so proactively added the check in codec_aom. AVM is actually just a fork of AOM, so it makes sense it would have the same issue. But AVM actually doesn't encode AVIF images; it offers experimental support for the next-generation codec. So it's not relevant to this library.

Why don't you think the min maxThreads change in libavif 1.0.2 fixes this issue? When I run avifenc --jobs 96 with avif 1.0.1, I get an error. But with 1.0.2 I don't.

[fdintino]

More to the point, if I hard-code maxThreads to 96, I see the same pytest failures as you. But when I recompile against the latest libavif, the tests pass. Hence my suggestion that we use preprocessor directives to target your change to versions of libavif that don't already cap the value passed to aom.

[yit-b]

I ran it again on a machine with way less than 64 cores and with libavif > 1.0.2 and I can still repro when setting maxThreads > 64.

Check my libavif version:

(base) Wyatts-MacBook-Pro:pillow-avif-plugin wyatt$ conda list | grep libavif
libavif                   1.0.3                hddeac66_2    conda-forge
libavif16                 1.0.3                hddeac66_2    conda-forge

Install clean from main:

(base) Wyatts-MacBook-Pro:pillow-avif-plugin wyatt$ pip install .
Processing /Users/wyatt/pillow-avif-plugin
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Installing backend dependencies ... done
  Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: pillow-avif-plugin
  Building wheel for pillow-avif-plugin (pyproject.toml) ... done
  Created wheel for pillow-avif-plugin: filename=pillow_avif_plugin-1.4.2-cp311-cp311-macosx_10_9_x86_64.whl size=12951 sha256=771ec14f3026c36784ab7b9f50c5be0cfa6e7ba2221c99a196c3c137e760133c
  Stored in directory: /Users/wyatt/Library/Caches/pip/wheels/8a/8b/95/e0aac7d28725197950751afc50e11e638e3bffc08c5cf5c295
Successfully built pillow-avif-plugin
Installing collected packages: pillow-avif-plugin
  Attempting uninstall: pillow-avif-plugin
    Found existing installation: pillow-avif-plugin 1.4.2
    Uninstalling pillow-avif-plugin-1.4.2:
      Successfully uninstalled pillow-avif-plugin-1.4.2
Successfully installed pillow-avif-plugin-1.4.2

Run the repro:

(base) Wyatts-MacBook-Pro:pillow-avif-plugin wyatt$ python ~/repro.py
Saving to /tmp/foobar.avif

Make and check my changes:

(base) Wyatts-MacBook-Pro:pillow-avif-plugin wyatt$ git diff
(base) Wyatts-MacBook-Pro:pillow-avif-plugin wyatt$ vi src/pillow_avif/_avif.c
(base) Wyatts-MacBook-Pro:pillow-avif-plugin wyatt$ git diff
diff --git a/src/pillow_avif/_avif.c b/src/pillow_avif/_avif.c
index 3b2970d..370fa95 100644
--- a/src/pillow_avif/_avif.c
+++ b/src/pillow_avif/_avif.c
@@ -448,6 +448,8 @@ AvifEncoderNew(PyObject *self_, PyObject *args) {
             init_max_threads();
         }

+       max_threads = 96;
+
         encoder->maxThreads = max_threads;
 #if AVIF_VERSION >= 1000000
         if (enc_options.qmin != -1 && enc_options.qmax != -1) {

Re-build and install:

(base) Wyatts-MacBook-Pro:pillow-avif-plugin wyatt$ pip install .
Processing /Users/wyatt/pillow-avif-plugin
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Installing backend dependencies ... done
  Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: pillow-avif-plugin
  Building wheel for pillow-avif-plugin (pyproject.toml) ... done
  Created wheel for pillow-avif-plugin: filename=pillow_avif_plugin-1.4.2-cp311-cp311-macosx_10_9_x86_64.whl size=12951 sha256=74668ac1e2d5b064074510a65df8b3b3eaae5375d0d2be5de5a819692ae03caf
  Stored in directory: /Users/wyatt/Library/Caches/pip/wheels/8a/8b/95/e0aac7d28725197950751afc50e11e638e3bffc08c5cf5c295
Successfully built pillow-avif-plugin
Installing collected packages: pillow-avif-plugin
  Attempting uninstall: pillow-avif-plugin
    Found existing installation: pillow-avif-plugin 1.4.2
    Uninstalling pillow-avif-plugin-1.4.2:
      Successfully uninstalled pillow-avif-plugin-1.4.2
Successfully installed pillow-avif-plugin-1.4.2

Run it again:

(base) Wyatts-MacBook-Pro:pillow-avif-plugin wyatt$ python ~/repro.py
Saving to /tmp/foobar.avif
Traceback (most recent call last):
  File "/Users/wyatt/repro.py", line 14, in <module>
    main()
  File "/Users/wyatt/repro.py", line 11, in main
    img_orig.save(out_path)
  File "/Users/wyatt/miniconda3/lib/python3.11/site-packages/PIL/Image.py", line 2413, in save
    save_handler(self, fp, filename)
  File "/Users/wyatt/miniconda3/lib/python3.11/site-packages/pillow_avif/AvifImagePlugin.py", line 244, in _save
    enc.add(
RuntimeError: Failed to encode image: Encoding of color planes failed

[fdintino]

Could you try pip installing this wheel and running your reproduction script? This is what the CI process creates for the latest libavif version.

[yit-b]

Sorry I'm not going to install a whl from dropbox... I hope you understand where I'm coming from. Can you try reproducing with conda?

Install deps

conda install -c conda-forge libavif libaom
conda list | grep libavif

Max sure libavif is > 1.0.2

Install pillow-avif-plugin and run

git clone git@github.com:fdintino/pillow-avif-plugin.git
cd pillow-avif-plugin
pip install .
python /path/to/repro.py

Then edit max_threads after the call to init_max_threads() to be = 96. Repeat the pip install and run again.

[yit-b]

I updated my PR to do the codec check since I confirmed (pending your repro) that this is still an issue on libavif 1.0.3. After this change I can not repro the crash no matter what I set max_threads to.

[yit-b]

I repeated the same repro with the same results with libavif 1.0.1. Also, to be super sure, I printed the libavif version string in there:

(base) Wyatts-MacBook-Pro:pillow-avif-plugin-fork wyatt$ git diff
diff --git a/src/pillow_avif/_avif.c b/src/pillow_avif/_avif.c
index 21c4e8f..c4ff7e1 100644
--- a/src/pillow_avif/_avif.c
+++ b/src/pillow_avif/_avif.c
@@ -448,13 +448,9 @@ AvifEncoderNew(PyObject *self_, PyObject *args) {
             init_max_threads();
         }

-        if (strcmp(codec, "aom") == 0 || strcmp(codec, "avm") == 0){
-            if (max_threads > 64 ) {
-                // Encoding with libaom and libavm will fail if
-                // encoder->maxThreads is set higher than 64
-                max_threads = 64;
-            }
-        }
+        max_threads = 92;
+
+        printf(avifVersion());

         encoder->maxThreads = max_threads;
 #if AVIF_VERSION >= 1000000

Rebuild and install:

(base) Wyatts-MacBook-Pro:pillow-avif-plugin-fork wyatt$ pip install .
Processing /Users/wyatt/pillow-avif-plugin-fork
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Installing backend dependencies ... done
  Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: pillow-avif-plugin
  Building wheel for pillow-avif-plugin (pyproject.toml) ... done
  Created wheel for pillow-avif-plugin: filename=pillow_avif_plugin-1.4.2-cp311-cp311-macosx_10_9_x86_64.whl size=13003 sha256=e31e96d91063db9971b87cdeba683a13159ab4c8c91b704b45a37cad0a94f870
  Stored in directory: /Users/wyatt/Library/Caches/pip/wheels/82/92/68/5def896a57056180bd7e2e3f8d733b6f67ef3df8523e44a3d9
Successfully built pillow-avif-plugin
Installing collected packages: pillow-avif-plugin
  Attempting uninstall: pillow-avif-plugin
    Found existing installation: pillow-avif-plugin 1.4.2
    Uninstalling pillow-avif-plugin-1.4.2:
      Successfully uninstalled pillow-avif-plugin-1.4.2
Successfully installed pillow-avif-plugin-1.4.2

Run it:

(base) Wyatts-MacBook-Pro:pillow-avif-plugin-fork wyatt$ python ~/repro.py
Saving to /tmp/foobar.avif
1.0.3Traceback (most recent call last):
  File "/Users/wyatt/repro.py", line 14, in <module>
    main()
  File "/Users/wyatt/repro.py", line 11, in main
    img_orig.save(out_path, codec="aom")
  File "/Users/wyatt/miniconda3/lib/python3.11/site-packages/PIL/Image.py", line 2413, in save
    save_handler(self, fp, filename)
  File "/Users/wyatt/miniconda3/lib/python3.11/site-packages/pillow_avif/AvifImagePlugin.py", line 244, in _save
    enc.add(
RuntimeError: Failed to encode image: Encoding of color planes failed

Note the 1.0.3 printed

Check with conda:

(base) Wyatts-MacBook-Pro:pillow-avif-plugin-fork wyatt$ conda list | grep avif
libavif                   1.0.3                hddeac66_2    conda-forge
libavif16                 1.0.3                hddeac66_2    conda-forge
pillow-avif-plugin        1.4.2                    pypi_0    pypi

I'm not sure how much more certainty we need.

[fdintino]

I see. I was compiling against the main branch from git source, which has the fix. But it looks like it never made it onto the v1.0.x release branch. I've flagged this to the maintainers of libavif. This had been the source of my hesitation: it's fine to fix it in this repository while we wait for it to make it into a release, but it really should be addressed in libavif.

The change you made to check the codec is incomplete because libaom is the default encoder if available (i.e. codecChoice = AVIF_CODEC_CHOICE_AUTO). We don't need to concern ourselves with avm because it's an experimental feature of libavif that can't be used with this library (whereas AOM is for AV1—aka AVIF—AVM is for AV2, a different format). I'll make a code suggestion for how I would adapt your patch to handle the auto codec condition.

[yit-b]

Makes sense to me. Thanks @fdintino!