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 from Silverfast DNG causes rawspeed to fail #283

Open
0x10 opened this issue Jul 9, 2021 · 9 comments · May be fixed by #302
Open

Exif data from Silverfast DNG causes rawspeed to fail #283

0x10 opened this issue Jul 9, 2021 · 9 comments · May be fixed by #302

Comments

@0x10
Copy link

0x10 commented Jul 9, 2021

This is a follow up issue from the darktable issue (darktable-org/darktable#9447).

tldr of 9447:

  • Silverfast (filmscanner) may produce DNG files with additional exif data (IFD1 + PreviewTiff) which causes rawspeed to report the image as corrupt.
  • sample images found here

First thought, as shared within the 9447, was that because of the IFD1 Block rawspeed may fail. So I tried to remove it.
I ran exiftool -all= <file> and magically rawspeed (accessed via darktable) could read the image and all went well. Hurray!
Now I tried to find out whats different within the files.

I made a diff of the exiftool -v output (see attached) and the IFD1 and PreviewTiff Blocks are still present, but what was removed was:

  • IFD0:ApplicationNotes
  • IFD0:ExifOffset

I tried removing those by hand. IFD0:ApplicationNotes could be removed, but didn't change anything. The image was then still reported as corrupt.
IFD0:ExifOffset could not be removed by hand with exiftool (it says "not writeable"). Does anyone has a tip how to remove it?

But as ExifOffset remains, I guess its whats causing the error and I guess its the content of the MakerNoteUnknownBinary.

Any Ideas how to check that?

Here are the exiftool -v outputs and diffs.
exif_meta_vs_no_meta_diff.log
exif_w_meta.log
exif_wo_app_note.log
exif_wo_meta.log

@kmilos
Copy link
Contributor

kmilos commented Aug 11, 2021

Don't think it's metadata per se, more the file structure itself.

rawspeed fails here when parsing the additional IFD1 which does not exist in the lower resolution variant, but I don't think the file is really "corrupt" (exiftools manages to follow and parse the interleaved IFDs and SubIFDs ok), but the layout is a bit unusual...

@kmilos
Copy link
Contributor

kmilos commented Aug 11, 2021

Ah, there is indeed an erroneous overlap detected. There is an extra (non-existent and invalid) IFD parsed after the Exif subIFD. It is incidentally stating at the 0x927c makernote buffer offset right after the Exif tag entries, I guess coming from here.

The makernote is only 130 bytes of some unknown binary data in this case (correctly stored in the 0x927c tag value count), but rawspeed tries to parse it as an IFD of bogus 234474 bytes (19539 tags), hence the overlap.

This also happens in the lower resolution case, but does not create the problem because there is no attempt to parse/insert any IFDs after it.

I guess only valid makernote IFDs should be added to the 'ifds' list, but not sure how to best detect this...

@kmilos
Copy link
Contributor

kmilos commented Aug 11, 2021

I guess its the content of the MakerNoteUnknownBinary

@0x10 It was a good hunch after all, but unfortunately I also wasn't able to remove just this tag using exiftool... But if you zero out those 130 bytes in a hex editor then it works out. (The first 6 bytes should suffice actually; maybe even 2...).

@LibRaw
Copy link
Contributor

LibRaw commented Aug 12, 2021

BTW, I've looked into makernotes (0x927c) parsing code. I see completely broken logic here:

  • makernotes tag data is analysed for known prefixes (e.g. OLYMP or Nikon)
  • if nothing known found then entire tag data is passed to TIFF IFD parser

Then, in TIFF IFD parser:

  • numEntries is read as 16bit int. For non-TIFF (binary) makernotes block this reads some random unsigned 16-bit number
  • IFDFullSize is calculated as 6 + 12 * numEntries (this is some random number based on previous read)
  • IFDFullSize is not compared to makernotes tag size (makernotes size is not passed here, so no value to compare), so some random-size file section will be parsed.
  • Buffer(offset, IFDFullSize) is inserted into ifds set: this buffer is random size so next IFD may overlap with it, resulting in (false) file refusal
  • and this Buffer get parsed via parseIFDEntry calls for numEntries items.
  • Again, we're parsing some binary (non TIFF-like) data, so wrong tag data types (this is just some binary bytes) may result into TIFFParserException and so in (correct) file refusal.

The easiest way to improve it is to add 'LSI1\0' as known binary-makernotes prefix and do not parse such sections.

This does not negate the need to correct the entire makernotes parsing logic to not fail on non-tiff makernotes (as noted by @kmilos above):

  • IFD parser should compare 0x927c tag size with IFDFullSize calculated and stop makernotes parsing (but do not refuse the file) if calculated size is larger than tag size (opposite case is OK).
  • IFD overlap check should be fixed for that case: passed tag size (+offset) should not overlap with other ifds, not calculated size.
  • TIFF tag parser should not stop/throw exception if makernotes section being parsed: makernotes are, by definition, vendor specific. It is possible that non-vendor parser does not know some vendor specific. In such case parser should skip unknowns, not raise fatal error.

Dixi

@kmilos
Copy link
Contributor

kmilos commented Aug 12, 2021

Thanks, much better summary of the problem than what I described.

I guess the same logic can be equally applied to 0xc634 (DNGPrivateData): if parsing as simple IFD (or other known vendor MakerNote variant) is unsuccessful, then fall back to adding it as binary blob as it is currently.

@LibRaw
Copy link
Contributor

LibRaw commented Aug 12, 2021

Actually we're discussing raw data decoder, not general-purpose metadata parser (like exiftool or exiv2).
I'm not sure at all if it makes sense to parse unknown makernotes blocks: known blocks contains known data (e.g. black/white level) that may be used for raw processing. But what is supposed to be done with unknown data from unknown blocks?

Of course, changing the logic will require additional checks: from such a vendor we parse only such a block (with fixed known prefix list) and these tables will have to be maintained, which is a lot of work.
But this is better than rejecting the correct dng

@0x10
Copy link
Author

0x10 commented Sep 1, 2021

Thanks for both of your summaries (sorry for the late reply, holiday-season...). I'm though a bit confused about the status of the issue now. Do you have a possible solution and its only pending on implementation? Or is this a "won't fix" because its a vendor thingy?

@kmilos
Copy link
Contributor

kmilos commented Sep 1, 2021

Or is this a "won't fix" because its a vendor thingy?

No, the file is perfectly valid.

I'm though a bit confused about the status of the issue now. Do you have a possible solution and its only pending on implementation?

Yep, pending until someone comes up with a proposed fix for rawspeed.

@0x10
Copy link
Author

0x10 commented Sep 1, 2021

Okay thank you!

@LibRaw LibRaw linked a pull request Sep 5, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants