-
Notifications
You must be signed in to change notification settings - Fork 200
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
Change imir.mode back to imir.axis #1509
Change imir.mode back to imir.axis #1509
Conversation
Change imir.mode back to imir.axis for the libavif 1.0.0 release. In ISO/IEC 23008-12:2017 Draft Amendment 2, the 'axis' field of the 'imir' property was renamed 'mode', so we changed imir.axis to imir.mode. But in ISO/IEC 23008-12:2022, the 'axis' field of the 'imir' property was not renamed. In comments and messages, avoid using the words "vertical" and "horizontal" to describe how the mirroring is performed. Also avoid the word "axis" when possible. Code that needs to support both old and new versions of libavif will need to test #if AVIF_VERSION_MAJOR >= 1.
@@ -117,7 +117,7 @@ static void avifImageDumpInternal(const avifImage * avif, uint32_t gridCols, uin | |||
printf(" * irot (Rotation) : %u\n", avif->irot.angle); | |||
} | |||
if (avif->transformFlags & AVIF_TRANSFORM_IMIR) { | |||
printf(" * imir (Mirror) : Mode %u (%s)\n", avif->imir.mode, (avif->imir.mode == 0) ? "top-to-bottom" : "left-to-right"); | |||
printf(" * imir (Mirror) : %u (%s)\n", avif->imir.axis, (avif->imir.axis == 0) ? "top-to-bottom" : "left-to-right"); |
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.
Note that I deleted "Mode" from this message rather than replace it with "Axis".
@@ -132,7 +132,7 @@ static void syntaxLong(void) | |||
printf(" --crop CROPX,CROPY,CROPW,CROPH : Add clap property (clean aperture), but calculated from a crop rectangle\n"); | |||
printf(" --clap WN,WD,HN,HD,HON,HOD,VON,VOD: Add clap property (clean aperture). Width, Height, HOffset, VOffset (in num/denom pairs)\n"); | |||
printf(" --irot ANGLE : Add irot property (rotation). [0-3], makes (90 * ANGLE) degree rotation anti-clockwise\n"); | |||
printf(" --imir MODE : Add imir property (mirroring). 0=top-to-bottom, 1=left-to-right\n"); | |||
printf(" --imir AXIS : Add imir property (mirroring). 0=top-to-bottom, 1=left-to-right\n"); |
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.
Here and at line 1446 I use the words "AXIS" and "axis" in the messages. I wonder if we should continue to use the word "MODE" and "mode" in the messages. Thoughts?
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.
The only reason we used "mode" anywhere was due to the renaming inside of the HEIF standard's draft, which ended up not being used. I think leveraging the original word "axis" in all of these places is appropriate.
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 the review. I forgot to add an entry to the changelog. Please review the second commit.
@@ -81,6 +81,7 @@ List of incompatible ABI changes in this release: | |||
memory allocation failures. | |||
* avifReadImage(), avifJPEGRead() and avifPNGRead() now remove the trailing zero | |||
byte from read XMP chunks, if any. See avifImageFixXMP(). | |||
* The 'mode' member of the avifImageMirror struct was renamed 'axis'. |
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 didn't mention this change in the list of incompatible ABI changes because even though this is an incompatible API change, this doesn't change the ABI. I wonder we should change that list to be the list of incompatible API changes.
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.
You can also start an "API changes" list.
@@ -81,6 +81,7 @@ List of incompatible ABI changes in this release: | |||
memory allocation failures. | |||
* avifReadImage(), avifJPEGRead() and avifPNGRead() now remove the trailing zero | |||
byte from read XMP chunks, if any. See avifImageFixXMP(). | |||
* The 'mode' member of the avifImageMirror struct was renamed 'axis'. |
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.
You can also start an "API changes" list.
Change imir.mode back to imir.axis for the libavif 1.0.0 release.
In ISO/IEC 23008-12:2017 Draft Amendment 2, the 'axis' field of the 'imir' property was renamed 'mode', so we changed imir.axis to imir.mode. But in ISO/IEC 23008-12:2022, the 'axis' field of the 'imir' property was not renamed.
In comments and messages, avoid using the words "vertical" and "horizontal" to describe how the mirroring is performed. Also avoid the word "axis" when possible.
Code that needs to support both old and new versions of libavif will need to test #if AVIF_VERSION_MAJOR >= 1.