Proposed patch to fix Bug 383520: Gwenview Importer doesn't use date/time from EXIF
ClosedPublic

Authored by vtasoulas on Aug 15 2017, 8:28 AM.

Details

Summary

Gwenview Importer only uses date/time from the filesystem metadata when renaming imported photos.
Not the EXIF information that holds the correct date/time that the picture has been taken.

After looking into the code, I found out that the datetime is determined by calling the function 'TimeUtils::dateTimeForFileItem'.

Consequently, the function 'TimeUtils::dateTimeForFileItem' is calling the function 'updateFromExif()' which in turn calls the function 'Exiv2ImageLoader::load' that returns 'false'. Then it falls back at reading the datetime from the filesystem metadata.

After printing out the error messages that cause the function 'Exiv2ImageLoader::load' to return 'false', I found that the Exiv2 error message is 'Failed to read image data' (doesn't help much). However, after I started playing with the size of the 'QByteArray header' (at the line where the file is read) that is passed as an argument to the 'Exiv2ImageLoader::load' function, I observed different errors until I managed to read the exif info from the file properly.

Some example sizes and different reported errors by Exiv2:

65536: Failed to read image data
66000: JPEG format error, rc = 5
131072 It reads fine. No error.

Didn't check numbers between 66000 and 131072 in order to find the exact threshold where I don't get a problem/error reading the exif information, but why should gwenview even bother about this number since exiv2 can do the job if we only provide the url (filepath) of the file to the Exiv2::ImageFactory::open() function? I guess exiv2 probably does the image reading in an even more efficient way according to the JPEG (or other) standard specifications (other than passing it a fixed number of bytes from an image as gwenview currently does).

So attached you can find a proposed patch that does exactly that: Adds a second instance of the function "Exiv2ImageLoader::load()" that acceps a "QString filePath" parameter and lets the Exiv2 library to handle the rest based on the given filePath.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
vtasoulas created this revision.Aug 15 2017, 8:28 AM
vtasoulas updated this revision to Diff 18169.Aug 15 2017, 9:53 AM
vtasoulas edited the summary of this revision. (Show Details)

In the first patch I had just changed the accepted parameter of the "Exiv2ImageLoader::load()" function from QByteArray& to QString.
But code that expects this function to accept a QByteArray exists in different parts of the software. In this updated patch I just added a second instance of the function "Exiv2ImageLoader::load()" without removing the original one.

cfeck added a subscriber: cfeck.Aug 30 2017, 9:52 PM
cfeck added inline comments.Aug 30 2017, 9:58 PM
lib/exiv2imageloader.cpp
57

I suggest to use QFile::encodeName(), even if its documentation says it uses the local 8 bit encoding.

59

.constData()

lib/exiv2imageloader.h
54

const QString&

vtasoulas updated this revision to Diff 19025.Aug 31 2017, 6:32 PM
vtasoulas marked 3 inline comments as done.

Incorporated the changes suggested by Christoph Feck.

@vtasoulas Is there any step-by step test plan to verify that a 1) bug exists and 2) that it is fixed by the patch?

I shoot with a Nikon D300 which clock is configured (usually) to UTC+1.

I also use Gwenview importer with rename format {date}_{time}_{name.lower}.{ext.lower} to copy and rename my photographs from a card reader to my computer.

Unfortunately, the filename of the renamed pictures contains the date/time that I see when I ls the image (ls -l picture.jpg).
However, the EXIF info of the image has a different (the correct) date/time.

After I did some debugging as explained in the summary, exiv2 was giving me a "Failed to read image data" error.

I tested this in a KDE Neon User edition installation.

I also use Gwenview importer with rename format {date}_{time}_{name.lower}.{ext.lower} to copy and rename my photographs from a card reader to my computer.
Unfortunately, the filename of the renamed pictures contains the date/time that I see when I ls the image (ls -l picture.jpg).
However, the EXIF info of the image has a different (the correct) date/time.

Sorry, I'm not a gwenview user; is it Pugins -> Import -> Import from a remote storage? So, do I need a RAW file with EXIF data to verify? Do you have an example file?
At least the patch compiles fine for me and gwenview continues to work as before :)

Here is a link with two JPG photographs: https://www.dropbox.com/s/ec7gmoerq4xf95a/test_gwenview_importer.tar?dl=0

dsc_1107.jpg and DSC_2843.JPG. The renaming is working fine for dsc_1107.jpg, but not working as it should for DSC_2843.JPG.

In my system, this is what the stat command prints (pay attention to the Modify time).

$ stat ~/test_gwenview_importer/dsc_1107.jpg 
  File: 'test_gwenview_importer/dsc_1107.jpg'
  Size: 2892298         Blocks: 5664       IO Block: 4096   regular file
Device: fc01h/64513d    Inode: 35127600    Links: 1
Access: (0644/-rw-r--r--)  Uid: ( 1000/   cyber)   Gid: ( 1000/   cyber)
Access: 2017-09-10 13:22:27.226758182 +0200
Modify: 2015-12-06 16:33:38.000000000 +0100
Change: 2017-09-10 13:20:34.662054889 +0200
 Birth: -

$ stat ~/test_gwenview_importer/DSC_2843.JPG 
  File: 'test_gwenview_importer/DSC_2843.JPG'
  Size: 3498246         Blocks: 6848       IO Block: 4096   regular file
Device: fc01h/64513d    Inode: 35141174    Links: 1
Access: (0644/-rw-r--r--)  Uid: ( 1000/   cyber)   Gid: ( 1000/   cyber)
Access: 2017-09-10 13:18:03.428338283 +0200
Modify: 2012-01-15 10:08:50.000000000 +0100
Change: 2017-09-10 13:17:44.556035328 +0200
 Birth: -

Then I run the gwenview_importer to import these two photographs to another folder. Typically, the source location of these two photos would be in a card reader in my CF, and I would want to use gwenview_importer to transfer them to my hard drive.

So assuming that you untared the folder test_gwenview_importer is in your home directory, run the command gwenview_importer ~/test_gwenview_importer/ to start the gwenview_importer app.

Click the Settings button and choose the Rename Format to be {date}_{time}_{name.lower}.{ext.lower}.

Then choose your import destination to be ~/test_gwenview_importer_destination.

Hit the Import All button...

First of all, the output in the command line window where you started gwenview_importer prints the following

$ gwenview_importer ~/test_gwenview_importer/
Icon theme "elementary" not found.
Unhandled role 4
Unhandled role 10
Unhandled role 4
Unhandled role 10
Warning: JPEG format error, rc = 5

Pay attention to the last line (the Warning: JPEG format error, rc = 5). This is the indication that there is a problem reading the exif from one of the files as I described in my initial summary.

The resulting filenames now are the following two:
~/test_gwenview_importer_destination/2013-03-17_22-53-23_dsc_1107.jpg <- This has been correctly renamed from the EXIF information. If you compare the stat times, you cannot find the 22:53:23 time that was used in the renamed filename. This time has been correctly picked from the EXIF information of the image.

~/test_gwenview_importer_destination/2012-01-15_10-08-50_dsc_2843.jpg <- For this one, the date/time used for renaming is the Modify time of the image (look at the stat output above). If you check the exif info of this file, the picture was captured at 2012:01:15 16:08:49. So the resulting filename should be 2012-01-15_16-08-49_dsc_2843.jpg instead.

vtasoulas updated this revision to Diff 19356.Sep 10 2017, 12:55 PM

There was this conditional compilation with the following comment:

For some unknown reason, trying to catch Exiv2::Error fails with Exiv2
>=0.14. For now, just catch std::exception. I would welcome any
explanation.
...
...
#else
In libexiv2 0.12, Exiv2::Error::what() returns an std::string.

I just tested in my current installation with Exiv2 0.26, and catch Exiv2::Error works perfectly fine, so I removed that conditional compilation.

Anyway, exiv2 0.12 is 11 years old so I do not believe it is of utmost importance to have latest version support for an 11 years old system. Such an old system will most likely not even have QT5, so the latest version will not compile no matter what.

gateau accepted this revision.Sep 10 2017, 1:43 PM

Looks good, thanks for simplifying exception handling code in Exiv2ImageLoader.

This revision is now accepted and ready to land.Sep 10 2017, 1:43 PM
ngraham accepted this revision.Sep 10 2017, 2:07 PM
This revision was automatically updated to reflect the committed changes.

@vtasoulas Thanks for such detailed explanation, I tested it and I see, it works. Guess I'm a bit late though.. :)