jcelaya / hdrmerge

HDR exposure merging
http://jcelaya.github.io/hdrmerge/
Other
362 stars 78 forks source link

Black levels for x-trans files are not read correctly #112

Closed sguyader closed 6 years ago

sguyader commented 6 years ago

This issue is related to #78, I think it's a general problem with Fujifilm x-trans raw files.

I have the same issue with the X-T2: preview is black, and the output DNG is pinkish. More correct colors can be retrieved by tweaking black levels in RawTherapee, but the resulting image is noisy.

In fact, in file RawParameters.cpp (line 197) I replaced black=r.color.cblack; with black=1022; ( I guess r.color.black gets a value of 0.) and this gives me an output dng with correct black levels and colors for my camera, but what I see is that in fact, only the darkest frame of the bracket is used and its brightness amplified, this is why the output is noisy.

Beep6581 commented 6 years ago

Confirmed that black is detected as 0. I opened an issue over at LibRaw to find out more: https://github.com/LibRaw/LibRaw/issues/107

sguyader commented 6 years ago

Thanks @Beep6581 Would making HDRMerge read the correct black level also solve the issue that only the pixels of the darkest frame are returned in the output DNG?

Beep6581 commented 6 years ago

I set black to 1022, set the white level to 15078 just to be safe (16100 - 1022), and the preview was still black. Commented-out adjustBlack();, no difference.

The next question is whether the black preview bug is indeed only a preview bug, or whether the input really is black. We could be able to tell based on the output, but not using the supplied images, because they are all underexposed, so even if this worked perfectly HDRMerge would still only use the brightest image alone. @sguyader it would help if you could supply a better bracketed set, one where the darkest image has no clipping and the brightest image has a lot of clipping, such as this: imgur-2017_11_06-19 58 32

sguyader commented 6 years ago

OK I'll do that later. However, in my case it's the darkest image, and not brightest one, that is used by HDRMerge. To see this, I compared in RT the DNG output (made with a modified RawParameters.cc with cblack=1022) with the 3 original files, boosting the exposure to make them as bright. And what I see is that the noise in the shadows is exactly the same in the DNG output as in the pushed darkest image. Also, note that with this modified build the preview is still dark, so maybe I didn't hard-code the black level in the right place, and this black level is just passed to the output file while not being used internally in HDRMerge.

sguyader commented 6 years ago

@Beep6581 here's a set of files, which I hope will be okay for our purpose: https://filebin.net/5cdl1y3dze1s9ge7

sguyader commented 6 years ago

I can get the preview work" by hardcoding the value 1022 for d.params.user_black and d.params.user_cblack[c] in file ImageIO.cpp. Edit: scratch that, I was wrong. But still, HDRMerge always uses the darkest image and pushes it to normal brightness (the saved mask is all white). When I load 3 raw files in HDRMerge, in the "status bar" at the bottom of the window, it always shows "Layer 3" wherever I place the mouse cursor, and it corresponds to the darkest layer, while when I load raw files from a supported camera, the indications jumps between Layers 1, 2 and 3, and the darker areas correspond to Layer 1, brightest parts to Layer 3. It's almost as if HDRMerge treats the layers in reverse order and thinks that the darkest one is the brightest.

Beep6581 commented 6 years ago

Ref:

The black levels need to be taken from imgdata.color.cblack[6..41]

unsigned cblack[4102]; Per-channel black level correction. First 4 values are per-channel correction, next two are black level pattern block size, than cblack[4]*cblack[5] correction values.

But even so, the preview is still completely black, and the output DNG uses only one of the source raws.

The filter describing the pixel order seems to be correctly detected as "9":

unsigned filters; Bit mask describing the order of color pixels in the matrix (0 for full-color images). 32 bits of this field describe 16 pixels (8 rows with two pixels in each, from left to right and from top to bottom). Each two bits have values 0 to 3, which correspond to four possible colors. Convenient work with this field is ensured by the COLOR(row,column) function, which returns the number of the active color for a given pixel. values less than 1000 are reserved as special cases:

1 - Leaf Catchlight with 16x16 bayer matrix; 9 - Fuji X-Trans (6x6 matrix) 3..8 and 10..999 - are unused.

However, when I change it to b4b4b4b4 (r.idata.filters = 3031741620;), then the preview is fine: screenshot_20171107_110557

Though now the saved image is detected as Bayer and so is broken.

Hopefully someone who can actually speak Greek++ can do something about this :]

sguyader commented 6 years ago

Thanks for your help.

The black levels need to be taken from imgdata.color.cblack[6..41]

Are you sure about that? RT seems to use only the 4 per-channel black level values (cblack[0..3]) Also, I get correct black level values and correct colors in dng output when I hack cblack[] in RawParameters.cpp this way, adding the 4 lines below at lines 199-202:

    black = r.color.black;
    copy_n(r.color.cblack, 4, cblack);
    cblack[0] = 1022;
    cblack[1] = 1022;
    cblack[2] = 1022;
    cblack[3] = 1022;
    adjustBlack();

I get the same wrong colors in output if I set cblack[0..3] to "0" instead of "1022", or if I comment out the added lines.

Beep6581 commented 6 years ago

Are you sure about that?

There's very little I'm sure of.

RT seems to use only the 4 per-channel black level values (cblack[0..3])

RT uses dcraw and its own herbs and spices. HDRMerge uses libraw. Having said that, RT seems to be using 6 as well, see https://github.com/Beep6581/RawTherapee/blob/dev/rtengine/rawimage.cc#L95

You don't need to set 1022 for each channel, just set it for black, and adjustBlack(); will propagate it into the 4 channels since cblack[0-3] == 0.

By the way, it's RawParameters.cpp, not .cc ;] I fixed it in your first post yesterday.

sguyader commented 6 years ago

Having said that, RT seems to be using 6 as well, see https://github.com/Beep6581/RawTherapee/blob/dev/rtengine/rawimage.cc#L95

If I understand the comment in this file, RT reads only the value of cblack[6] (because all 36 cblack[6..41] values are identical) and then it populates its own cblack_[0..3] with this value?

You don't need to set 1022 for each channel, just set it for black, and adjustBlack(); will propagate it into the 4 channels since cblack[0-3] == 0.

I'm such a noob...

By the way, it's RawParameters.cpp, not .cc ;] I fixed it in your first post yesterday.

Thanks, I edited my last post too.

heckflosse commented 6 years ago

@sguyader

If I understand the comment in this file, RT reads only the value of cblack[6] (because all 36 cblack[6..41] values are identical) and then it populates its own cblack_[0..3] with this value?

correct

sguyader commented 6 years ago

Being a noob, I don't mind testing what may be crazy... So, in RawParameters.cpp I changed black = r.color.black to black = r.color.cblack[6]. And to my surprise, the output DNG looks right in terms of colors (but it's still made only from the darkest frame). AND, it also works with Pentax files (tested with "amsterdam_moving_boat" files).

Edit: this modification also works with Nikon D800 files.

Also, by setting the custom white level to what looks right for my camera (16250) the preview is no longer black, though it is always as dark as the darkest frame loaded.

sguyader commented 6 years ago

Who may help for this issue?

heckflosse commented 6 years ago

This patch possibly fixes the issue:

diff --git a/src/ImageStack.cpp b/src/ImageStack.cpp
index 83d22e7..53ecb09 100644
--- a/src/ImageStack.cpp
+++ b/src/ImageStack.cpp
@@ -62,7 +62,7 @@ void ImageStack::calculateSaturationLevel(const RawParameters & params, bool use
     }
     satThreshold = params.max == 0 ? maxPerColor[0] : params.max;
     for (int c = 0; c < 4; ++c) {
-        if (maxPerColor[c] < satThreshold) {
+        if (maxPerColor[c] > 0 && maxPerColor[c] < satThreshold) {
             satThreshold = maxPerColor[c];
         }
     }
diff --git a/src/RawParameters.cpp b/src/RawParameters.cpp
index a8b9b88..b9d8c6c 100644
--- a/src/RawParameters.cpp
+++ b/src/RawParameters.cpp
@@ -196,6 +196,11 @@ void RawParameters::fromLibRaw(LibRaw & rawData) {
     max = r.color.maximum;
     black = r.color.black;
     copy_n(r.color.cblack, 4, cblack);
+    if(r.idata.filters == 9) { //xtrans
+        for (int c = 0; c < 4; c++) {
+            cblack[c] = r.color.cblack[6];
+        }
+    }
     adjustBlack();
     copy_n(r.color.pre_mul, 4, preMul);
     copy_n(r.color.cam_mul, 4, camMul);

background: calculateSaturationLevel() calculates the minimum value of the per channel maximum values of the brightest image. This works fine for bayer sensor because we have 4 channels (R,G1,B,G2). For xtrans we have only three channels (R,G,B) but the code to get the minimum value used 4 channels and so always picked 0 as the minimum value.

sguyader commented 6 years ago

Thank you very much Ingo for fixing the issue, and giving the background!

heckflosse commented 6 years ago

@sguyader De rien.

cribari commented 6 years ago

I believe this is related to the following Darktable bug: https://redmine.darktable.org/issues/11340