Skip to content
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

EXIF data may create problem when saving in Database #117

Closed
ABLomas opened this issue Mar 5, 2019 · 26 comments · Fixed by #478
Closed

EXIF data may create problem when saving in Database #117

ABLomas opened this issue Mar 5, 2019 · 26 comments · Fixed by #478
Labels
bug Something isn't working

Comments

@ABLomas
Copy link

ABLomas commented Mar 5, 2019

Trying to migrate my photo collection into Lychee-Laravel, created albums with sub-albums, but many uploads (i would say > 20% so far) fail with errors:

Upload failed. Server returned an error!
"Error: Id is a bit too big... 15518221109952"
err1
(would be nice if this window could allow text selection, i need to go developer tools -> inspector or console and only here i can copy error message - but this is just minor problem, would be better if this would not show at all ;-))

Trying to re-upload does not gets better, getting the same problem.
Files are simple photos, 0.5-2MB in size, up to 2048px longest dimension. Looks like error thrown is in /app/ModelFunctions/PhotoFunctions.php, line 486 - but originates from DB (mysql) - in this case this would be "Out of range value for column". Can we set field type to something bigger then?

@d7415
Copy link
Contributor

d7415 commented Mar 5, 2019

Are you using 32 or 64-bit php?

(This sounds like a known 32-bit problem)

@ABLomas
Copy link
Author

ABLomas commented Mar 5, 2019

# php-cgi --version
PHP 7.3.2-3+0~20190208150725.31+stretch~1.gbp0912bd (cgi-fcgi) (built: Feb  8 2019 15:07:25)
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.3.2, Copyright (c) 1998-2018 Zend Technologies
    with Zend OPcache v7.3.2-3+0~20190208150725.31+stretch~1.gbp0912bd, Copyright (c) 1999-2018, by Zend Technologies
# uname -a
Linux nc 4.9.0-8-amd64 #1 SMP Debian 4.9.110-3+deb9u6 (2018-10-08) x86_64 GNU/Linux

I guess it's x86_64.

@d7415
Copy link
Contributor

d7415 commented Mar 5, 2019

I guess it's x86_64.

Yeah, looks 64-bit.

@ildyria
Copy link
Member

ildyria commented Mar 6, 2019

Can you check the type (INT or BIG INT) of the column Id in your mysql database?
As far as I know, 15518221109952 < 2^64 - 1 thus this should not be a problem.

Edit: The error might actually be else where... The column is too small, and this implies to slightly change the code that show this error to actually show the incriminated column.

@ABLomas
Copy link
Author

ABLomas commented Mar 6, 2019

On which table?
On "photos" table it's bigint(20). I upgraded database from Lychee3 using 'php artisan migrate' so there may be still some compatibility issues while upgrading (but cleaned all contents before upgrade).

@ildyria
Copy link
Member

ildyria commented Mar 6, 2019

Nah, there is no problem with the ID.
https://github.com/LycheeOrg/Lychee-Laravel/blob/master/app/ModelFunctions/PhotoFunctions.php#L485

The problem is that you get a $errorCode == 1264 which means that one of the column is too small for some reason. I will need to fix that code to see which column.

@ABLomas
Copy link
Author

ABLomas commented Mar 6, 2019

I will turn on trace on SQL side and catch error, but already have idea why this may be.
Dimensions for resized photos should be integers, but for some photos they are stored as:
1620.3956043956x1080 or 540.13186813187x360
As "medium" or "small" columns are only char(20), this can easily fill entire space.
I will check where code is generated, most likely one round(); will solve this issue completely.

@ABLomas
Copy link
Author

ABLomas commented Mar 6, 2019

Setting medium/small column to char(40) does not resolve issue (but i still think that it's bad idea to store size in fractional units).
Failing query from trace is (i wrapped it so arguments can be easily matched)::

insert into `photos` 
(`id`, `title`, `url`, `description`, `tags`, `width`, `height`
, `type`, `size`, `iso`, `aperture`, `make`, `model`, `lens`, `shutter`, `focal`, `takestamp`
, `latitude`, `longitude`, `altitude`
, `public`, `owner_id`, `star`, `checksum`
, `album_id`, `thumb2x`, `thumbUrl`
, `medium`, `medium2x`, `small`, `small2x`, `updated_at`, `created_at`) 
values 
('15518774834946', 'antakalnis4', 'dc30fe79d3a0d07628080d2a808fbd0f.jpg', '', '', 2048, 1152
, 'image/jpeg', '1.7 MB', 100, 'f/4.
0', 'Hasselblad', 'L1D-20c', '28.0 mm f/2.8', '1/160 s', '10.3 mm', '2018-10-09 18:11:13'
, 54.718195, 25.316371666666665, 102166873.4
, 0, 0, 0, 'd2f73a79113d8deb1423cf9dd10b5fa6d3fd05f5'
, '15518220108863', 1, 'dc30fe79d3a0d07628080d2a808fbd0f.jpeg'
, '1920x1080', '', '640x360', '', '2019-03-06 13:04:47', '2019-03-06 13:04:47');

But if i manually execute it on this table - it works fine.
Next query is already insert into error log and before statement is checking for id existance, both execute correctly. Maybe SQL error handling is not working properly?

(i can attach entire failing photo if someone wants to try reproduce issue)

@ABLomas
Copy link
Author

ABLomas commented Mar 6, 2019

OK, found it.
"Original" database has altitude field defined as decimal(10,4). This is OK for "common photos" but will fail on most DJI copters with older firmware. I guess, even with new firmware they will still break Lychee, as they provide GPS alt and calibrated (baro) alt in three separate EXIF tags (altitude, AbsoluteAltitude and RelativeAltitude).

Extending "altitude" field to decimal(14,4) fixes this problem.
Proper solution could be taking altitude from "RelativeAltitude field, if "XMP-drone-dji" section exists, but this is just minor improvement.

@d7415
Copy link
Contributor

d7415 commented Mar 6, 2019

Extending "altitude" field to decimal(14,4) fixes this problem.

102166873.4

A quick search implies that altitude EXIF should be in metres. That is not in metres (unless you're on your way to the moon). Though it also seems that the field should be named GPSAltitude.

@d7415
Copy link
Contributor

d7415 commented Mar 6, 2019

Though it also seems that the field should be named GPSAltitude.

That's from the EXIF 2.31 standard (which is the most recent)

@ABLomas
Copy link
Author

ABLomas commented Mar 6, 2019

A quick search implies that altitude EXIF should be in metres. That is not in metres (unless you're on your way to the moon). Though it also seems that the field should be named GPSAltitude.

I agree that this is issue with EXIF data. But unless you want to discard smth like >50% of aerial photos (or force all owners to batch-rewrite RelativeAltitude into altitude field in exif) software should not fail on import.
This was bug (at least i call it bug, some ppl still say that this is "not a problem as you should get alt data from "RelativeAltitude") on DJI copters: https://forum.dji.com/thread-163684-1-1.html - and i doubt it is completely fixed by now. Having in mind that so many aerial pictures are taken with this faulty firmware (or will be taken in future, AFAIK not all models got update) you need to work around this. Ofc, failing with "Id is a bit too big... " error is always an option =)

@d7415
Copy link
Contributor

d7415 commented Mar 6, 2019

I probably won't be implementing it (I haven't done much Laravel stuff) but I'd suggest dropping the field if it's out of bounds (with an error to the log) rather than increasing the size of the DB field for bad data.

@d7415
Copy link
Contributor

d7415 commented Mar 6, 2019

and i doubt it is completely fixed by now.

The thread you linked implies it is.

@ABLomas
Copy link
Author

ABLomas commented Mar 6, 2019

The thread you linked implies it is.

Well, if you believe what DJI support says - let it be.
Even if we pretend that it's fixed (while it's not, i'm owner of such device and so far no solution or date provided) it will fail with mystery error for older photos. So, unless uploader is smart enough to figure out that "Id is a bit too big... " means "your altitude data is wrong" and can batch-update EXIF on all photos - he can't use Lychee for such photos.
Anyway, i'm not forcing you to fix anything ;-)

@d7415
Copy link
Contributor

d7415 commented Mar 6, 2019

As I proposed above, I think dropping the data and logging a warning would be an appropriate solution. Ideally this would be extended to other fields as well.

Increasing the field size to accept corrupt data increases the size of the database for no reason, and hides the real issue here.

@ildyria
Copy link
Member

ildyria commented Mar 7, 2019

With the latest version, I made so that it Logs with a proper error message when this error happen. :)

@ildyria ildyria added the bug Something isn't working label Mar 7, 2019
@ABLomas
Copy link
Author

ABLomas commented Mar 11, 2019

So far my workaround with "enlarged" altitude field was working good for my photos (1000+ of them) but in reality i agree with d7415 that increasing field is bad choice.
I also spotted many photos from phones with bad lan/lon coords, corrupted EXIF location data when doing photo manipulations and so on. So, probably correct way to fix this would be:

  • check if lat/lon/alt is valid (many libs to validate this already on web)
  • set to 0 or NaN/no data if invalid, keeping the same field width
  • else insert it as-is.

Even with "extended reporting" it's not good idea to drop image import just because some exif field is corrupt (due to software bug or intentionally).

@ildyria
Copy link
Member

ildyria commented Mar 11, 2019

If you can propose such a fix, we will gladly merge it :)

@ildyria ildyria changed the title Many uploads fail with ""Error: Id is a bit too big... " EXIF data may create problem when saving in Database Apr 9, 2019
@ildyria
Copy link
Member

ildyria commented Sep 29, 2019

@ABLomas did the recent commit had an impact on this bug (solving wise) ?

@ABLomas
Copy link
Author

ABLomas commented Sep 29, 2019

  • map is shown (if enabled in settings) and valid coords are in EXIF. Coords looks OK
  • on older photos from DJI (older firmware) altitude is still invalid (113838052.3m - example: https://pics.lag.lt/#15519904888473/15519904975214)
  • with new firmware alt is displayed sort-of correctly (ABSOLUTE - not relative. Example: https://pics.lag.lt/#r/15694180521690). Not sure if relative alt would be better, at least it is parsed without errors.
  • uploading photo taken earlier (with higher altitude/earlier firmware/earlier model) will probably (i changed DB field as described above) fail again as altitude won't fit into 10,4 field. I did not tested this, but can provide examples if needed.

P.S. map is cool feature =)

@tmp-hallenser
Copy link
Contributor

tmp-hallenser commented Dec 6, 2019

To be checked with #401
Edit: not fixed

@ildyria ildyria mentioned this issue Apr 16, 2020
@ABLomas
Copy link
Author

ABLomas commented Jul 8, 2022

Back again with migration in master, ./database/migrations/2021_12_04_181200_refactor_models.php - migration will fail, fix:
717: $table->decimal('altitude', 14, 4)->nullable()->default(null);
790: $table->decimal('altitude', 14, 4)->nullable()->default(null);
(change table creation script from 10, 4 to 14, 4 - else migration will fail due to invalid EXIF data or it won't allow to upload photos, done with older DJI firmware, where altitude field is longer)

@nagmat84
Copy link
Collaborator

nagmat84 commented Jul 8, 2022

Please open a new issue for that error. Is is not a good approach to recycle an already closed issue, in particular as the problem is a completely different one. The original problem was about the id field being too small, this issue is about altitude and latitude.

On top, I cannot follow the problem. The migration, which you refer to, re-creates the columns the same way as they have been before. Originally, the columns have been created by this migration in 2018: https://github.com/LycheeOrg/Lychee/blob/master/database/migrations/2018_08_03_110936_create_photos_table.php#L63-L65

$table->decimal('latitude', 10, 8)->nullable();
$table->decimal('longitude', 11, 8)->nullable();
$table->decimal('altitude', 10, 4)->nullable();

The migration which you refer to only re-creates the columns and moves the data between tables: https://github.com/LycheeOrg/Lychee/blob/master/database/migrations/2021_12_04_181200_refactor_models.php#L698-L700

$table->decimal('latitude', 10, 8)->nullable()->default(null);
$table->decimal('longitude', 11, 8)->nullable()->default(null);
$table->decimal('altitude', 10, 4)->nullable()->default(null);

The definitions are identical except for the explicit default value. So, unless you have changed your table definitions manually and have entered longer values into the tables manually, there is no way how this specific part of the migration can fail.

@ABLomas
Copy link
Author

ABLomas commented Jul 8, 2022

No, initial issue was exactly about the same problem, just i cannot define it clearly in first post and refined later:
#117 (comment)

I was sure that column length was already default but exactly the same problem resurfaced 3 years later, so it's not fixed even if comment above says so.
I added comment so others (i know many who have the same camera so i'm sure that this problem is not isolated and affects only one person) can find this in search and apply fix.

Downside - after initial script run users will have completely screwed up database (renamed tables, moved data, etc) so:

  1. fix migration script by changing those lines above
  2. restore lychee DB from backup
  3. repeat migration

There's no point to open yet another issue with the same problem - altitude can't fit into DB field. If it's not fixed, then at least all information can be found in one place and users can fix own issues.

@kamil4
Copy link
Contributor

kamil4 commented Jul 8, 2022

We never extended the altitude column in Lychee. Instead, PR #478 added checks for invalid values which, if triggered, result in the extracted altitude being ignored. I just checked that those checks are still there in the current version.

Your case @ABLomas is a bit special because you customized your database instance and I guess you imported photos with the broken altitude value before #478 got merged. So the database migration script got exposed to values that it wasn't supposed to see...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants