Fix 582 Add support for FocusPosition in Sony RAW files #906
Conversation
Codecov Report
@@ Coverage Diff @@
## 0.27-maintenance #906 +/- ##
====================================================
+ Coverage 62.88% 62.89% +0.01%
====================================================
Files 156 156
Lines 21606 21626 +20
====================================================
+ Hits 13586 13602 +16
- Misses 8020 8024 +4
Continue to review full report at Codecov.
|
|
I would like to see this integrated into Exiv2 v0.27.2.1 (Exiv2 v0.27.2-RC1). It is not presenting the correct values at present and here are the symptoms: It appears to be decoding SHORTS from the wrong location. I've stepped the binary decoder |
|
Sure, Dan. There's a great deal of discussion with Phil Harvey (ExifTool) about that function here: #582 |
There was a problem hiding this comment.
Great contribution @clanmills !
I am approving already, although I made two tiny suggestions.
doc/templates/Makefile
Outdated
| @@ -98,7 +98,7 @@ TABLES = Exif \ | |||
| OlympusFi \ | |||
| OlympusFe1 \ | |||
| OlympusRi \ | |||
| Panasonic \ | |||
| Panasonic \ | |||
There was a problem hiding this comment.
Ups, this line is not aligned properly. It seems the issue is a tab character at the beginning.
There was a problem hiding this comment.
You're right. If I was granted one wish, I'd want the tab character to be made illegal in all source code, scripts, makefiles and documentation. It's a horror. An invisible character without a definition that can end up on every line of code.
src/sonymn_int.cpp
Outdated
| } | ||
|
|
||
| // code byte-by-byte | ||
| uint32_t i = 0; |
There was a problem hiding this comment.
[suggestion] I think that by replacing the while-loop by a for-loop the code could be reduced 2 lines.
There was a problem hiding this comment.
You're right. I've changed it.
I still have the puzzle of encoding this data which I believe is causing the .exv file to be incorrect. I will investigate that. However, let's get this PR into v0.27.2 because it's good for reading the data which is the priority for @cryptomilk
I believe sonyFpCrypt() is a symmetric encrypt/decrypt. The structure ArrayCfg has an crypt entry. However it's not clear how the function knows if it's encrypting or decrypting and I don't want to change the API.
There was a problem hiding this comment.
I believe sonyFpCrypt() is a symmetric encrypt/decrypt
In the ExifTool Decipher() routine, the $encipher parameter is a flag which is true to reverse the direction of the cipher (ie. encipher instead of decipher the data). When you had this line of code:
code[i] = (i * i * i) % 249 ;
you were enciphering. When you switched it to this:
code[(i * i * i) % 249] = i;
you were deciphering.
There was a problem hiding this comment.
That's very useful, Phil. There is a bug in my implementation when I write those tags. I'm sure it's because writeMetadata() does not cipher the data. I will try to fix that now with this insight.
There was a problem hiding this comment.
YES. Thanks that's fixed the write in TiffEncoder::visitBinaryArray(). Muchy thanky.
I'll update the explanation I posted this morning to add a parameter 'decipher/encipher' to the utility sony_decipher (which will be renamed sony_cipher). Very pleased about this. Thank You very much for you help.
|
@clanmills do you intend to add some documentation for the |
Please at least reference the discussion in the issue in the source code, so that next person that will touch this doesn't have to go through all the work that you guys did in #582 . |
|
Thank You @piponazo and @D4N for your thoughtful comments. #582 is a very long discussion. I will put a summary here tomorrow and I hope you'll be happy to push this to v0.27.2. I believe we are decoding the data correctly and will please @cryptomilk. I still have matters to investigate about re-writing this data. |
|
Yes, I'm looking forward to it :-) |
|
Andreas (@cryptomilk) provided a test file which is available on the build-server: Tag 0x9402 is stored in the MakerNote. It's ciphered. We can locate the data with exiv2 as follows: And extract it with dd: We can extract and decode it with dd and sony_cipher: And of course decifer/encipher get you back to where you started: The code for sony_cipher.cpp is: The code for dmpf.cpp (my homemade file dump utility): If you want to know how Phil Harvey (@boardhead) discovered the |
|
Do you have access to https://raw.pixls.us/ ? If not you should talk to Pat or Mica to get access. |
|
Sure. I know Pat quite well. I met him at LGM in London and he helped a lot when I gave a presentation about Exiv2 in Rio in 2017. Very nice guy. Would like to visit him and his family one day in Alabama. When I make releases of Exiv2, I announce them on Facebook and Pixls. If you have suggestions about other places on which to make announcements, I'll be happy to update our release procedure to include them. I know that the Pixls community have an archive of raw files. One day we'll integrate that into our test suite. |
|
Robin wrote:
I suggest using the term "enciphered" for this tag. Sony has other tags which use a true (and very different) encryption. To avoid confusion, I reserve the word "encrypted" for these.
|
|
@boardhead Thanks. I've updated the comment I posted earlier today AND updated the PR to rename the function |
…e. Fixed typos.
Sadly, ArrayCfg/crpyt does not know if he's encrypting/decrypting. I've added a sniff in TiffEncoder::visitBinaryArrayEnd to avoid changing the API.
src/sonymn_int.hpp
Outdated
|
|
||
| }; // class SonyMakerNote | ||
|
|
||
| DataBuf sonyTagCipher (uint16_t, const byte*, uint32_t, TiffComponent* const, bool bCipher); |
There was a problem hiding this comment.
This function can very well be static in the .cpp, couldn't it?
There was a problem hiding this comment.
Yes, you're right, it could be static. It started of life as a single decipher function and grown. sonyTagCipher does not need to be visible outside of sonymn_int.cpp.
There was a problem hiding this comment.
I'm going to update the PR to be sure sonyTagCipher() doesn't end up in visible symbols. I think you'll have to approve that change. I understand what "squashing commits" does, however I've never done it.
There was a problem hiding this comment.
2335 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/build $ nm -g --demangle lib/libexiv2.0.27.2.1.dylib | grep sonyTag
00000000001c75a0 T Exiv2::Internal::sonyTagCipher(unsigned short, unsigned char const*, unsigned int, Exiv2::Internal::TiffComponent*, bool)
00000000001c77b0 T Exiv2::Internal::sonyTagDecipher(unsigned short, unsigned char const*, unsigned int, Exiv2::Internal::TiffComponent*)
00000000001c7890 T Exiv2::Internal::sonyTagEncipher(unsigned short, unsigned char const*, unsigned int, Exiv2::Internal::TiffComponent*)
2336 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/build $ ce ../src/sonymn_int.cpp
bbedit "../src/sonymn_int.cpp"
2337 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/build $ make
[ 10%] Built target exiv2-xmp
...
2338 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/build $ nm -g --demangle lib/libexiv2.0.27.2.1.dylib | grep sonyTag
00000000001c75a0 T Exiv2::Internal::sonyTagDecipher(unsigned short, unsigned char const*, unsigned int, Exiv2::Internal::TiffComponent*)
00000000001c7890 T Exiv2::Internal::sonyTagEncipher(unsigned short, unsigned char const*, unsigned int, Exiv2::Internal::TiffComponent*)
There was a problem hiding this comment.
Robin, you did not push that change yet, right?
* Fix 582 Add support for FocusPosition in Sony RAW files * Thanks to @boardhead sonyFpCrypt() works correctly. Removed debug code. Fixed typos. * Update doc/templates/Makefile to process Sony2Fp * Following review by @boardhead. Renamed sonyFpCrypt() as sonyTagDecipher(). * Fixed writing the tag thanks to @boardhead explaining encipher/decipher. Sadly, ArrayCfg/crpyt does not know if he's encrypting/decrypting. I've added a sniff in TiffEncoder::visitBinaryArrayEnd to avoid changing the API. * Added URL to discussion concerning sonyTagCipher() * make sonyTagCipher() a static function with no external visibility. (cherry picked from commit ab375fb)
|
Awesome, thanks you! |
* Fix 582 Add support for FocusPosition in Sony RAW files * Thanks to @boardhead sonyFpCrypt() works correctly. Removed debug code. Fixed typos. * Update doc/templates/Makefile to process Sony2Fp * Following review by @boardhead. Renamed sonyFpCrypt() as sonyTagDecipher(). * Fixed writing the tag thanks to @boardhead explaining encipher/decipher. Sadly, ArrayCfg/crpyt does not know if he's encrypting/decrypting. I've added a sniff in TiffEncoder::visitBinaryArrayEnd to avoid changing the API. * Added URL to discussion concerning sonyTagCipher() * make sonyTagCipher() a static function with no external visibility. (cherry picked from commit ab375fb)
Sony2 Tag 0x9402 handler:
https://sno.phy.queensu.ca/~phil/exiftool/TagNames/Sony.html#Tag9402