Black levels for R,G, B.

[Beep6581]

Originally reported on Google Code with ID 717

This patch is a possible answer to the question from "Deatrock" in: http://www.rawtherapee.com/forum/viewtopic.php?t=2615

In the tab "Raw white point" I put "Black Level" which acts separately on the R, G,
B.
Negative values ​​are usable only on the camera whose values ​​"cblack" were not clipped
(eg Canon).
Positive values ​​are used on all cameras.

Generally small values ​​(-2 to -10) are sufficient to recover the "black" to zero.
One can use the histogram Raw to make this work.

For example, on "King image" it is possible to remove the colored pixels.

Reported by jdesmis on 2011-06-04 05:06:07

---

• Attachment: blaclev.patch

[Beep6581]

A cleanup that does not affect the results

Reported by jdesmis on 2011-06-04 05:34:29

---

• Attachment: blaclev1.patch

[Beep6581]

Hello Jacques,

Thanks for the addition, some technical feedback:
Are you sure you want to set maximum values in raw white point only above 65000? From
my experience the clipping in some channels may occur way earlier, even after scaling.
Why is the int cbl[4] in ramimage.cc added? Does not seem to be used, just like the
new include you added.
As for the naming of the new parameters, why do You call it blackz, blacko instead
of readable versions like e.g. blackLevGreen, blackLevRed or something?
Why are resulting negative black levels additions (so total positive) not allowed?
As for the resources you must also add some HISTORY_MSG to have the display working.

Olli

Reported by oduis@hotmail.com on 2011-06-04 09:50:54

• Status changed: PatchSubmitted
• Labels added: Type-Enhancement, 3.1B1
• Labels removed: Type-Defect

[Beep6581]

Hello Olli

For "raw white point only 65000 Above" .. I do not understand ...

For # Include <guiutils.h> and CBL [4] "It needs cleaning, because no use.

blackz = blackzero
blacko = blackone
etc..
you can not directly call them "blackLevGreen" because it depends on the type of sensor
.. (Bayer or not).

I will consider how to incorporate HISTORY_MSG

Reported by jdesmis on 2011-06-04 10:43:05

[Beep6581]

Hi Jaques,
The 65000something is in expo_before_b.cc, line 71. You changed the initial max to
65535. So if there is a max below that, it’s not correctly detected.
As for the naming of blacks, you label them with red/green/blue in the GUI, so it should
be possible to clean up the same naming for the parameters.
As for the HISTORY_MSG I corrected some missing ones some days ago, they are linked
to the event IDs.
Olli

Reported by oduis@hotmail.com on 2011-06-04 13:14:46

[Beep6581]

I'm also not understanding Olli's comment about raw white point; line 71 of expo_before
is a free_Array command...

If this is to be implemented, I think it would be more useful to have the slider variables
be double instead of int.  Integer shifts of channel black points don't have sufficient
granularity.

Reported by ejm.60657 on 2011-06-04 14:19:12

[Beep6581]

I still do not understand ... for "expo_before_b.cc" ... If you speak of the variable
MaxVal, it was initialy to 65535 and I went to 0
- MaxVal int = 65535;
+ MaxVal int = 0;

Emil, I agree in principle to pass the variable type "int" to "double", but the reference
variables "cblack []" are "int"...In this cas all "cblack[]" must be "double"

Jacques

Reported by jdesmis on 2011-06-04 14:29:40

[Beep6581]

Sorry, for the maxval, I mixed up before and after.

Reported by oduis@hotmail.com on 2011-06-04 14:31:59

[Beep6581]

RawData is a float type.  A logical place to apply this offset is as an additive shift
to RawData array values.  We need not be constrained by data types from dcraw.

Reported by ejm.60657 on 2011-06-04 15:18:59

[Beep6581]

Jacques, why is the range so large (-2048 to 100)?  I would think -100 to 100 at most
would be sufficient, and maybe even -10 to 10.

Reported by ejm.60657 on 2011-06-04 16:20:43

[Beep6581]

2048 is the maximum cblack for a Canon, but obviously the values ​​used are smaller,
I think you're right, we can safely restrict them to +-100

Reported by jdesmis on 2011-06-04 16:50:39

[Beep6581]

Some improvements following discussions:

* Transformation "cblack" from "int" to "float"

* Restrictions on the amplitude + -50

* Possible adjustment of the cursor (double): 0.1

* rename variables to make them more explicit: (eg: cblackz ==> cblackzero)

* 'HISTORY_MSG' appear

* Removed unnecessary variables

*...

Reported by jdesmis on 2011-06-05 08:22:28

---

• Attachment: blacklevel2.patch

[Beep6581]

it's better with the good patch

Reported by jdesmis on 2011-06-05 08:36:51

---

• Attachment: blacklevel3.patch

[Beep6581]

Good morning Jacques,

Thanks for incorporating the feedback, getting in professional shape!
Now the variables are named blackzero, blackone, blacktwo, blacktr   (tr=three?), while
in the GUI they are called Black level Red, Green, Blue, Green2.
Why not call the parameters red/green/blue also, so every dev knows how they map? Or
call everything like black0, black1, black2...?

Olli

(PS: Sorry if I am the star of your sweating nightmares now ;-)

Reported by oduis@hotmail.com on 2011-06-05 08:54:59

[Beep6581]

hello Olli

I repeat, the variables "cblackzero", "cblackone", "cblacktwo" (cblacktr is not used...second
channel green ??) refer to the sensor ... According to its type (Bayer or not) or not
these variables correspond to the colors:
Bayer:
blackzero = Green
blackone = Red
blacktwo = Blue

Others:
blackzero = Red
blackone = Green
blacktwo = Blue

In the GUI the values ​​displayed are "red", "Green", "Blue ", and where it is performed
in "rawimagesource" correspondence to black_lev[] with values ​​read.

Another point: everyone has their view of things ... Some time ago I was a consultant
and specialist in social sciences and humanities (change management, human factors,
safety, project ...)... What may seem obvious to one is obscure to another, and sometimes
a source of conflict. For my part, long variable names are not explicit. When the explicit
naming of variables and functions of D. Coffin is often a guessing game.

But what do you think of the use?

Reported by jdesmis on 2011-06-05 09:28:11

[Beep6581]

Hi Jacques,

I already understood that the mapping may be different depending on sensor type. The
bug from my point of view is that it’s not done all the way through. If I understand
the patch correctly, the black level 1 is always labeled as “Black level RED” in the
GUI, though it is GREEN on some non-Bayer sensors.
So either we make it short cut like “only supported on Bayer sensors” (and the variable
names accordingly blackRed etc.), or fix the labeling (dynamically mapping the GUI
according to RAW sensor type of the image).
If a users changes RED and it affects GREEN instead he’ll be quite astonished (and
shout at us how this big bug could pass the quality check ;-)

> (cblacktr is not used...second channel green ??)
That’s what I meant, what does the variable naming “tr” stand for? ;-)  And when it’s
always zero (if I understand correctly), I do not understand why it is still there
in the code (just adds confusion).

As for variable naming, there are different approaches but all agree that it should
be explicit (e.g. something like "blackLevGreen2" (for black level green channel 2)
instead of more cryptic "blacktr"). On Wikipedia there is a nice sum up and some reasoning
why that’s good:
http://en.wikipedia.org/wiki/Naming_convention_(programming)

Olli

Reported by oduis@hotmail.com on 2011-06-05 10:05:59

[Beep6581]

OK for the link ... But it does not change anything about my vision of working together
and differences of viewpoint ... 

I worked in business for over 10 years on this, and every time, which is normal for
one is abnormal for another. Wanting to normalize refers to the theories of regulation.
In the world there are several schools, the "powers", the "identities" ,  "aurorities",
the "rules"... and refer to authors as "Simon", "Crozier", "Reynaud". ..
Of course there are the beliefs in the Quality and standards (ISO. ..)

Of course every player believes in what he said (written) and it's normal ... but the
partner has a different view. This explains the conflict, including in the workplace,
between countries (France, Germany ,..), politics, etc..

Reported by jdesmis on 2011-06-05 10:26:29

[Beep6581]

I have not looked at the code in detail, so I don't know it operates.  But I agree with
Olli's principle -- the presentation should be uniform to the user; sliders should
always mean the same thing.  Why does it have to be different for Bayer and non-Bayer?
 I can't imagine what is useful about this mapping:

Bayer:
blackzero = Green
blackone = Red
blacktwo = Blue

Others:
blackzero = Red
blackone = Green
blacktwo = Blue

One suggestion for this: there should be four sliders, since there is in principle
a difference between the black levels of the two greens.  For non-Bayer, the second
green slider can be 'greyed out' much as the shadow recovery slider is when black level
= 0.  It would be nice also to have a keyboard acceleration, that if one say holds
down the control or shift key and moves either green slider, they are both adjusted
identically.  That saves time when one doesn't want to treat the two greens individually.
 Alternative would be a check button that sets both green blackpoints equal (and greys
out the second slider).

Reported by ejm.60657 on 2011-06-05 13:04:35

[Beep6581]

I would do it as follows:

1. Data types from rtgui set by four sliders are blackR,blackG1,blackG2,blackB.

2. Define offsets

if( ri->isBayer() ) {
black_lev[0]=raw.blackR;
black_lev[1]=raw.blackG1;
black_lev[2]=raw.blackB;
black_lev[3]=raw.blackG2;
}
else {
black_lev[0]=raw.blackR;
black_lev[1]=raw.blackG1
black_lev[2]=raw.blackB;
black_lev[3]=raw.blackG1;
}

Reported by ejm.60657 on 2011-06-05 14:05:11

[Beep6581]

I agree about the readability ... but in this case, I think
Bayer
blackzero = Green
blackone = Red
blacktwo = Blue

Others:
blackzero = Red
blackone = Green
blacktwo = Blue

is true because it is similar to code that is in "getRawhistogramm"
isgreen ==> cblack [0]
isred ==> cblack [1]
isblue ==> cblack [2]
What is more readable cblack [0] or blackzero ? for me it's the same level...

Brief agreed for readability ... but watch for the proper functioning ....

I am trying to add the fourth cursor to the 2nd green channel, it works (I changed
getRawhistogram accordingly).

I still have to improve (gray, etc.,) as suggested by Emil.

Reported by jdesmis on 2011-06-05 16:48:44

[Beep6581]

Sorry, I assumed the standard dcraw convention applied: R=0, G1=1; B=2; G2=3.  Why is
it different?

Reported by ejm.60657 on 2011-06-05 17:20:29

[Beep6581]

I am also surprised by the data structure cblack []...

I modified the patch:
* "float" instead of "int" for cblack[]
* limitations + -50
* Precision=  0.1
* second green channel
* ability to process two channels green simultaneously (green1 is the priority)
* change (very slight) of "getrawhistogram" to take into account the fourth channel

But I have no pictures "No Bayer" to test ...

Reported by jdesmis on 2011-06-07 11:23:41

---

• Attachment: blacknew.patch

[Beep6581]

Hi Jacques, I'll try to test as soon as I have time.  You can probably get Foveon raws
from  dpreview.com if they have reviews of recent Sigma cameras -- for instance go
to the bottom of the page here:

http://www.dpreview.com/reviews/sigmadp2/page9.asp

Reported by ejm.60657 on 2011-06-07 13:51:49

[Beep6581]

Hello Emil

Files Foveon (Sigma) are not recognized by RawTherapee ...

I made a slight improvement in the patch.

Reported by jdesmis on 2011-06-07 15:59:17

---

• Attachment: blacknew1.patch

[Beep6581]

Hmm.  I wonder why we have all this raw processing code for non-Bayer files...?

Reported by ejm.60657 on 2011-06-07 16:14:39

[Beep6581]

Emil

I tried with 2 files "X3F" ... could not open raw ... (at least on my computer)

I do not know what types of files "no Bayer" that can open RT?

Reported by jdesmis on 2011-06-07 16:22:33

[Beep6581]

Some little technical feedback:
black_lev is defined as object-global, but it seems to be just used temporarily in
scaleColors. It's better to move it locally into the scaleColors function.

Reported by oduis@hotmail.com on 2011-06-07 17:31:29

[Beep6581]

Olli

Thank's for this advice.
Done.

Jacques

Reported by jdesmis on 2011-06-07 17:49:33

---

• Attachment: blacknew2.patch

[Beep6581]

Man, you are quick! Looks great.

Reported by oduis@hotmail.com on 2011-06-07 17:51:16

[Beep6581]

No new features. Just an adaptation following the amendment of "Hombre" (78b1fa904738)
which introduces a "Hunk" in the patch "blacknew2" with Mercurial...

Reported by jdesmis on 2011-06-08 06:55:52

---

• Attachment: blacknew3.patch

[Beep6581]

Committed to DEFAULT.

Reported by oduis@hotmail.com on 2011-06-08 18:21:28

• Status changed: Fixed