-
Notifications
You must be signed in to change notification settings - Fork 5
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
add support for the AVIF, HEIC and HEIF formats #13
base: master
Are you sure you want to change the base?
Conversation
d6a9686
to
22bf034
Compare
Thanks for this, @bu6n. Great addition. Yes, it seems a proper approach mimicking the style of the previous ones, so be sure you add enough proper tests and check coverage to be sure ~100% of new code is tested. Checking the code, performance should be close to its optimal, so, I won't worry about doing benchmarks again for these new formats. Nevertheless, if you think about any optimization while doing test coverage, feel free to try and let me know (benchee could help for quick verifications). |
Hey @bu6n I am adding you as collaborator to be able to review a future PR. I have been studying this today, since it has been untouched for some months, as tests + enough code coverage of the new functionality would be required. However, as I started creating tests, I have seen that it is not extracting the proper height: $ convert tiff/layers.tiff heif/layers.heic
$ hexdump -C heif/layers.heic | head
00000000 00 00 00 1c 66 74 79 70 68 65 69 63 00 00 00 00 |....ftypheic....|
00000010 6d 69 66 31 68 65 69 63 6d 69 61 66 00 00 01 6b |mif1heicmiaf...k|
00000020 6d 65 74 61 00 00 00 00 00 00 00 21 68 64 6c 72 |meta.......!hdlr|
00000030 00 00 00 00 00 00 00 00 70 69 63 74 00 00 00 00 |........pict....|
00000040 00 00 00 00 00 00 00 00 00 00 00 00 0e 70 69 74 |.............pit|
00000050 6d 00 00 00 00 00 01 00 00 00 22 69 6c 6f 63 00 |m........."iloc.|
00000060 00 00 00 44 40 00 01 00 01 00 00 00 00 01 8f 00 |...D@...........|
$ identify heif/layers.heic
heif/layers.heic HEIC 130x42 130x42+0+0 8-bit sRGB 1467B 0.000u 0:00.002
$ mix test
1) test force - heic disk image - #seems? #type #info (ExImageInfoTest.Images.HEIFTest)
test/ex_image_info_test/images/heif_test.exs:27
Assertion with == failed
code: assert info(images["heic"], :heic) == {"image/heic", 130, 42, "HEIC"}
left: {"image/heic", 130, 64, "HEIC"}
right: {"image/heic", 130, 42, "HEIC"}
stacktrace:
test/ex_image_info_test/images/heif_test.exs:30: (test) Therefore, this requires more work. I don't close this PR, in case you want to provide all tests and fixes. Meanwhile, as I have time (I hope at some point in Jan-Feb), I will work in a new branch |
Thanks for taking the time to check, I had put this on the shelf a bit. Would you be able to provide the whole file as an attachment so that I can further check what the problem is? Edit: I was too fast, found the file in the test folder :) |
So, there seems to be more info in the |
I have pushed some code that should fix the test with your specific image. However the offsets in the |
These 3 formats have in common that they use the HEIF container format specified in ISO/IEC 23008-12. The MIME types and variant types (file extension) for HEIC and HEIF are based on sections C.2 and D.2 of the specification. A HEIF container can contain multiple images, so the logic is kept as in `:ico` (take the size of the largest image). The size of an image is found in the `ispe` box.
This PR adds support for the AVIF, HEIC and HEIF formats.
These 3 formats have in common that they use the HEIF container format specified in ISO/IEC 23008-12.
https://nokiatech.github.io/heif/technical.html also summarizes the format quite well.
The MIME types and variant types (file extension) for HEIC and HEIF are based on sections C.2 and D.2 of the ISO specification:
[...]
[...]
A HEIF container can contain multiple images, so I kept the logic as in
:ico
(take the size of the largest image).The size of an image is found in the
ispe
box:Examples here: https://mpeggroup.github.io/FileFormatConformance/?query=%3D%22ispe%22
This is also valid for AVIF per the specification: https://aomediacodec.github.io/av1-avif/latest-approved.html#image-spatial-extents-property
If you find the approach ok, I could add some tests. I have tested it with some images I have found online and compared the results with
exiftool
, but improvements are probably still possible.Let me know what you think.
closes #12