Fix pixelwidth and pixelheight for raf files #810

astippich · 2019-04-28T16:17:22Z

Extract the pixel width and pixel height of raf images from the raf metadata found in fuji raw files.
Code inspired by exiftool

Fixes #755

codecov · 2019-04-28T17:27:21Z

Codecov Report

Merging #810 into master will increase coverage by 0.05%.
The diff coverage is 21.73%.

@@            Coverage Diff             @@
##           master     #810      +/-   ##
==========================================
+ Coverage   70.94%   70.99%   +0.05%     
==========================================
  Files         147      147              
  Lines       19268    19306      +38     
==========================================
+ Hits        13669    13706      +37     
- Misses       5599     5600       +1

Impacted Files	Coverage Δ
src/rafimage.cpp	`16.25% <21.73%> (-3%)`	⬇️
src/tiffimage_int.cpp	`90.64% <0%> (-0.59%)`	⬇️
src/tiffvisitor_int.cpp	`87.36% <0%> (-0.11%)`	⬇️
src/tags_int.cpp	`87.1% <0%> (ø)`	⬆️
src/exiv2.cpp	`100% <0%> (ø)`	⬆️
src/params.cpp	`73.61% <0%> (ø)`	⬆️
src/exif.cpp	`81.09% <0%> (ø)`	⬆️
src/tags_int.hpp	`62.5% <0%> (ø)`	⬆️
src/params.hpp	`100% <0%> (ø)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70a1b1b...982a8a7. Read the comment docs.

piponazo

We would need to have a new system test (check tests under the tests folder) to make sure that your changes are fixing the issue.

Normally we create first a python test for reproducing the detected issue and then you can apply the fix (preferentially the fix and the test in different commits). In #792 you can find an example of this procedure. Note that for providing a sample image, we remove the image content and we just keep the metadata.

src/rafimage.cpp

D4N

Thanks for your contribution!

I've added some comments myself. A test case would be nice as @piponazo noted.

D4N · 2019-04-29T13:23:16Z

src/rafimage.cpp

@@ -296,6 +292,37 @@ namespace Exiv2 {
        io_->read(jpg_img_length, 4);
        long jpg_img_off = Exiv2::getULong((const byte *) jpg_img_offset, bigEndian);
        long jpg_img_len = Exiv2::getULong((const byte *) jpg_img_length, bigEndian);
+        byte cfa_header_offset [4];
+        io_->read(cfa_header_offset, 4);


Before doing all these reads, please check that the image is actually large enough, or check the return value of each io_->read().

D4N · 2019-04-29T13:32:13Z

src/rafimage.cpp

+        io_->read(byte_count, 4);
+        byte byte_tag[2];
+        byte byte_size[2];
+        int32_t count = getLong(byte_count, bigEndian);


Please add a sanity check for this value, otherwise this will create a near-infinite loop.

What is a sane value for you? The used test file has 32 tags, but I have no clue if other RAF files have a different number of raf tags.

If there is no upper limit mandated by a standart, then you can make a rough check like: enforce(count < remaining_file_size/size_of_a_tag);, provided that this is at least an order of magnitude less than the maximum value of int32_t for all reasonable files.

If I understand your code, then aren't you searching for the pixelwidth and pixelheight inside the cfa_hdr? And you've just extracted its length a few lines above, so given the tag size and the length, that gives you a pretty good upper limit on the number of tags that can possibly exist in there.

tag size is not constant according to exiftool, but I guess I could add a check with the minimal size for at least something

src/rafimage.cpp

astippich · 2019-04-29T20:40:54Z

Thanks for the feedback. I will try to implement it, but I am quite busy the next weeks.

Regarding the unit test, I expected and somewhat feared this request :)

I implemented it for a user of KDE's KFileMetaData and tested it using the user's file (https://bugs.kde.org/show_bug.cgi?id=380919) which I would rather not like to upload as a test file, so I basically have no test file for this
Since these are raw file they are quite big. I do not know how to reduce the file size

Any suggestions welcome.

D4N · 2019-04-29T20:49:09Z

Thanks for the feedback. I will try to implement it, but I am quite busy the next weeks.

No problem & no rush.

Regarding the unit test, I expected and somewhat feared this request :)

You don't have to write a unit test, an integration test would be easier imho. We have a custom integration test framework written in Python (take a look at the Python files in tests/bugfixes/). You essentially just feed it a exiv2 invocation and the expected output and the framework does the rest for you.

I implemented it for a user of KDE's KFileMetaData and tested it using the user's file (https://bugs.kde.org/show_bug.cgi?id=380919) which I would rather not like to upload as a test file, so I basically have no test file for this

Since these are raw file they are quite big. I do not know how to reduce the file size

exiv2 -ex $your_image will extract the metadata from the image and drop all pixel contents, so unless they put their credit card number in the metadata, it should be fine to upload. You might want to check the files contents nevertheless.

Any suggestions welcome.

astippich · 2019-05-26T07:44:53Z

exiv2 -ex $your_image will extract the metadata from the image and drop all pixel contents, so unless they put their credit card number in the metadata, it should be fine to upload. You might want to check the files contents nevertheless.

That does not seem to work, probably because it is a raw file and the size information is also in the custom RAF metadata. I have a RAF image which the user granted me the rights to upload, but it is 31 MB. Is this a problem?

D4N · 2019-05-29T02:59:44Z

I have a RAF image which the
user granted me the rights to upload, but it is 31 MB. Is this a problem?

Kind of, we do not want to add a 31 MB file to git and even for git LFS it's
quite a huge file (I wouldn't want too many files larger than 1 MB, so that we
won't hit the 2GB limit anytime soon).

Is it possible to scale this file down, e.g. to 100x100 px?

astippich · 2019-06-01T08:38:45Z

I have a RAF image which the
user granted me the rights to upload, but it is 31 MB. Is this a problem?

Kind of, we do not want to add a 31 MB file to git and even for git LFS it's
quite a huge file (I wouldn't want too many files larger than 1 MB, so that we
won't hit the 2GB limit anytime soon).

Is it possible to scale this file down, e.g. to 100x100 px?

According to the user, no (I asked for lowest quality and resolution). Again, probably because it is a raw file. I found https://rawsamples.ch/index.php/en/fuji which is content provided under Creative Commons and provides a 6 MB raf file. Is this acceptable?

D4N · 2019-06-03T23:03:54Z

According to the user, no (I asked for lowest quality and resolution). Again,
probably because it is a raw file. I found
https://rawsamples.ch/index.php/en/fuji which is content provided under Creative
Commons and provides a 6 MB raf file. Is this acceptable?

Unfortunately 6 MB is still too much to be checked into git.

I've created #896 to enable git lfs, so that we can add your test file.

astippich · 2019-06-20T14:59:51Z

Finally found some time again to work on this. I've rebased and added error handling as requested.
As a consequence, the test for issue 857 failed differently, and I've adjusted the test.

piponazo · 2019-06-20T20:51:44Z

Thanks @astippich for the contribution. The changes look good to me (the formatting is not satisfying the clang-format style we have defined, but that's something we could clean-up later, unless you do not mind to add an additional commit applying clang-format to your additions [not to the complete file]).

However as @D4N already mentioned, we should not include such a big binary file in the repository. Until we add support for git LFS, or other strategy to deal with such big files, I would propose to drop the commit including the test and the binary file. Maybe we could keep a patch of that commit in this PR for future reference. Or if you guys have any other better idea, it will be welcomed.

tests/bugfixes/github/test_issue_755.py

src/rafimage.cpp

D4N

This looks better now, thanks for the changes.

Please add an explanation to the chunk readMetadata() what you are doing there (something high level and if it makes sense, link to the issue that you reported).
Consider using the --grep function from exiv2 in the test file to grep for the relevant metadata only.

D4N · 2019-06-20T23:10:46Z

However as @D4N already mentioned, we should not include such a big binary file in the repository. Until we add support for git LFS, or other strategy to deal with such big files, I would propose to drop the commit including the test and the binary file. Maybe we could keep a patch of that commit in this PR for future reference. Or if you guys have any other better idea, it will be welcomed.

Not really, beside waiting until we enable git LFS.

astippich · 2019-06-23T18:17:19Z

This looks better now, thanks for the changes.

* Please add an explanation to the chunk `readMetadata()` what you are doing there (something high level and if it makes sense, link to the issue that you reported).

I added a sentence to the code, if you want more let me know

* Consider using the `--grep ` function from `exiv2` in the test file to grep for the relevant metadata only.

I tried that, but could not find any documentation on what tag name to use. Could it be that this also works only for Exif tag names?

I will open a separate PR for the test so that you can merge when the infrastructure is ready.

D4N · 2019-07-08T03:49:30Z

I added a sentence to the code, if you want more let me know

I was hoping for a something that will explain the future maintainer (i.e. me or someone else from the team) how the format looks like that you are parsing. Currently, I have to read all your code and try to find out what it does. If you could explain that for someone that doesn't have your knowledge, that would be greatly appreciated.

I tried that, but could not find any documentation on what tag name to use. Could it be that this also works only for Exif tag names?

@clanmills do you know that?

I will open a separate PR for the test so that you can merge when the infrastructure is ready.

I think setting up git lfs has been long overdue, I'll try to look into that soon.

Pull request has been modified.

astippich · 2019-07-28T09:29:06Z

Anything to do here from my side?

piponazo · 2019-07-28T09:33:51Z

Sorry for the delay on getting this merged @astippich . I'll let @D4N take care of merging it, since the PR is pending on his approval.

D4N

Thanks for your patience and your effort!

piponazo requested changes Apr 28, 2019

View reviewed changes

src/rafimage.cpp Outdated Show resolved Hide resolved

src/rafimage.cpp Outdated Show resolved Hide resolved

D4N requested changes Apr 29, 2019

View reviewed changes

D4N mentioned this pull request Jun 3, 2019

Enable git LFS #896

Open

astippich force-pushed the raf_size branch 2 times, most recently from af8269e to 072f910 Compare June 20, 2019 14:57

D4N reviewed Jun 20, 2019

View reviewed changes

tests/bugfixes/github/test_issue_755.py Outdated Show resolved Hide resolved

D4N reviewed Jun 20, 2019

View reviewed changes

tests/bugfixes/github/test_issue_755.py Outdated Show resolved Hide resolved

D4N reviewed Jun 20, 2019

View reviewed changes

src/rafimage.cpp Outdated Show resolved Hide resolved

D4N requested changes Jun 20, 2019

View reviewed changes

astippich force-pushed the raf_size branch from 072f910 to 28aec8b Compare June 23, 2019 18:19

astippich added 2 commits June 25, 2019 20:50

read image width and height from RAF metadata

738b630

fix test for issue 857

1e869f6

astippich force-pushed the raf_size branch from 28aec8b to 1e869f6 Compare June 25, 2019 19:00

astippich mentioned this pull request Jun 25, 2019

Add test for issue 755 #930

Open

piponazo previously approved these changes Jun 27, 2019

View reviewed changes

piponazo requested a review from D4N June 27, 2019 18:58

document raf metadata structure

982a8a7

denzp mentioned this pull request Jul 9, 2019

Fujifilm RAW Exposure Bias darktable-org/rawspeed#198

Closed

piponazo approved these changes Jul 28, 2019

View reviewed changes

D4N approved these changes Jul 28, 2019

View reviewed changes

D4N merged commit 98e63e4 into Exiv2:master Jul 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pixelwidth and pixelheight for raf files #810

Fix pixelwidth and pixelheight for raf files #810

astippich commented Apr 28, 2019

codecov bot commented Apr 28, 2019 •

edited

Loading

piponazo left a comment

D4N left a comment

D4N Apr 29, 2019

D4N Apr 29, 2019

astippich Jun 20, 2019

D4N Jun 20, 2019

astippich Jun 25, 2019

astippich commented Apr 29, 2019

D4N commented Apr 29, 2019 •

edited

Loading

astippich commented May 26, 2019

D4N commented May 29, 2019

astippich commented Jun 1, 2019

D4N commented Jun 3, 2019

astippich commented Jun 20, 2019

piponazo commented Jun 20, 2019

D4N left a comment

D4N commented Jun 20, 2019

astippich commented Jun 23, 2019 •

edited

Loading

D4N commented Jul 8, 2019

astippich commented Jul 28, 2019

piponazo commented Jul 28, 2019

D4N left a comment

Fix pixelwidth and pixelheight for raf files #810

Fix pixelwidth and pixelheight for raf files #810

Conversation

astippich commented Apr 28, 2019

codecov bot commented Apr 28, 2019 • edited Loading

Codecov Report

piponazo left a comment

Choose a reason for hiding this comment

D4N left a comment

Choose a reason for hiding this comment

D4N Apr 29, 2019

Choose a reason for hiding this comment

D4N Apr 29, 2019

Choose a reason for hiding this comment

astippich Jun 20, 2019

Choose a reason for hiding this comment

D4N Jun 20, 2019

Choose a reason for hiding this comment

astippich Jun 25, 2019

Choose a reason for hiding this comment

astippich commented Apr 29, 2019

D4N commented Apr 29, 2019 • edited Loading

astippich commented May 26, 2019

D4N commented May 29, 2019

astippich commented Jun 1, 2019

D4N commented Jun 3, 2019

astippich commented Jun 20, 2019

piponazo commented Jun 20, 2019

D4N left a comment

Choose a reason for hiding this comment

D4N commented Jun 20, 2019

astippich commented Jun 23, 2019 • edited Loading

D4N commented Jul 8, 2019

astippich commented Jul 28, 2019

piponazo commented Jul 28, 2019

D4N left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 28, 2019 •

edited

Loading

D4N commented Apr 29, 2019 •

edited

Loading

astippich commented Jun 23, 2019 •

edited

Loading