-
Notifications
You must be signed in to change notification settings - Fork 255
Use AOM_TUNE_IQ by default #2830
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| tuneMetric = AOM_TUNE_IQ; | ||
| } | ||
| #endif | ||
| if (aom_codec_control(&codec->internal->encoder, AOME_SET_TUNING, tuneMetric) != AOM_CODEC_OK) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this PR, Yannis!
Can tune iq be applied to just the YUV planes, and not alpha? The perceptual optimizations are likely harmful for the alpha channel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understood "only YUV" as "not RGB", and not "not alpha". But I did not even test for identity MC. Do you know if AOM_TUNE_IQ for all matrix coefficients and color spaces (besides identity I guess)?
Good point for alpha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if AOM_TUNE_IQ for all matrix coefficients and color spaces (besides identity I guess)?
Good question! So, AOM_TUNE_IQ has been tuned for the YCbCr families of matrix coefficients and color spaces. That said, the tune tweaks do generalize (to various degrees) to all matrix coefficients, color spaces, and transfer characteristics that have one luma and two chroma channels.
I think we should be good turning it on globally on still images, except for the identity MC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should AOM_TUNE_PSNR or AOM_TUNE_SSIM be used for alpha?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably both PSNR or SSIM are fine for alpha, but anecdotally I've seen PSNR ring a bit less on grayscale images, so PSNR might be the better choice of the two for alpha (if the decoder interprets ringing as incorrect alpha values that make the underlying image too opaque or transparent).
|
WIth this PR, using |
|
@maryla-uc good catch! tune iq sets up many optimized defaults (including |
Co-authored-by: Vincent Rabaud <[email protected]>
To take sharpness into account with tune_iq. Add aviftunetest.
Done. |
|
Any idea what's holding up this PR? |
There are a few consequences of TUNE=IQ to take into consideration when using it by default:
|
|
@castano another thing worth mentioning: tune IQ is already considered production quality (on libaom 3.13.1 and newer). If a project doesn't have to worry about specific tune ssim behavior, then said project can just switch to use tune IQ right away. For example, The Guardian has been using tune IQ for a few months, and we've received positive feedback from the website admin in the form of overall better image quality and consistency. Tune IQ's improvement in bit allocation by itself can be seen as a "bug fix". I wouldn't wait for this PR to be merged as a sign of feature "readiness". On the contrary, we're just trying to be super thorough in understanding the inherent differences in bitrate allocation and encode time with tune IQ across images, compared to tune SSIM. |
|
I think it makes sense to evaluate the consequences of enabling tune IQ before making it the default choice, but it would be helpful to first add support for tune IQ as a user option, and only later make it the default after more users have had the chance to evaluate it and provide feedback. I have clients interested in using tune IQ, but they won't maintain a custom build of libavif with modifications that they have to merge on every update. Similarly, tools that use libavif won't offer this option until it's officially supported by libavif, so not having this exposed also delays its upstream adoption. |
|
@castano you can enable tune IQ in avifenc with the following parameter: avifenc in turn passes in the parameter to libavif as a key ("tune")/ value ("iq") pair in the following manner: static avifBool avifCodecSpecificOptionsAdd(avifCodecSpecificOptions * options, const char * keyValue)
{
const char * value = strchr(keyValue, '=');
if (value) {
// Keep the parts on the left and on the right of the equal sign,
// but not the equal sign itself.
const size_t keyLength = value - keyValue;
return avifCodecSpecificOptionsAddKeyValue(options, keyValue, keyLength, value + 1);
}
// Pass in a non-NULL, empty string. Codecs can use the mere existence of a key as a boolean value.
return avifCodecSpecificOptionsAddKeyValue(options, keyValue, strlen(keyValue), "");
}
|
|
Hmm... I haven't tried avifenc, but I didn't see the iq option in the aomOptionDefs struct, so I assumed that would not work without additional changes: Ah, I see it gets routed to |
|
Ignacio: The aomOptionDefs struct is only used with older versions of libaom that don't have the For testing this pull request, please use the latest version of libaom, v3.13.1 if you can. Thanks! |
Based on #2599.
Note that "(Default: psnr)" was incorrect.