-
Notifications
You must be signed in to change notification settings - Fork 282
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
Base Media File Format #1475
Base Media File Format #1475
Conversation
…. Removed C++11 style code. Removed unused code.
…INGS_AS_ERRORS=On
Reason: clang/ubuntu fails saying "Package cmake is not available, but is referred to by another package.".
This pull request introduces 1 alert when merging f0a321d into 443944d - view on LGTM.com new alerts:
|
case TAG_avif: | ||
return "image/avif"; | ||
|
||
case TAG_heic: |
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.
Seems there is dedicated image/heic mime type: https://www.iana.org/assignments/media-types/media-types.xhtml#image
src/bmffimage.cpp
Outdated
|
||
#define TAG_ftyp ID("ftyp") /**< File type box */ | ||
#define TAG_avif ID("avif") /**< AVIF */ | ||
#define TAG_heic ID("heic") /**< HEIF */ |
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.
Typo: HEIF->HEIC
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.
These are different tags for files already produced. So no typo, even if it seems obvious.
heic means container
heif means format
heix is extended precision, i.e 10 bits per channel, however I haven't seen such files yet.
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 Canon HEIV files that Gordon sent have brand: heix. I'm hoping to setup the test environment tomorrow (2021-02-26). I don't want to drop big fat files into test/data in the repos. The test files will be stored somewhere in GitHub and downloaded when cmake -DEXIV2_ENABLE_BMFF=On is used.
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.
Bingo! I was waiting for a first encounter with 10 bpc files.
This pull request introduces 1 alert when merging 7b5854e into 443944d - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 81e0f99 into 443944d - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 4fa0a88 into 443944d - view on LGTM.com new alerts:
|
This pull request introduces 5 alerts when merging 6d13e44 into c6de72f - view on LGTM.com new alerts:
|
This pull request introduces 5 alerts when merging 630fb23 into c6de72f - view on LGTM.com new alerts:
|
@@ -41,7 +41,7 @@ The file ReadMe.txt in a build bundle describes how to install the library on th | |||
16. [Cross Platform Build and Test on Linux for MinGW](#2-16) | |||
17. [Building with C++11 and other compilers](#2-17) | |||
18. [Static and Shared Libraries](#2-18) | |||
19. [Support for bmff files (CR3, HIF, AVIF and HEIC)](#2-19) | |||
19. [Support for bmff files (CR3, HEIF and AVIF)](#2-19) |
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.
Hm, I didn't mean remove HEIC (and from main page neither below)... As per Wikipedia, I believe we should have HEIF, HEIC, and AVIF documented.
The HIF/HEIF is the same as JPG/JPEG IMHO, no need to handle specially.
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.
@clanmills Maybe I just do a PR with what I had in mind?
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.
Yes. My telepathy is down at the moment! Can you wait until this PR has been merged into 0.27-maintenance and then open a new PR.
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.
Sorry, saw this late and already pushed - just minor formatting changes though...
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.
Milos. Please don't make cosmetic changes to existing code that isn't part of the PR. It introduces changes between old-master and 0.27-maintenance which makes porting fixes from old-master more tricky. @neheb did a lot of work last Fall on 'master' and it would be good to see if we can merge his great work into 0.27-maintenance.
@neheb: I'm sorry to let you know that the Team has decided to hibernate 'master'. #1466 (comment)
@piponazo and @hassec have both proposed reformatting all the code with clang-tidy. I think we should consider doing that on 'main' when that ships as 0.28. Currently scheduled for 2021-09-15.
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.
Apologies Robin, I reverted my commits now. I'll do just the relevant subset once this PR is merged.
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.
@kmilos Thank you for both making the change and reverting the change. I know you intended well with those changes.
C'mon guys. Let's get this reviewed, approved and merged. Please log any testing you have done other than the test suite. There are 168 CR3 files (in 4.5GB) on raw.pixls.us. 26mb/image. Exiv2 reads them all in 12 seconds (on a 7 year old mac mini). 70msecs/image. It's 90% overhead (waiting for the disk?). So, the library reads > 100 images/second. Good, eh? 528 rmills@rmillsmm-local:/Users/Shared/Jenkins/Home/userContent/testfiles $ type finder
finder is aliased to `find . -depth -iname'
529 rmills@rmillsmm-local:/Users/Shared/Jenkins/Home/userContent/testfiles $ finder "*.cr3" -print0 | xargs -0 ls -l | wc
168 1950 18134
530 rmills@rmillsmm-local:/Users/Shared/Jenkins/Home/userContent/testfiles $ c=0;for i in $(finder "*.cr3" -print0 | xargs -0 ls -l | cut -d' ' -f 7); do c=$((c+i)); done; echo $(( c/(1024*1024) ))gb
4750gb
531 rmills@rmillsmm-local:/Users/Shared/Jenkins/Home/userContent/testfiles $ finder "*.cr3" -print0 | xargs -0 exiv2 -K Exif.Image.DateTime
./Pixls/Canon/Canon PowerShot G5 X Mark II/Canon_-_PowerShot_G5_X_Mark_II_-_RAW_(3:2).CR3 Exif.Image.DateTime Ascii 20 2019:10:27 10:00:47
./Pixls/Canon/Canon PowerShot G5 X Mark II/Canon_-_PowerShot_G5_X_Mark_II_-_CRAW_(1:1).CR3 Exif.Image.DateTime Ascii 20 2019:10:27 10:05:09
./Pixls/Canon/Canon PowerShot G5 X Mark II/Canon_-_PowerShot_G5_X_Mark_II_-_CRAW_(4:3).CR3 Exif.Image.DateTime Ascii 20 2019:10:27 10:02:12
...
532 rmills@rmillsmm-local:/Users/Shared/Jenkins/Home/userContent/testfiles $ time finder "*.cr3" -print0 | xargs -0 exiv2 -K Exif.Image.DateTime | wc
168 1446 23174
real 0m12.734s
user 0m1.023s
sys 0m2.582s
533 rmills@rmillsmm-local:/Users/Shared/Jenkins/Home/userContent/testfiles $ The tvisitor code from my book takes 11 seconds. That's proof that Exiv2 is as efficient as tvisitor. tvisitor is very streamlined. I don't think it's possible to read those files faster.
|
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 did not put too much attention in the implementation details, but overall the job you guys have done here looks great! 👍
// namespace extensions | ||
namespace Exiv2 | ||
{ | ||
EXIV2API bool enableBMFF(bool enable = true); |
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.
Should not this function has return type void
? I see that in the examples and documentation we do not use the returned value.
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 return value is important. It tells the caller if the code has been built with BMFF support. In the documentation and samples, we ignore the return value. However, "real" applications can use this to "gray out" a UI option such as "Use bmff support", or to post an alert such as bmff support cannot be enabled on this platform.
include/exiv2/jp2image.hpp
Outdated
@@ -42,7 +42,7 @@ namespace Exiv2 | |||
// Add JPEG-2000 to the supported image formats | |||
namespace ImageType | |||
{ | |||
const int jp2 = 15; //!< JPEG-2000 image type | |||
const int jp2 = 19; //!< JPEG-2000 image type |
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.
Why is the new type using the value 15 and jp2
value has been updated from 15 to 19? Was not possible to keep the previous values for jp2 and use a new one for BMFF?
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.
15 is used by bmff, and 19 by jp2.
Let's replace that with an enum in 'main'. I didn't want change it to an enum for a "dot" because if it's not broken, don't fix it. However, when we get to 'main/0.28' we can fix this.
509 rmills@rmillsm1:~/gnu/github/exiv2/bmff $ grep 'const int' include/exiv2/*image.hpp
include/exiv2/bmffimage.hpp: const int bmff = 15; //!< BMFF (bmff) image type (see class BMFF)
include/exiv2/bmpimage.hpp: const int bmp = 14; //!< Windows bitmap (bmp) image type (see class BmpImage)
include/exiv2/cr2image.hpp: const int cr2 = 7; //!< CR2 image type (see class Cr2Image)
include/exiv2/crwimage.hpp: const int crw = 3; //!< CRW image type (see class CrwImage)
include/exiv2/epsimage.hpp: const int eps = 18; //!< EPS image type
include/exiv2/gifimage.hpp: const int gif = 11; //!< GIF image type (see class GifImage)
include/exiv2/image.hpp: const int none = 0; //!< Not an image
include/exiv2/jp2image.hpp: const int jp2 = 19; //!< JPEG-2000 image type
include/exiv2/jpgimage.hpp: const int jpeg = 1; //!< JPEG image type (see class JpegImage)
include/exiv2/jpgimage.hpp: const int exv = 2; //!< EXV image type (see class ExvImage)
include/exiv2/mrwimage.hpp: const int mrw = 5; //!< MRW image type (see class MrwImage)
include/exiv2/orfimage.hpp: const int orf = 9; //!< ORF image type (see class OrfImage)
include/exiv2/pgfimage.hpp: const int pgf = 17; //!< PGF image type (see class PgfImage)
include/exiv2/pngimage.hpp: const int png = 6; //!< PNG image type (see class PngImage)
include/exiv2/psdimage.hpp: const int psd = 12; //!< Photoshop (PSD) image type (see class PsdImage)
include/exiv2/rafimage.hpp: const int raf = 8; //!< RAF image type (see class RafImage)
include/exiv2/rw2image.hpp: const int rw2 = 16; //!< RW2 image type (see class Rw2Image)
include/exiv2/tgaimage.hpp: const int tga = 13; //!< Truevision TARGA (tga) image type (see class TgaImage)
include/exiv2/tiffimage.hpp: const int tiff = 4; //!< TIFF image type (see class TiffImage)
include/exiv2/tiffimage.hpp: const int dng = 4; //!< DNG image type (see class TiffImage)
include/exiv2/tiffimage.hpp: const int nef = 4; //!< NEF image type (see class TiffImage)
include/exiv2/tiffimage.hpp: const int pef = 4; //!< PEF image type (see class TiffImage)
include/exiv2/tiffimage.hpp: const int arw = 4; //!< ARW image type (see class TiffImage)
include/exiv2/tiffimage.hpp: const int sr2 = 4; //!< SR2 image type (see class TiffImage)
include/exiv2/tiffimage.hpp: const int srw = 4; //!< SRW image type (see class TiffImage)
include/exiv2/webpimage.hpp: const int webp = 23; //!< Treating webp as an image type>
510 rmills@rmillsm1:~/gnu/github/exiv2/bmff $
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've realised during lunch that you're asking "why not leave jp2 unchanged and set bmff=19?". When I started working with Peter, the only way I could change anything was to send him a patch and that's what I did. I didn't want to discuss what that code means, so I accepted his 15 (even though it's wrong).
You're right. Now that we're cleaning up and the end, I should restore jp2 = 15 and set bmff = 19. Good Catch.
return result; | ||
} | ||
|
||
long BmffImage::boxHandler(std::ostream& out /* = std::cout*/ , Exiv2::PrintStructureOption option /* = kpsNone */,int depth /* =0 */) |
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 guess the comments can be removed from here, 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.
Comments? Are you asking me to add comments? I've put comments in the declaration in bmffimage.hpp
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 am not asking you to add comments. It looks like the IDE you were using added those default values:
/* = std::cout*/
/* = kpsNone */
from the .h and commented them out in the .cpp. If possible, it would be good to remove them from the .cpp.
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.
Ah, right. It's a common convention to repeat the defaults in the cpp. Of course that have to be in comments or the compiler gets upset.
I'll change that. NP.
@clanmills |
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 PR adds three test cases: tests/bugfixes/github/test_pr1475_*.py
And two functions, runTest()
, verbose_version()
, have been added to tests/system_tests.py
without affecting the existing test code.
I've passed the test cases:
test_run (test_pr1475_AVIF.pr_1475_avif_avif) ... ok
test_run (test_pr1475_AVIF.pr_1475_exif_xmp_avif) ... ok
test_run (test_pr1475_AVIF.pr_1475_metadata2_avif) ... ok
test_run (test_pr1475_HEIC.pr_1475_2021_heic) ... ok
test_run (test_pr1475_HEIC.pr_1475_IMG_3578_heic) ... ok
test_run (test_pr1475_HEIC.pr_1475_Stonehenge_heic) ... ok
test_run (test_pr1475_HEIC.pr_1475_heic_heic) ... ok
test_run (test_pr1475_HIF.pr_1475_Canon_hif) ... ok
test_run (test_pr1475_HIF.pr_1475_Sony_hif) ... ok
@LeoHsiao1 Thanks for looking at this. I'll look at your changes later today. BTW. I was working on the release notes for v0.27.4 this morning and of course you get acknowledged. By the end of this week, I hope to have removed the legacy bash tests and your code with have the responsibility for performing this important test activity. Make and bash will be no longer required to test msvc builds. #1018 (comment). |
@LeoHsiao1 I can see you've simplified the python code. Thanks. I'm not sure what is going on in git. I don't see a difference anywhere. The important thing is that the simpler/modified code works. I've thought of a possible bug. The environment string EXIV2_BINDIR can be used to specify the location of the exiv2. My code to modify PATH and LD_LIBRARY_PATH doesn't use EXIV2_BINDIR. However, the code could be correct because Dan's file tests/suite.conf locates exiv2. Could you investigate this and let me know. Thanks. |
src/futils.cpp
Outdated
size_t input_length = in ? ::strlen(in) : 0; | ||
if (!in || input_length % 4 != 0) return result; | ||
|
||
unsigned char encoding_table[]= {'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', |
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 think it would be nice if w can move this table out of this function and share it with the encode
function, no?
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.
Yes. I thought of that. In decode it's a char**
and in encode it's unsigned char**
(or the other way). And I thought "what happens if this is exposed as an external in the library?". So, I thought leave it alone - it's working.
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.
Since we are in a .cpp
we can just put it into an anonymous namespace and wouldn't have to worry about leaking 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.
That's probably right. I'm worrying about nothing. Tomorrow I'll move encoding_table into a static outside the encode and decode functions.
@LeoHsiao1. Can you put my code into your file I've thought of several things which we need concerning test for v0.27.4. Can you deal with the following and submit a separate PR as the following has nothing to do with bmff support.
$ make unit_test or $ cmake --build . --target unit_test # runs bin/unit_tests
$ make bash_tests or $ cmake --build . --target bash_tests # runs your code in tests/bash_tests
$ make python_tests or $ cmake --build . --target python_tests # runs Dan's code in tests/bugfixes
$ make version_test or $ cmake --build . --target version_test # runs and formats output of exiv2 --verbose --version
$ make tests or $ cmake --build . --target tests # runs every test
I don't need this done today. However if this could be done at the weekend, it'll save me some time. And I really want you to totally understand and own the test code in Exiv2. You can do it. |
…xiv2::enableBMFF().
…oth Exiv2::base64encode() and Exiv2::base64decode(). I'm a little uneasy about changes to the API of the shared object/DLL. I will investigate Exiv2#890 during the release process. So I have a plan to identify this if it's an issue. I think it's OK. ``` 703 rmills@rmillsmm-local:~/gnu/github/exiv2/bmff/build $ nm --demangle lib/libexiv2.dylib | grep base64 00000000000384a0 T Exiv2::base64decode(char const*, char*, unsigned long) 0000000000038340 T Exiv2::base64encode(void const*, unsigned long, char*, unsigned long) 00000000001c33a0 s Exiv2::base64_encode 704 rmills@rmillsmm-local:~/gnu/github/exiv2/bmff/build $ nm -g --demangle lib/libexiv2.dylib | grep base64 00000000000384a0 T Exiv2::base64decode(char const*, char*, unsigned long) 0000000000038340 T Exiv2::base64encode(void const*, unsigned long, char*, unsigned long) 705 rmills@rmillsmm-local:~/gnu/github/exiv2/bmff/build $ ```
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 is it. Time to merge. Thank you to @alexvanderberkel @hassec @piponazo @LeoHsiao1 @kmilos and @1div0. Great Team Work by Everybody.
Shit. There's something horribly wrong and clearly caused by a collision with #1482. Git strikes again! [ 41%] Building CXX object unitTests/CMakeFiles/unit_tests.dir/test_LangAltValueRead.cpp.o
/Users/rmills/gnu/github/exiv2/0.27-maintenance/unitTests/test_LangAltValueRead.cpp:21:35: error: no member named 'kerInvalidLangAltValue' in namespace 'Exiv2'
ASSERT_EQ(e.code(),Exiv2::kerInvalidLangAltValue); I'll investigate and fix it this morning. |
Storm in a tea-cup. Deleted the build directory and generated a new environment. Everything appears to be fine on macOS. I'll use the build server to build/test every platform (including UNIX) this morning. |
Forgot to create PR against 0.27-maintenance instead of master. Sorry for the noise.