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

Black level is re-added to output, leading to inefficient use of representable dynamic range #216

Open
Entropy512 opened this issue Nov 23, 2022 · 11 comments

Comments

@Entropy512
Copy link

Approximately a year ago, I did some work with hdrmerge and originally got very poor results (banding in shadows). Back then, I just changed the output depth from float16 (default) to float32 and went on my merry way, not thinking much of it.

I recently was looking in more detail at the metadata of the resulting DNG for other purposes, and noticed that the black level was 512 (which is normal for a Sony camera, but abnormal for an image that has had the kind of processing hdrmerge performs).

The problem with this is that when the black level is 512 in the output, a large portion of the dynamic range that can be represented by a float16 number is unused - above 512, the interval between possible representable numbers is 0.5 - https://en.wikipedia.org/wiki/Half-precision_floating-point_format#Precision_limitations

The original black level of the camera is subtracted immediately on load at https://github.com/jcelaya/hdrmerge/blob/master/src/ImageIO.cpp#L272, and all further processing is performed with a black level of 0.

The black level is re-added on export at https://github.com/jcelaya/hdrmerge/blob/master/src/ImageStack.cpp#L467 , leading to significant truncation of precision in the shadows with the default output of float16 if the camera has a nonzero black level

Images should be export with a black level of 0 (e.g. do not re-add the original camera black level) to maximize use of the dynamic range that can be represented by float16

@Beep6581
Copy link
Collaborator

@Entropy512 have you tested how such a change would affect opening those DNG files in e.g. RawTherapee or libraw-using programs like darktable?

@Entropy512
Copy link
Author

Not yet, I plan on taking a look at this after the holiday weekend. I just noticed the black level being 512 last night when looking at the DNG for other purposes and having a light bulb in my head turn on explaining why I might have had such poor results with float16 last year, and started digging through the code before I left for work this morning. I know you mentioned wanting to do an HDRMerge release after finishing RT 5.9, and this is something that merits some investigation beforehand.

I think i've determined roughly how this could be fixed - hardcode the output black/white levels near line 467 of ImageStack.cpp along with the DNG writer, then test in RT and DT.

If this fails in RT and DT, I would consider it to be a bug/undesirable behavior from RT of applying camconst JSON white/black levels (which are intended for SOOC raw images) to DNGs even if the DNG metadata indicates different white/black levels when the camera model is detected.

@Entropy512
Copy link
Author

RT handles the WIP patch at Entropy512@a331ae2 very well - I get nearly identical results using the old implementation with float32 when using this patch and float16.

Output DNGs for old float16/32 and new float16, plus a quick sample of RT output (+12EV exposure compensation, not my typical processing method for this shot but it's simple and demonstrates the bug with the old float16 implementation effectively) at https://filebin.net/i4twvwugzx1enm91

@Entropy512
Copy link
Author

I'll upload the original ARWs sometime after the holiday - I just noticed that it seems like in this particular case, something broke that is causing all shadow detail to get obliterated, even though the input +6EV image has good shadow detail. It affects both the old and new approach.

@Entropy512
Copy link
Author

Entropy512 commented Nov 24, 2022

I suspect this issue is why @Floessie saw an extremely small float16 result in #113 - and the other issue I'm having may be the same as his issue with float32 there, needs more investigation. Will provide the full bracket set once my family leaves for the holidays, and also when you can confirm you'll be around to grab the files within filebin's timeout.

Edit: Confirmed, the other issues I have appear to be similar to #113, and @fanckush 's patch in that thread definitely improves the situation. Will upload an example image set once the family leaves.

Entropy512 added a commit to Entropy512/hdrmerge that referenced this issue Nov 24, 2022
There was a rejected hunk against ImageStack where I made different changes for jcelaya#216

DO NOT MERGE/CHERRY-PICK - commit authorship is wrong and needs to be fixed!
@Floessie
Copy link
Contributor

@Entropy512 Thank you for picking this up. Extreme stacks are still a problem with float16 (and sometimes even with float32, as you just experienced).

@Entropy512
Copy link
Author

I'll try and upload the ARWs for my test stack tonight since you and @Beep6581 seem to be paying more attention. Unfortunately filebin is at capacity and not accepting new uploads right now.

@fanckush 's patch addresses multiple issues - I disagree with the approach for solving this one - it sets the white level to the maximum representable value for an unsigned int with the same number of bits - this is approximately the same for signed float16 (but 65535 rounds to infinity!), but not true for float24 and float32. I am concerned that extremely large numbers might lead to issues with some software (including accurately representing the white level in the DNG metadata), so it's probably better to choose a fixed number that optimizes float16 efficiency but should also be quite effective for float24 and float32 even if it doesn't use all of the range available. 65504 is maximum efficiency for float16, 32767 is a "nicer" number though even if it gives up a small amount of range?

With the black level fix, float16 alone works quite well if you don't get hit by the other issues.

We still should track #113 separately because there are still stacking issues. @fanckush 's patch leads to significant improvement there, but I want to understand what is going on better. I agree with him, the ImageStack code is a bit hard to read/understand, especially what is going on in computeResponseFunction. I'm not understanding why reversing the order seems to improve things since the response functions seem independent, but I must obviously be misreading the code because they're clearly not.

My stack might also reproduce #170 - I'm noticing that sometimes there are regions of black inside of the Christmas lights, which are obviously the most brightly lit parts of the scene. (Yes, I'm attempting to tonemap this aggressively enough that you can see the ambient room, and also discern the facet patterns within each light. Although it may turn out that feeding individual exposures to enfuse rather than tonemapping an HDRMerged stack in RT winds up better for this application...)

@kmilos
Copy link

kmilos commented Jul 10, 2023

If this fails in RT and DT, I would consider it to be a bug/undesirable behavior from RT of applying camconst JSON white/black levels (which are intended for SOOC raw images) to DNGs even if the DNG metadata indicates different white/black levels when the camera model is detected.

I think dt should handle valid DNGs just fine as well (please merge the 24-bit FillOrder PR though), and shouldn't override DNG metadata from its own model database.

@kmilos
Copy link

kmilos commented Jul 10, 2023

Haven't seen the patch - is the new white point also corrected after black level substraction?

Could potentially, as a last step, stretch by 1023/(white-black) to make max use of float16 quantisation range for the middle exposure? (I guess this stretching would make sense only if starting data was 10b like smartphone DNGs...)

@Entropy512
Copy link
Author

Not sure where you got 1023 from, maximum non-inf value representable by float16 is 65504:
Entropy512@a331ae2#diff-1e833bf43749b1d1bad5e13ce7654e232a3c97771efcfc593f6fc256961a70b5R270

I decided not to stretch further for float24 and float32 as the benefit would be fairly minimal (I think float16 on its own is more than enough for the majority of use cases with the fixes being discussed here) and the risk of some downstream software choking on it because of a hardcoded clamp somewhere is greater.

@kmilos
Copy link

kmilos commented Jul 11, 2023

Not sure where you got 1023 from, maximum non-inf value representable by float16 is 65504

It's not about the range, but the precision: https://en.wikipedia.org/wiki/Half-precision_floating-point_format#Precision_limitations

Should've been 2047 though (I assumed 1023 because of 10b mantissa, but forgot the implied leading bit) - after that you no longer can represent every integer as the quantization step goes above 1... Raws not coming from smartphones typically have more than 11b so one could potentially start rounding to nearest permitted levels.

Anyway, it is a moot point - what matters is that even if steps are growing, it is only their relative size to the value that counts and one should be staying below contrasts sensitivity [1] no matter the scaling and rounding (well, unless you try to represent an extremely high luminance w/ 65504).

[1] see p17 at https://www.color.org/hdr/04-Timo_Kunkel.pdf

Indeed, in 24b and 32b float there is no point worrying about it, there is plenty of precision and range.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants