-
-
Notifications
You must be signed in to change notification settings - Fork 852
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
Convert OrientationMode into something usable. #1976
Conversation
} | ||
else | ||
{ | ||
orientation = (OrientationMode)Convert.ToUInt16(value.Value); | ||
orientation = Convert.ToUInt16(value.Value); | ||
source.Metadata.ExifProfile.RemoveValue(ExifTag.Orientation); |
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.
This line should be removed, because the value is always set below:
source.Metadata.ExifProfile.RemoveValue(ExifTag.Orientation); |
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.
We remove to ensure that any invalid values are gone. We've seen in the wild some odd implementations of this property. SetValue
actually calls TrySetValue
on the value which can return false.
source.Metadata.ExifProfile.RemoveValue(ExifTag.Orientation); | ||
} | ||
|
||
source.Metadata.ExifProfile.SetValue(ExifTag.Orientation, (ushort)OrientationMode.TopLeft); | ||
source.Metadata.ExifProfile.SetValue(ExifTag.Orientation, ExifOrientationMode.TopLeft); |
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.
Would you be open to make it possible to opt-in to removing the value, instead of resetting it to ExifOrientationMode.TopLeft
? This would require a single bool RemoveExifValue { get; }
property and the following code:
if (this.RemoveExifValue)
{
source.Metadata.ExifProfile.RemoveValue(ExifTag.Orientation);
}
else
{
source.Metadata.ExifProfile.SetValue(ExifTag.Orientation, ExifOrientationMode.TopLeft);
}
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.
No, we'll keep it since the value was explicitly set by the previous encoder.
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 wish there was a good way to consume enums in ExifTag.SetValue()
, but I don't see how would it be possible, so LGTM.
Prerequisites
Description
I noticed while working on SixLabors/ImageSharp.Web#215 that our
OrientationMode
enum was internal. I'm assuming because explicitly casting to aushort
each time was not a great developer experience. By converting it to a static class with constants we're able to expose the type in a much more useful form.