[imageio] [DAM] Fix crop factor calculation for many Fujifilm cameras

[victoryforce]

For the vast majority of Fujifilm cameras, the focal plane pixel resolution data is correct not relative to the actual pixel dimensions of the sensor, but (for some incomprehensible reason) relative to the dimensions of the preview image. Therefore, for them, we have to take values from Exif.Photo.PixelXDimension and Exif.Photo.PixelYDimension into the diagonal calculation formula.

[kmilos]

for some incomprehensible reason

As I already mentioned, Exif (for RAFs) really is part of the preview image, so it really describes that image only, and unfortunately we can't make so many other assumptions.

Every proprietary raw format has its own quirks, only DNG (and NEF claiming to be TIFF/EP compliant, maybe some other one as well?) have a defined behaviour we can rely on. For everything else, it's a custom and format-by-format mix of MakerNotes, Exif, whatever tags we can dig out and correlate...

[victoryforce]

Thank you, @kmilos. However, I was hoping for a review. But if you don't have time for that, it's okay. This PR has been tested on samples from many models, so it definitely increases the number of models for which the crop factor will be calculated correctly.

As I already mentioned, Exif (for RAFs) really is part of the preview image, so it really describes that image only, and unfortunately we can't make so many other assumptions.

I am aware of this. It was not the best decision of Fujifilm programmers. The point here is not what assumptions we can make, but that the programmers put data in the tag that is different from what the semantics of that tag require. This is what I wrote about in the comment accompanying the PR.

Every proprietary raw format has its own quirks

That's exactly my point, only you put it even more bluntly than I did. Maybe, in fact, not every format has them to the same degree. Some have far more quirks than others. And Fujifilm claims a prize here.

[kmilos]

However, I was hoping for a review.

LGTM

And Fujifilm claims a prize here.

I wouldn't dare to call it, the race is wide open on that one as far as I'm concerned... 😖

[victoryforce]

@kmilos Since you are also active in the exiv2 project, I have a question for you that is not directly related to this specific PR, but to the accuracy of displaying information about images.

What are the expected semantics of exiv2's pixelWidth and pixelHeight methods? Are these expected to be full image dimensions?

[kmilos]

What are the expected semantics of exiv2's pixelWidth and pixelHeight methods? Are these expected to be full image dimensions?

It's difficult to define what a "full image" dimension is for some formats as we've learned. I think the semantics are "the best educated guess" for the format. In the case of RAF format, this indeed turns out to abstract the dimensions of the embedded preview:

https://github.com/Exiv2/exiv2/blob/v0.28.1/src/rafimage.cpp#L30-L42

[victoryforce]

@kmilos Right. So should I raise an issue on exiv2 github or will you do it yourself?

[kmilos]

So should I raise an issue on exiv2 github or will you do it yourself?

What's the issue?

[victoryforce]

It's difficult to define what a "full image" dimension is for some formats as we've learned.

We definitely learned something else. Regardless of the format, a "full image" is obviously the image whose file exiv2 has opened to read the metadata. The dimensions of this image are the dimensions it will have after decoding. It is surprising to hear that it is difficult to define. Maybe you meant to say "find out values"?

But, BTW, for many RAF images the Windows file properties pane shows correct dimensions, while the exiv2 preview dimensions. I do not think that exiv2 should provide information of worse quality than Windows.

What's the issue?

In the case of RAF format we we will get values that have nothing to do with reality. This is much worse than providing no values. I'd say it's a pretty serious bug.

I was hoping that you, as a person active in both projects, would help fix this bug in exiv2, as it causes the wrong data to be displayed in darktable.

[kmilos]

We definitely learned something else. Regardless of the format, a "full image" is obviously the image whose file exiv2 has opened to read the metadata. The dimensions of this image are the dimensions it will have after decoding. It is surprising to hear that it is difficult to define. Maybe you meant to say "find out values"?

Not so simple for raw files I'm afraid. Even without those vendor implementation shenanigans, there can also be masked pixels for black level compensation, etc., all of which are not necessarily described anywhere in the metadata. LibRaw and rawspeed databases are full of sensor crop information that was determined manually for individual camera models (and even specific modes), many times there are no heuristics even within the same format...

But, BTW, for many RAF images the Windows file properties pane shows correct dimensions, while the exiv2 preview dimensions. I do not think that exiv2 should provide information of worse quality than Windows.

No idea what Windows does. And how do you know those dimensions are "correct"? Have you inspected the raw data dump and verified the acceptable crop? While they might be closer, they still might not be entirely "correct".

In the case of RAF format we we will get values that have nothing to do with reality. This is much worse than providing no values. I'd say it's a pretty serious bug.

You're of course entitled to your opinion, and you're free to open an issue if you wish to do so.

I was hoping that you, as a person active in both projects, would help fix this bug in exiv2, as it causes the wrong data to be displayed in darktable.

For this particular 35mm equivalence issue, we figured out that the Exif data pertains to the embedded image only, and that the relevant tags are correctly correlated when it comes to the physical aspects of the sensor. exiv2, while not making it trivial, and is certainly not perfect, did the job nonetheless. Good enough for me.

The only remaining issue (and one that should be tracked separately) I see is initial image information in lighttable view provided by exiv2 before you open the image in the darkroom (when some of the image information is updated by the actual rawspeed decoding process). As I mentioned, you're free to open an issue on exiv2, but I don't see a solution yet - Exif.Fujifilm.{RawImageFullWidth,RawImageFullHeight} provides uncropped dimensions so I don't feel that's good enough either (we don't have access to rawspeed's manual crop database at that point), and for many RAFs from not-so-recent Fujifilm models, those tags don't even exist anyway as the format internals are quite different.

exiv2 is about providing access to normalized Exif, XMP, and IPTC data, not so much about decoding raw files "perfectly". MakerNotes and other proprietary information is parsed on a best effort basis. I have very little interest (and time) in dealing w/ the raw file format circus in exiv2 in search of some general perfection, and keep that activity to what is just absolutely necessary (when it comes to raw data decoding support, i.e. one needs to get to the image first before the metadata from my point of view). In fact, I'm afraid I'll have to reduce my involvement across the board, as I already have a full-time job. exiv2, like many other projects, is in dire need of contributors.

[victoryforce]

@TurboGit This is a fairly simple (in terms of code changes) PR, which I tested on Fujifilm camera samples from the RPU. It is guaranteed to improve output for many Fujifilm camera models.

[TurboGit]

@victoryforce : I have some many thing to review/test given the free time I have at the moment that I'm monitoring PR a bit less at the moment. For this PR I see some disagreements between you and @kmilos (which is fine of course) and since I'm not expert at all in this area I have hard time to know what to do.

So @kmilos do you agree to merge given last comment from @victoryforce which says that it improved things in many Fujifilm camera models?

[kmilos]

@TurboGit There is no disagreement, the PR is good!

The discussion took a turn about exiv2 expectations...

[TurboGit]

All good then, thanks!

[kmilos]

The only remaining issue (and one that should be tracked separately) I see is initial image information in lighttable view provided by exiv2 before you open the image in the darkroom (when some of the image information is updated by the actual rawspeed decoding process).

FYI, after poking around exiv2 code some more, I have found a precedent: for Panasonic RW2 files pixelWidth() will return the Exif.PanasonicRaw.SensorWidth, which is, obviously, the uncropped sensor size (not necessarily entirely "correct" IMHO as I explained already)...

For Canon CR2/CRW, exiv2 also wraps Exif.Photo.PixelXDimension, but I need to do some more checking if those reflect the sensor or the thumbnail...

Not sure what's the best course of action here TBH, and I'd rather not deal with it in depth... 😕 I guess I might be open for a "quick/dumb/dirty fix" that would make Fujifilm behaviour aligned w/ Panasonic for now if people really feel this is somehow closer to reality...

Edit: for all other TIFF-based raw formats, uncropped ImageWidth is wrapped by pixelWidth(), so I'll just do it and make everything consistent (i.e. equally "incorrect") then.

Note that for some legacy Fujifilm files the Exif.Fujifilm.FujiIFD don't exist as such, so thumbnail dimensions are still used as fallback. Parsing the custom Fujifilm data structure is where I draw the line at this point.

[victoryforce]

so thumbnail dimensions are still used as fallback

Why not return 0 in this case, signaling that there is no relevant information in the metadata, instead of returning what is known to be grossly incorrect data?

[kmilos]

Why not return 0 in this case, signaling that there is no relevant information in the metadata

Hm, maybe... I think I'll also make another change that makes it more universal, in case someone implements the parsing of the Fujifilm proprietary block down the line.

[victoryforce]

@TurboGit A proposal for the Release notes:

Added the ability to calculate the crop factor for those cameras that do not contain this information in the metadata.

[TurboGit]

Thanks!