Skip to content
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

ParseGeometry() can't digest chroma subsampling formatted by MagickSetSamplingFactors() #4793

Closed
janlinde opened this issue Feb 2, 2022 · 2 comments

Comments

@janlinde
Copy link
Contributor

janlinde commented Feb 2, 2022

ImageMagick version

51f487a / Current Git master

Operating system

Linux

Operating system, version and so on

Any

Description

Among other things, ParseGeometry() is designed to parse chroma subsampling definition strings, and its description states: "Values may also be separated by commas, colons, or slashes, and offsets". Consequently, it's used to interpret
_ImageInfo::sampling_factor from various pieces of code decoding and encoding images. ParseGeometry() correctly converts the argument string into sampling factors if the argument is in the form of a:b:c. It doesn't, however, correctly convert subsampling definitions in the form of a,b,c, and _ImageInfo::sampling_factor sometimes contains just that, notably if set by MagickSetSamplingFactors().

ParseGeometry() does

if (strchr(pedantic_geometry,':') != (char *) NULL)

to check if it should normalize sigma and rho. This obviously evaluates to false if ',' is used as a delimiter and omits normalization. See the output of the (attached) test-geometry:

./test-geometry
ratio      flags      rho sig xi  psi chi
4:2:2      0x0010000d 2.0 1.0 2.0 0.0 0.0
4,2,2      0x0000000d 4.0 2.0 2.0 0.0 0.0
4:2:0      0x0010000d 2.0 2.0 0.0 0.0 0.0
4,2,0      0x0000000d 4.0 2.0 0.0 0.0 0.0

I suspect the reason why this was never noticed is that the command-line tools use the core API and set _ImageInfo::sampling_factor string taken from the command-line, i.e. as a:b:c, which is the widely known format. Hence, they are unaffected, e.g.:

convert  -size 640x480 -depth 8 -sampling-factor 4:2:0 -colorspace srgb logo.yuv logo.jpg

works fine.

Over the Wand API, however, this yields incorrect results, see the attached test-set-sampling-factors. It uses MagickSetSamplingFactors(), which formats a 4:2:0 as 4,2,0, warps that into wrong sampling factors, then miscounts input data, and subsequently bails out with "unexpected end of file". Encoding JPEG also fails.

Steps to Reproduce

Steps to Reproduce

Compile and run the attached test-set-sampling-factors.

Expected Behaviour

It should read the included 4:2:0 planar YUV image and store it into a JPEG file

Actual Behaviour

$ make run
gcc -I/usr/include/ImageMagick-7 -DMAGICKCORE_HDRI_ENABLE=1 -DMAGICKCORE_QUANTUM_DEPTH=16  -lMagickWand-7.Q16HDRI -lMagickCore-7.Q16HDRI  -o test-set-sampling-factors main.c
xz -kd logo.yuv.xz
./test-set-sampling-factors logo.yuv logo.jpg
Failed to interpret input data (unexpected end-of-file '': No such file or directory @ error/yuv.c/ReadYUVImage/315)
make: *** [Makefile:16: run] Error 1

Images

No response

janlinde added a commit to janlinde/ImageMagick that referenced this issue Feb 2, 2022
Use colons instead of commas to delimit the contents of
_ImageInfo::sampling_factors, e.g. 4:2:0 instead of 4,2,0, see issue

   ImageMagick#4793.

Signed-off-by: Jan Lindemann <[email protected]>
urban-warrior pushed a commit to ImageMagick/ImageMagick6 that referenced this issue Feb 2, 2022
@urban-warrior
Copy link
Member

Thanks for the problem report. We can reproduce it and will have a patch to fix it in the GIT main branch @ https://github.com/ImageMagick/ImageMagick later today. The patch will be available in the beta releases of ImageMagick @ https://imagemagick.org/download/beta/ by sometime tomorrow.

urban-warrior added a commit that referenced this issue Feb 3, 2022
Use colons instead of commas to delimit the contents of
_ImageInfo::sampling_factors, e.g. 4:2:0 instead of 4,2,0, see issue

   #4793.

Signed-off-by: Jan Lindemann <[email protected]>

Co-authored-by: Cristy <[email protected]>
@janlinde
Copy link
Contributor Author

janlinde commented Feb 5, 2022

Manually closing this issue: c506c6a was merged by @urban-warrior through the commits 6d69fce / 73901f2. Thank you!

@janlinde janlinde closed this as completed Feb 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants