DngDecoder: default white level for floats is simply 1

[kmilos]

1. We divide by white level to normalize the image s.t. the 1.0 becomes the white level.
2. In DNG spec, white level is always an integer, there can not be a non-integral white level.
3. Additionally, no white level for FP DNG's means that it is pre-normalized (default is 1.0).

Therefore, we can surmise that
a) FP DNG's never have white level of <1.0
b) FP DNG's always have integral white level
c) if FP DNG has a white level of w, it is always correct to divide by float(w) to normalize the image
... therefore, we can, as an optimization, store FP DNG white level as an integer.

This matches DNG SDK behavior.

[LebedevRI]

I don't think this is better.

[kmilos]

I don't think this is better.

Better or not is not relevant - it is what the SDK does.

[LibRaw]

it is what the SDK does.

in addition to the SDK source code, this is also written in the standard: https://helpx.adobe.com/content/dam/help/en/photoshop/pdf/DNG_Spec_1_7_1_0.pdf#page=29

[LebedevRI]

I'm fully aware of that, and none of that is a relevant argument here,
because it dictates how the value is stored in DNG,
it does not and can not dictate how the value is later stored in implementations.

[kmilos]

The spec is somewhat ambiguous: says 1.0 but type is SHORT/LONG.

The SDK removes the ambiguity for me.

[parafin]

Spec says default is 1.0 for float, it doesn’t say it’s the only possible value.

[LebedevRI]

I don't think this is better.

Better or not is not relevant - it is what the SDK does.

Does SDK really store 1.0f as (int)1 (0x1 hex)?

[kmilos]

Yep, see the link.

[LebedevRI]

Aha. But as i have said, i don't see how that is relevant here.
It is still a magic constant. Why would we better off using that one over the one we use?

[kmilos]

It's the correct default value 3rd parties could assume and rely on, just like 2^bps-1.

[LebedevRI]

Is this a practical or theoretical concern?

[kmilos]

For example, darktable-org/darktable#16206 relies on either the float DNG file itself carries WhiteLevel=1, or rawspeed feeds it a 1 if absent.

[LebedevRI]

Once again, the fact that SDK returns (int)1 as default white level for FP DNG's
is a hack / workaround for the lack of std::variant for when that function was written.

We are not required to be bug-for-bug compatible with some other library.
Returning 1 there is just as much as a magic number as returning 65535 or 65536.

There is absolutely no reason whatsoever why that idiotic behavior
must now be mirrored in/by dt's img->raw_white_point.
(just a add a boolean img->raw_is_prenormalized?)

DNG spec does not say that for FP DNG's, white level of (int)1 means that it is normalized.

Likewise, there's inversion of cause and effect ordering there.
The current code does not rely on (int)1 being returned in such situations.
Changing the magic constant breaks all other users (i should not have merged #635.)
That patch will just have to rely on some other magic number.

[kmilos]

Returning 1 there is just as much as a magic number as returning 65535 or 65536.

The DNG spec text says the default value for float files is 1.0, not 65535.0, nor 65536.0... I don't think there's anything arbitrary/magic here.

That's like saying one can return a random number in the integer case, while the spec says it's 2^bps-1?

Let's just remove both defaults then?

And yes, the previous PR was a bit hasty before the dust settled down, sorry about that.

[LebedevRI]

Returning 1 there is just as much as a magic number as returning 65535 or 65536.

The DNG spec text says the default value for float files is 1.0, not 65535.0, nor 65536.0... I don't think there's anything arbitrary/magic here.

Once again, there is a confusion/conflation of the specification,
which only dictates what some particular value of particular tag (or lack thereof) means,
and how an implementation needs to further represent that knowledge.

I never argued that the default isn't 1.0, i only said that we can't represent float white level,
and thus any representation of that fact as an integer results in a magic number
that requires special treatment by consumers. Regardless of what that number is.

[LebedevRI]

#638 either makes things slightly more sufferable, or worse.

[kmilos]

#638 either makes things slightly more sufferable, or worse.

I feel it's overcomplicating things...

[LebedevRI]

After sleeping on this, the wordiage that should have been here, and would have helped, is:

1. we divide by white level to normalize the image s.t. the 1.0 becomes the white level
2. in dng, white level is always an integer, there can not be a non-integral white level.
3. additionally, no white level for FP DNG's means that it is pre-normalized.
therefore, we can surmise that
a) FP DNG's never have white level of <1.0
b) FP DNG's always have integral white level
c) if FP DNG has a white level of `w`, it is always correct to divide by float(w) to normalize the image
... therefore, we can, as an optimization, store FP DNG white level as an integer

One-line commit messages (and PR descriptions) suck.
Please update the commit message with that wording.

But even with that, #638 is correct.
Encoding the state "we don't actually know what the value is" via some magic numbers is just wrong.

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (db1b955) 60.78% compared to head (75ebaaa) 60.80%.
Report is 17 commits behind head on develop.

Files	Patch %	Lines
src/librawspeed/decoders/DngDecoder.cpp	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #636      +/-   ##
===========================================
+ Coverage    60.78%   60.80%   +0.01%     
===========================================
  Files          266      266              
  Lines        15947    15947              
  Branches      2047     2047              
===========================================
+ Hits          9694     9696       +2     
+ Misses        6125     6123       -2     
  Partials       128      128              

Flag	Coverage Δ
benchmarks	10.54% <0.00%> (ø)
integration	46.01% <0.00%> (ø)
linux	57.26% <0.00%> (ø)
macOS	24.30% <0.00%> (ø)
rpu_u	46.01% <0.00%> (ø)
unittests	21.44% <0.00%> (ø)
windows	∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kmilos]

But even with that, #638 is correct.

Formally, yes. But thanks for reconsidering, I feel it makes sense and is the pragmatic thing to do here.

I'll improve the commit message. Shall I put those comments in the code as well?

[LebedevRI]

I'll improve the commit message. Shall I put those comments in the code as well?

Yes, i think something like this should be best:

// A white level of the image, if known.
// NOTE: it is always correct to divide the pixel by float(whiteLevel) to normalize the image.
// NOTE: for floating-point images, the white level is never non-integral, and thus >= 1.0f
Optional<int> whiteLevel;

[LebedevRI]

@kmilos Thank you!