Refactor avifAOMOptionsContainExplicitTuning()

[y-guyon]

Hopefully the logic around useTuneIq is easier to follow.

[wantehchang]

Yannis: Thank you for refactoring the code. I think this PR is correct. I suggest some changes. To save a roundtrip, I implemented my suggestions in the following patch. Please apply the patch and edit it as you see fit. You can merge the PR without waiting for me.

diff --git a/src/codec_aom.c b/src/codec_aom.c
index 53971885..f9d72b27 100644
--- a/src/codec_aom.c
+++ b/src/codec_aom.c
@@ -390,29 +390,34 @@ static avifBool avifKeyEqualsName(const char * key, const char * name, avifBool
 // in libaom, so this definition won't ever get out of sync.
 #define AOM_TUNE_IQ 10
 #endif
-static const struct aomOptionEnumList tuningEnum[] = { { "psnr", AOM_TUNE_PSNR },
-                                                       { "ssim", AOM_TUNE_SSIM },
-                                                       { "iq", AOM_TUNE_IQ },
-                                                       { NULL, 0 } };
+// Tune IQ string -> enum mapping
+static const struct aomOptionEnumList tuneIqEnum[] = { { "iq", AOM_TUNE_IQ }, { NULL, 0 } };
 
 // Returns true if codec-specific options for AOM contain a tune metric setting. Returns false otherwise.
-// Sets tuneMetric if codec-specific options for AOM contain a known tune metric setting. Otherwise sets tuneMetric to -1.
-static avifBool avifAOMOptionsContainExplicitTuning(const avifCodec * codec, avifBool alpha, int * tuneMetric)
+// Sets *useTuneIq to true if codec-specific options for AOM contain AOM_TUNE_IQ. Otherwise sets *useTuneIq to false.
+static avifBool avifAOMOptionsContainExplicitTuning(const avifCodec * codec, avifBool alpha, avifBool * useTuneIq)
 {
-    *tuneMetric = -1;
+    *useTuneIq = AVIF_FALSE;
+    avifBool isAnyTuneDefined = AVIF_FALSE;
+
     // If there are multiple "tune" options specified, honor the last one.
     // For consistent behavior, handle both cases where tune was either specified as a string (e.g. tune=iq),
     // or as an enum value (e.g. tune=10).
     for (uint32_t i = 0; i < codec->csOptions->count; ++i) {
         const avifCodecSpecificOption * entry = &codec->csOptions->entries[i];
         if (avifKeyEqualsName(entry->key, "tune", alpha)) {
-            if (!aomOptionParseEnum(entry->value, tuningEnum, tuneMetric)) {
-                *tuneMetric = -1; // Unknown tune option.
+            isAnyTuneDefined = AVIF_TRUE;
+
+            int val;
+            if (aomOptionParseEnum(entry->value, tuneIqEnum, &val)) {
+                assert(val == AOM_TUNE_IQ);
+                *useTuneIq = AVIF_TRUE;
+            } else {
+                *useTuneIq = AVIF_FALSE;
             }
-            return AVIF_TRUE;
         }
     }
-    return AVIF_FALSE;
+    return isAnyTuneDefined;
 }
 
 static avifBool avifProcessAOMOptionsPreInit(avifCodec * codec, avifBool alpha, struct aom_codec_enc_cfg * cfg)
@@ -450,6 +455,12 @@ struct aomOptionDef
     const struct aomOptionEnumList * enums;
 };
 
+static const struct aomOptionEnumList tuningEnum[] = { //
+    { "psnr", AOM_TUNE_PSNR },                         //
+    { "ssim", AOM_TUNE_SSIM },                         //
+    { NULL, 0 }
+};
+
 static const struct aomOptionDef aomOptionDefs[] = {
     // Adaptive quantization mode
     { "aq-mode", AV1E_SET_AQ_MODE, AVIF_AOM_OPTION_UINT, NULL },
@@ -720,9 +731,8 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,
     // False otherwise (including if libaom uses tune=iq by default, which is not the case as of v3.13.1 and earlier versions).
     avifBool useTuneIq;
 
-    int tuneMetric;
-    if (avifAOMOptionsContainExplicitTuning(codec, alpha, &tuneMetric)) {
-        useTuneIq = (tuneMetric == AOM_TUNE_IQ);
+    if (avifAOMOptionsContainExplicitTuning(codec, alpha, &useTuneIq)) {
+        // avifAOMOptionsContainExplicitTuning() has set useTuneIq.
     } else if (!codec->internal->encoderInitialized) {
         // libavif only needs to set the default tune metric for the first frame,
         // because libaom will persist that setting until explicitly changed.

[wantehchang]

I changed this array back to the tuneIqEnum array in the original code. I also restored the tuningEnum array at its original location.

[wantehchang]

I changed the output parameter int * tuneMetric to avifBool * useTuneIq because that's what the caller cares about.

[wantehchang]

To honor the last "tune" option, the for loop must run to completion as the avifImageUsesTuneIq() function in the original code does.