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

dcraw: prefer raw crop and mask from dng metadata #7016

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

sgotti
Copy link
Contributor

@sgotti sgotti commented Mar 29, 2024

Prefer raw crop and mask from dng metadata instead of camconst.json entries.

The unique exception, to keep backward compatibility, is from in camera Pentax DNGs. In such case, the preference goes to a camconst entry, if it exists.

This is based on discussion (see last comments) in #6826

It uses the same logic already implemented for RT_whitelevel_from_constant and RT_blacklevel_from_constant

To sum up, from my understanding of the code and after reading some old discussions/issues:

How to test

To test this PR use, for example, a Fuji XT3 file and convert it to dng with ADC. Change camconst to remove the source size filter from raw_crop:

--- a/rtengine/camconst.json
+++ b/rtengine/camconst.json
@@ -1596,10 +1596,7 @@ Camera constants:
     { // Quality A, samples provided by Daniel Catalina (#5839) and pi99y (#5860)
         "make_model": [ "FUJIFILM X-T3", "FUJIFILM X-PRO3" ],
         "dcraw_matrix": [ 13426,-6334,-1177,-4244,12136,2371,-580,1303,5980 ], // DNG_v11, standard_v2 d65
-        "raw_crop" : [
-           { "frame" : [6384, 4182], "crop": [ 0, 5, 6252, 4176] },
-           { "frame" : [6384, 3348], "crop": [624, 0, 5004, 3348] }
-        ],
+        "raw_crop": [ 0, 5, 6252, 4176],
         "white": [ 16170, 16275, 16170 ] // typical safe-margins with LENR
         // negligible aperture scaling effect
     },

Without this PR crop data is taken from camconst and the file is wrongly demosaiced, with this PR the crop data is taken from DNG metadata and correctly demosaiced.

@Lawrence37 @kmilos @Entropy512

Prefer raw crop and mask from dng metadata instead of camconst.json
entries.

The unique exception, to keep backward compatibility, is from in camera
Pentax DNGs. In such case, the preference goes to a camconst entry, if
it exists.
@Lawrence37
Copy link
Collaborator

We should review camconst.json for any raw crops for dng-only cameras, aside from Pentax since it's already handled by your code.

@sgotti
Copy link
Contributor Author

sgotti commented Mar 31, 2024

We should review camconst.json for any raw crops for dng-only cameras, aside from Pentax since it's already handled by your code.

I know only of leica SL cameras. The SL, SL2 and the just released SL3 creates only dng files. Not sure about other Leica models.

Since this will be just a cleanup (without side effects), do you prefer to do this in this PR or open an issue when this will be merged and do it in another PR?

Do you mean to check if the dng raw crop is different than the one in the camconst? In such case what should be done?

@Lawrence37
Copy link
Collaborator

If there's a raw crop for a dng-only camera, that means the crop is meant to override the values in the dng metadata. The camconst crop should take precedence just like it is with Pentax dngs.

@sgotti
Copy link
Contributor Author

sgotti commented Apr 3, 2024

@Lawrence37 @kmilos

For Leica SL cameras, their unique raw format is dng. I analyzed some samples for the three SL models and probably the same should also apply for the Q models

For example:

Leica SL2-S

There'a a relative raw crop defined in camconst.json [ 0, 2, 0, -4 ] . Please note that the the DNG Active Area is used as an initial raw_crop, so the entries in camconst.json, if relative like in this case, are doing an additional subtraction from the dng ActiveArea.

See also issue #6390.

So for a normal image:

  • raw dimensions: 6048 x 4048
  • dng active area/row crop: 0 0 6024 4048
  • camconst raw crop: 0 2 6024 4042

for a high resolution image:

  • raw dimensions: 12096 x 8082
  • dng active area/row crop: 0 0 12034 8082
  • camconst raw crop: 0 2 12034 8076

Something similar also for the other SL models.

With the raw crop provided by the dng I don't see any garbage pixels so I don't see where they came from when the entries were added.

So looks like the raw crop is not required in camconst and the ones in camconst are greater/too conservative since, when relative, they are substracting from something already cropped (dng ActiveArea) and not the whole raw size.

Also darktable/rawspeed doesn't have any crop but uses the dng values.

What should we do?

  • Remove the entries from camconst also it will change a bit current edits (is this considered a backward compatibility issue?)
  • keep the entries in camconst, document they aren't need but kept for bw compat, and perhaps remove them in 6.0?

@kmilos
Copy link
Contributor

kmilos commented Apr 3, 2024

I analyzed some samples for the three SL models and probably the same should also apply for the Q models

M series as well I think...

On RPU also CL and X2 samples are DNGs, and also X-U and TL2 at DPR.

@sgotti
Copy link
Contributor Author

sgotti commented Apr 3, 2024

M series as well I think...

On RPU also CL and X2 samples are DNGs, and also X-U and TL2 at DPR.

Yeah but many of these are not in camconst

$ grep -i leica rtengine/camconst.json | grep make_model
        "make_model" : [ "LEICA C-LUX", "LEICA CAM-DC25" ],
        "make_model" : "LEICA D-LUX 7",
        "make_model": "LEICA Q (Typ 116)",
    //    "make_model": "LEICA M8",
        "make_model": "LEICA Q2",
        "make_model": "LEICA SL (Typ 601)",
        "make_model": "Leica SL2",
        "make_model": "Leica SL2-S",
        "make_model" : ["LEICA V-LUX 5","Panasonic DC-FZ1000M2"],
        "make_model": [ "Panasonic DMC-FZ1000", "Leica V-LUX (Typ 114)" ],
        "make_model": [ "Panasonic DMC-LF1", "Leica C (Typ 112)" ],
        "make_model": [ "Panasonic DMC-LX100", "Leica D-LUX (Typ 109)" ],

Leica m8 commented out since it was causing issues and with just dng metadata it works (#6237 and #6498).

But we can't know if they are supported or just not tested by anyone

@Lawrence37 probably a list of supported cameras, also without any overrides should be kept. Perhaps camconst.json is the right place (just add an entry without any overrides or also add a field reporting it as unsupported) like done by rawspeed with cameras.xml. If it was not already discussed I'd like to open a dedicated issue.

@kmilos
Copy link
Contributor

kmilos commented Apr 3, 2024

Perhaps camconst.json is the right place (just add an entry without any overrides or also add a field reporting it as unsupported) like done by rawspeed with cameras.xml.

One caveat - dt/RawSpeed doesn't explicitly test nor mark as supported/unsupported DNG based models (except for Pentax maybe because of the dual PEF output), and many samples are missing on RPU indeed (Roman generally doesn't even want them unless they are to test some new functionality). DNGs are self-contained and should "just work" (assuming the implementation of various compression modes) w/o any additional metadata supplied or override. If that's not the case, it is probably an invalid/uncompliant file. Checking and listing all the possible devices outputting DNG is a fool's errand IMHO...

The presence of DNG based models in RawSpeed's cameras.xml is there just to normalize and prettify the Make/Model strings (and we pretty much draw the line at "real" camera bodies like Leica, Pentax/Ricoh, and Sigma; trying to cover smartphones and the myriad of other devices is not worth it IMHO...).

In other words, IMHO you should only use your camconst.json to exceptionally override some known problematic DNGs, not to track valid ones.

@Lawrence37
Copy link
Collaborator

As far as Leica dngs go, these models have raw crops in camconst.json.

  • LEICA Q (Typ 116)
  • LEICA Q2
  • LEICA SL (Typ 601)
  • Leica SL2
  • Leica SL2-S

For the Leica SL2-S, we know the original raw crop was [ 0, 2, 6024, 4042 ] with a comment explaining it:

2 rows at top and 4 rows at bottom are black

It is strange that the dng ActiveArea says otherwise, so I would like to confirm if there really are black rows.

The other models should be reviewed too, or just take the easy option of allowing Leica dng raw crops to be overridden by the camconst entries.

@Lawrence37 Lawrence37 added scope: file format Camera or image file formats scope: calibration Camera calibration (camconst.json) labels Jun 1, 2024
@sgotti
Copy link
Contributor Author

sgotti commented Jun 25, 2024

It is strange that the dng ActiveArea says otherwise, so I would like to confirm if there really are black rows.

@Lawrence37 I did more tests and just found that until now there was the need to define custom raw crops also for dng files because dcraw isn't handling dng DefaultCropOrigin and DefaultCropSize (that should be applied to the dng ActiveArea) but only ActiveArea. For this reason there're additional raw crop for Leica DNGs in camconst.

libraw parses and handles DefaultCrop (and also additional UserCrop) but doesn't apply them by default but only if the user calls LibRaw::adjust_to_raw_inset_crops to adjust the image dimensions to the cropped info provided by dng (or other vendors). So also the current libraw branch doesn't handle vendor crops like dcraw since it isn't calling that function.

Additionally there's a special case in libraw for leica sl2 (not sure the reason why): https://github.com/LibRaw/LibRaw/blob/d3cbbd0e9934898eb28e4963ee99b51928e2acaa/src/utils/open.cpp#L975-L978

From all of the above some open issues arise related to this issue and also the the libraw branch:

  • dcraw and libraw images sizes con be different for the same file due to different handling (this may affect any raw file, not only dng).
  • Sizes will change if we implement handling if DefaultCrop in dcraw.
  • Sizes will change if we use LibRaw::adjust_to_raw_inset_crops and this may affect any raw file, not only dng, because it handles not only dng raw crop metadata but also all the other files (or it should be called only for dng files).
  • The above image size changes won't affect camconst raw_crop with absolute values but will affect relative raw crops. (i.e. leica SL2 "raw_crop": [ 0, 0, 0, -18 ]
  • The use of DNG DefaultCrop could lead to smaller image sizes. i.e. (Leica SL2 files have DefaultCropOrigin 12, 24 and DefaultCropSize 8368 5584, much smaller than current rawcrop that brings to 0, 0, 8392, 5613 )

@Lawrence37
Copy link
Collaborator

The dng documentation doesn't explicitly say it, but it does imply that the DefaultCrop is not necessary to guarantee a good image. Using the whole ActiveArea should be acceptable. If the Leica dngs contain black pixels in the ActiveArea, then they do not follow the specification (masked pixels must not be in the ActiveArea). We should use the ActiveArea to crop images and have an exception for Leica dngs.

@Lawrence37 Lawrence37 added this to the v6.0 milestone Jul 28, 2024
@Lawrence37
Copy link
Collaborator

Converting to a draft because Leica camconst crops need to be considered.

@Lawrence37 Lawrence37 marked this pull request as draft September 21, 2024 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: calibration Camera calibration (camconst.json) scope: file format Camera or image file formats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants