-
Notifications
You must be signed in to change notification settings - Fork 214
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
Fix monochrome ICC profile error message in apps #2673
Conversation
Add CHANGELOG entry. [skip ci]
[skip ci]
apps/shared/avifjpeg.c
Outdated
@@ -911,7 +911,7 @@ static avifBool avifJPEGReadInternal(FILE * f, | |||
iccData = iccDataTmp; | |||
if (requestedFormat == AVIF_PIXEL_FORMAT_YUV400) { | |||
fprintf(stderr, | |||
"The image contains an RGB ICC profile which is incompatible with the requested output " | |||
"The image contains an ICC profile which is incompatible with the requested output " |
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.
Grayscale ICC profiles are also a thing (see iccmaker.h/c). It's true that here we don't actually check the type of profile, just that there is a profile. But I guess it's safe to assume that if the original image was not grayscale, then the icc is not grayscale. We handle grayscale jpegs, right? Is this code path triggered then as well? Because then it probably shouldn't be... I guess this is for another PR if something needs to be fixed.
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.
Actually if this is the main goal of this PR then it should be in this PR... (or dropped)
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 handle grayscale jpegs, right? Is this code path triggered then as well?
No idea.
I guess this is for another PR if something needs to be fixed.
Yes.
Actually if this is the main goal of this PR then it should be in this PR... (or dropped)
No, just "RGB ICC" looked wrong, especially when reading a JPEG.
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.
IMO it's a bit misleading to say that ICC is incompatible with grayscale, because fundamentally it's not, but we can say this is meant to say that we don't support it...
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.
Feel free to send a PR.
* main: (53 commits) Fix reading gray/color ICC profile from color/gray image in apps (AOMediaCodec#2675) Revert "Use SVT_LOG=0 in avifsvttest for SVT-AV1 3.0.0 (AOMediaCodec#2668)" (AOMediaCodec#2674) Fix monochrome ICC profile error message in apps (AOMediaCodec#2673) Add missing changelog for `avifdec --index all` (AOMediaCodec#2672) Specify target platform version in binary artifacts (AOMediaCodec#2652) avidec: output all frames of animations when passed `--index all` (AOMediaCodec#2670) Do not allow keeping the ICC when converting from RGB to gray (AOMediaCodec#2669) Update CHANGELOG (AOMediaCodec#2666) Use SVT_LOG=0 in avifsvttest for SVT-AV1 3.0.0 (AOMediaCodec#2668) read.c: fix empty struct initializers (AOMediaCodec#2661) avifutil.c: fix avifQueryCPUCount() empty param list (AOMediaCodec#2662) Add libyuv dependency to the README.md (AOMediaCodec#2664) fix: patch libyuv CMakeLists for compatibility with gcc 10 (AOMediaCodec#2660) Use default for tensorflow with libavm (AOMediaCodec#2624) Bump some Rust dependencies (AOMediaCodec#2658) Use C99 syntax to initialize avifFileType struct (AOMediaCodec#2657) Bump the github-actions group with 3 updates libargparse: use a git patch file instead of sed command (AOMediaCodec#2654) y4m: Remove some repeated code Remove fully static instructions (AOMediaCodec#2649) ...
Add CHANGELOG entry for #2669.