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

Tonecurve - LCh tab #2017

Open
wants to merge 23 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@phweyland
Copy link
Contributor

phweyland commented Jan 19, 2019

As tonecurve changes are sometime critical for saturation I think it is interesting to be able to fix it in the same module.

That's why I've made a try to add an LCh mode to tonecurve.
There are 3 tabs: L (same as for Lab), C(L) and C(h). The last two let the user apply a correction on C based on L and h.
I've experimented that C and h tabs are not very helpful, but C(L) and C(h) tabs seem to me more convenient.

I've found some difficulties due to the fact tonecurve is truly Lab oriented. Therefore there are lot of specificities which could be avoided adapting the design to a more general case.

Let me know if that can be interesting.
Your comments are welcome.
If the function make sense for you I'll work the open cl piece.

@TurboGit TurboGit self-requested a review Jan 19, 2019

@TurboGit TurboGit self-assigned this Jan 19, 2019

@TurboGit TurboGit added this to the 2.8 milestone Jan 19, 2019

@aurelienpierre

This comment has been minimized.

Copy link
Contributor

aurelienpierre commented Jan 19, 2019

I don't understand why the hue gets involved in the chroma correction. The chroma is the colorfulness, more or less equivalent to the saturation, you usually want to keep it separated from the hue (that's the whole purpose of the Lch space). Hues shifts are nearly impossible to handle with curves, hence the color zones module, so I think you should keep only the L and C channels, keep C away from any interaction with H, and let the hue handling for the color zones module.

{
const float pi = 3.14159f;
const float C_in = sqrtf(in[1]*in[1] + in[2]*in[2]) / 181.019f;
const float h_in = ((in[1]!=0.0f ? atanf(in[2]/in[1]) : (in[2]>0.0f ? 0.5f : -0.5f) * pi) + ((in[1] < 0.0f) ? pi : ((in[2] < 0.0f) ? 2.0f * pi : 0.0f))) / (2.0f * pi);

This comment has been minimized.

@aurelienpierre

aurelienpierre Jan 19, 2019

Contributor

there is an atan2(y, x) function doing just that (computing atan(y/x) in the right quadrant)

This comment has been minimized.

@phweyland

phweyland Jan 20, 2019

Author Contributor

Great ! Done.

const float pi = 3.14159f;
const float C_in = sqrtf(in[1]*in[1] + in[2]*in[2]) / 181.019f;
const float h_in = ((in[1]!=0.0f ? atanf(in[2]/in[1]) : (in[2]>0.0f ? 0.5f : -0.5f) * pi) + ((in[1] < 0.0f) ? pi : ((in[2] < 0.0f) ? 2.0f * pi : 0.0f))) / (2.0f * pi);
float C_out = L_in == 0.0f ? C_in : C_in * d->table[ch_CL][CLAMP((int)(L_in * 0x10000ul), 0, 0xffff)] * 0.02f;

This comment has been minimized.

@aurelienpierre

aurelienpierre Jan 19, 2019

Contributor

Please write a color conversion function in the lib src/common/colorspaces_inline_conversion.h

This comment has been minimized.

@aurelienpierre

aurelienpierre Jan 19, 2019

Contributor

I have a SSE one in progress:

static inline __m128 dt_Lab_to_Lch_sse2(const __m128 Lab)
{
  __m128 Lch = Lab;
  __m128 Lab2 = Lab * Lab;
  Lch[1] = powf((Lab2[1] + Lab2[2]), 0.5f);
  Lch[2] = atan2f(Lab[2], Lab[1]);
  return Lch;
}

static inline __m128 dt_Lch_to_Lab_sse2(const __m128 Lch)
{
  __m128 Lab = Lch;
  Lab[1] = cosf(Lch[2]) * Lch[1];
  Lab[2] = sinf(Lch[2]) * Lch[1];
  return Lab;
}

This comment has been minimized.

@phweyland

phweyland Jan 20, 2019

Author Contributor

Need more time to understand this.

This comment has been minimized.

@phweyland

phweyland Jan 20, 2019

Author Contributor

I've added open cl support as done in the module. It uses basic.cl and colorspaces.cl.
It doesn't use SSE. Could be interesting to improve that.

const float C_in = sqrtf(in[1]*in[1] + in[2]*in[2]) / 181.019f;
const float h_in = ((in[1]!=0.0f ? atanf(in[2]/in[1]) : (in[2]>0.0f ? 0.5f : -0.5f) * pi) + ((in[1] < 0.0f) ? pi : ((in[2] < 0.0f) ? 2.0f * pi : 0.0f))) / (2.0f * pi);
float C_out = L_in == 0.0f ? C_in : C_in * d->table[ch_CL][CLAMP((int)(L_in * 0x10000ul), 0, 0xffff)] * 0.02f;
C_out = h_in == 0.0f ? C_out : C_out * d->table[ch_Ch][CLAMP((int)(h_in * 0x10000ul), 0, 0xffff)] * 0.02f;

This comment has been minimized.

@aurelienpierre

aurelienpierre Jan 19, 2019

Contributor

so you are shifting input hue by the same amount you shift the chroma ? Don't you want to preserve the hue untouched ? I don't understand what's going on here.

This comment has been minimized.

@phweyland

phweyland Jan 20, 2019

Author Contributor

Hue doesn't change. C is modified for the selected hue.

float C_out = L_in == 0.0f ? C_in : C_in * d->table[ch_CL][CLAMP((int)(L_in * 0x10000ul), 0, 0xffff)] * 0.02f;
C_out = h_in == 0.0f ? C_out : C_out * d->table[ch_Ch][CLAMP((int)(h_in * 0x10000ul), 0, 0xffff)] * 0.02f;
C_out = C_out * 181.019f;
const float h_out = h_in * (2.0f * pi);

This comment has been minimized.

@aurelienpierre

aurelienpierre Jan 19, 2019

Contributor

I think you should handle hue shifts directly in radians to spare a few divisions and multiplications, if you really want to rotate the hue.

This comment has been minimized.

@phweyland

phweyland Jan 20, 2019

Author Contributor

No hue shift. Hue is just used as an index in the C table (needs 0->1).
Div and mul saving done.

C_out = C_out * 181.019f;
const float h_out = h_in * (2.0f * pi);

out[1] = C_out * cosf(h_out);

This comment has been minimized.

@aurelienpierre

aurelienpierre Jan 19, 2019

Contributor

you might want to use Taylor-Young approximations instead of pure cos/sin to speed-up the computation

This comment has been minimized.

@phweyland

phweyland Jan 20, 2019

Author Contributor

image
Why not if the accuracy is enough. Order 7 seems ok, but is it worth for calculation time ?

{
const float pi = 3.14159f;
float C_m = sqrtf(in[1]*in[1] + in[2]*in[2]) / 181.019f;
float h_m = (atanf(in[2]/in[1]) + ((in[1] < 0.0f) ? pi : ((in[2] < 0.0f) ? 2.0f * pi : 0.0f))) / (2.0f * pi);

This comment has been minimized.

@aurelienpierre

aurelienpierre Jan 19, 2019

Contributor

same, you might want to use atan2

@@ -344,6 +347,19 @@ void process(struct dt_iop_module_t *self, dt_dev_pixelpipe_iop_t *piece, const
: d->table[ch_b][CLAMP((int)(b_in * 0x10000ul), 0, 0xffff)]);
}
}
else if(autoscale_ab == DT_S_SCALE_MANUAL_LCH)
{
const float pi = 3.14159f;

This comment has been minimized.

@aurelienpierre

aurelienpierre Jan 19, 2019

Contributor

pi is already set as a global constant M_PI I think

This comment has been minimized.

@phweyland

phweyland Jan 20, 2019

Author Contributor

ok. done

const float h_in = ((in[1]!=0.0f ? atanf(in[2]/in[1]) : (in[2]>0.0f ? 0.5f : -0.5f) * pi) + ((in[1] < 0.0f) ? pi : ((in[2] < 0.0f) ? 2.0f * pi : 0.0f))) / (2.0f * pi);
float C_out = L_in == 0.0f ? C_in : C_in * d->table[ch_CL][CLAMP((int)(L_in * 0x10000ul), 0, 0xffff)] * 0.02f;
C_out = h_in == 0.0f ? C_out : C_out * d->table[ch_Ch][CLAMP((int)(h_in * 0x10000ul), 0, 0xffff)] * 0.02f;
C_out = C_out * 181.019f;

This comment has been minimized.

@aurelienpierre

aurelienpierre Jan 19, 2019

Contributor

same here, I think building your curve LUT between 0 and 181.019 would spare one mul and one div by pixel (ie scale the LUT once, don't scale every pixel twice).

This comment has been minimized.

@phweyland

phweyland Jan 20, 2019

Author Contributor

LUT kept as is but saving done.

@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Jan 20, 2019

I don't understand why the hue gets involved in the chroma correction.
[..] so you are shifting input hue by the same amount you shift the chroma ?

The channels C(L) and C(h) do not modify L and h, but just apply correction on C for the selected L or h.
Per default the value 0.5 means no correction.

edit: for example, for this node a correction of -5.7% is made on C for the node's hue. For the color picker pixel, the C varies from 15.5 to 13.8 (correction cumulated with the C(L) channel is any).
image

I look at the other comments and come back to you.

@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Jan 20, 2019

One question.
Adding LCh doesn't change any parameter. From this standpoint there is no difference between version 4 and 5. Therefore is the version 5 necessary, desirable ?

@TurboGit

This comment has been minimized.

Copy link
Member

TurboGit commented Jan 20, 2019

@phweyland : no if there is no new parameter and if the current edit are 100% compatible with this new version you do not have to change the version.

What I understand from your comment (I didn't look at the code yet) is that this PR is only about a new GUI interface which are actually triggering the current parameters in a different way. Right?

@edgardoh

This comment has been minimized.

Copy link
Contributor

edgardoh commented Jan 20, 2019

This will introduce conflicts with PR 1841, maybe it will be better to wait until it is merged.

@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Jan 20, 2019

this PR is only about a new GUI interface which are actually triggering the current parameters in a different way. Right?

Exact.

if there is no new parameter and if the current edit are 100% compatible with this new version you do not have to change the version.

OK, I'll remove the corresponding code.

@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Jan 20, 2019

This will introduce conflicts with PR 1841, maybe it will be better to wait until it is merged.

Which kind of conflict ?

@edgardoh

This comment has been minimized.

Copy link
Contributor

edgardoh commented Jan 20, 2019

PR 1841 adds rgb curves, so more or less the same lines has been modified. But more important, it adds the ability for the modules to ask for a specific colorspace, so the transform must be done outside the iop. This allows the module to have a LCh histogram, but for that the curve will have to be modified, so strange things will have to be done to keep backwards compatibility.

@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Jan 21, 2019

@edgardoh, thanks for explanation. Do you have any idea when this merge is expected ?
Edit: by principle tonecurve can use whatever color space Lab, rgb, LCh and why not any other.
But this is known only after the user choice. Is the new design (PR 1841) considering this ?

@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Jan 21, 2019

I've noticed an issue on data shifts. I'm unsure how to handle that.
The goal of LCh is to make changes on L and C letting the other channels untouched.
This never happens ...
Some screen captures here:
tonecurve_calculation_issues.docx

  • Changing Luminance shifts a & b on color picker (same for LAB independant channels).
  • Changing Chroma shifts Luminance and Hue.

That may be a normal behaviour but that's disturbing to have a measure (color picker) which contradicts what we try to do ... :-)

Where and when color picker does make the measure (before or after output color profile) ?
What do you think ?
Is it possible to improve this ?

@edgardoh

This comment has been minimized.

Copy link
Contributor

edgardoh commented Feb 6, 2019

I did some testing and RawTherapee and PhotoFlow don't shift the middle grey, RawTherapee works on prophotoRGB (as far as I know) and PhotoFlow allows to select the working colorspace. No idea internally how they handle the curve and the histogram, but the code and authors are there.

My main concern is that the meaning of the curve should remain the same, no matter the colorspace. And a standard S curve is not supposed to shift the middle grey, as shown by the current presets "contrast * (linear)"

I'm not saying that we should do exactly the same as other programs, if we find something better then great, but by changing colorspaces there should be a way of having the same curve with the same meaning.

Take for instance the contrast, changing the colorspace will shift the middle grey, so in my "basic adjustments" I have added a fulcrum so it can be corrected when changing from a linear to a non-linear RGB (is a WIP, so is far from perfect), maybe something similar can be done with the curves and histogram.

Maybe @TurboGit has an opinion on this.

@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Feb 6, 2019

@TurboGit
Looking at the way to add the waveform histogram I'm wondering where to put that code.
A lot drawing routines are put in draw.h, which surprises me a bit because the code distributed in every file referencing it, using it or not. Shouldn't we setup a draw.c file ?

@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Feb 6, 2019

And a standard S curve is not supposed to shift the middle grey, as shown by the current presets "contrast * (linear)"

Yes because this particular preset sets a node on (x,y) = (50,50) given in Lab color space, which is weird because in the same time the preset sets the tonecurve mode to RGB ...
The other presets, which do not set a node on middle grey (50,50) (or (18,18) depending on the color space we use), do not preserve the middle grey.

But I agree we need a consistent method to display the histogram.
Today, the pipe works on Lab and the main histogram is displayed on monitor RGB, the tonecurve one in Lab ...

On one hand we could say that the tone curve histogram should follow the main one (monitor or display RGB).
But on the other hand the tabs a & b for Lab , C & h for LCh, have nothing to do with RGB.

Currently the tonecurve mode selects the working color space (on which the curve is applied).
We could let the user choose also the tonecurve histogram color space, whatever the working color space.

I see now that only one preset works with my change, the Lab one (Nikon 750). The others pretend to use RGB but provide Lab nodes...
Edit: I've fixed the issue on the preset, but as the Lab nodes are read in the working color space, the result is not as expected (by the preset). Another principle to establish: are the preset nodes in the tonecurve working space or always in Lab ?

Yes, others standpoint would be very welcome.

@TurboGit

This comment has been minimized.

Copy link
Member

TurboGit commented Feb 6, 2019

No, sorry no opinion on this. This is far from my expertise at this point!

@aurelienpierre

This comment has been minimized.

Copy link
Contributor

aurelienpierre commented Feb 7, 2019

Ok you need to separate the view and the data.

No matter the color space, the curve sets the middle grey at 50 % of the graph view. In commit_params(), the coordinates of the curve's LUT are then properly remapped in 1D to the target color space, meaning 50 % -> 18.45 % in RGB and XYZ modes:

if(p->tonecurve_autoscale_ab == DT_S_SCALE_AUTOMATIC_XYZ)
{
// derive curve for XYZ:
for(int k=0;k<0x10000;k++)
{
float XYZ[3] = {k/(float)0x10000, k/(float)0x10000, k/(float)0x10000};
float Lab[3] = {0.0};
dt_XYZ_to_Lab(XYZ, Lab);
Lab[0] = d->table[ch_L][CLAMP((int)(Lab[0]/100.0f * 0x10000), 0, 0xffff)];
dt_Lab_to_XYZ(Lab, XYZ);
d->table[ch_L][k] = XYZ[1]; // now mapping Y_in to Y_out
}
}

(But @phweyland, you have removed that part.)

Then, in process() the LUT mapping happens in the right color space.

So I think we are spinning around a non-issue. It's not a "Lab" or "RGB" curve, it's a curve drawn in a space where the middle grey is 50 % so the graph is centered (which happens to be Lab, but we don't care in that particular case. Could have been a simple ^2.35 too).

@edgardoh

This comment has been minimized.

Copy link
Contributor

edgardoh commented Feb 7, 2019

@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Feb 7, 2019

@aurelienpierre
(But @phweyland, you have removed that part.)

OK, I understand the purpose of this now. The current full tonecurve module UI (histogram, color picker, presets and saved values) is Lab based while some LUT operations are made on other color spaces (XYZ and RGB). Displaying the Lab luminance for RGB or XYZ is not really an issue (while counter intuitive from my stand point). I'll revert my changes on this (at least to keep the downward compatibility).

We could do the same (translate presets to Lab) for LCh and LRGB but some channels (C(L), C(h), R, G B) have nothing to do with Lab luminance. For these channels the Lab luminance is useless. We need to present to the user a meaningful histogram with the associated color picker and presets values.
Remains, as @edgardoh pinpoints, to choose the correct RGB color space to be displayed (histogram, color picker, presets). Linear RGB (grey at 18%), the one that @edgardoh doesn't like too much ? the sRGB one as on the main histogram (not sure that grey is at 50%) ?
sRGB would have the advantage to be homogeneus with the main histogram (linear, log and waveform when available).

@aurelienpierre

This comment has been minimized.

Copy link
Contributor

aurelienpierre commented Feb 7, 2019

Although having a Lab histogram is not super intuitive, it is consistent with the rest of the UI. For chroma-based channels, you can leave that linear, but you will need a special histogram. If you want an RGB histogram, you can convert each channel to Lab as if they were grey to build the histogram like that:

int histogram_bins[3][255] = { 0 }; // RGB histogram bins

#pragma openmp parallel for simd reduction(+:histogram_bins)
for( size_t k = 0; k < width * height * channels; k += channels)
{
    float *Lab = (float *const)roi_in + k;
    float RGB[3];
    dt_Lab_to_prophotorgb(Lab, RGB);

    float fake_R[3] = { RGB[0], RGB[0], RGB[0] }; // red × 3
    float fake_G[3] = { RGB[1], RGB[1], RGB[1] }; // green × 3
    float fake_B[3] = { RGB[2], RGB[2], RGB[2] }; // blue × 3

    // convert colors to pseudo-Lab for UI:
    // only the [0] element of the vector is relevant (pseudo-L)
    dt_prophotorgb_to_Lab(fake_R, ui_R);
    dt_prophotorgb_to_Lab(fake_G, ui_G);
    dt_prophotorgb_to_Lab(fake_B, ui_B);

    // RGB corrected in UI space:
    uint8_t UI_RGB[3] = { (uint8_t)ui_R[0], (uint8_t)ui_G[0], (uint8_t)ui_B[0] };

    // histogram_RGB can be put to 255 bins to process an histogram
    histogram_bins[0][ UI_RGB[0] ] += 1;
    histogram_bins[1][ UI_RGB[1] ] += 1;
    histogram_bins[2][ UI_RGB[2] ] += 1;
}

But of course, it's not super efficient from a performance point of view.

@aurelienpierre

This comment has been minimized.

Copy link
Contributor

aurelienpierre commented Feb 7, 2019

Also, sRGB is crap, because it has 2 different gamma depending the range of the input, so you have to branch stuff and the code is not vectorizable. If this is the path you want to take, just use a ^(1/2.45)

@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Feb 8, 2019

Although having a Lab histogram is not super intuitive, it is consistent with the rest of the UI.

I'll make a try with this philosophy. Thanks for the piece of code. That enlightens the way the histogram is built.

@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Feb 8, 2019

I've made some progress with LRGB with Lab mode histogram.
But I've 2 issues.

  1. The only place I've found in the module to get the input image is in process(). It's probably not the right place. Is there any other optional callback I could use instead ? When the callback doesn't provide the input buffer, is there a way to get it starting from dt_iop_module_t ?
  2. The histogram is not stable. Moving around a node of the curve makes the histogram tremble. Probably because dt_iop_tonecurve_draw() runs in parallel (without exclusion).
    My preference would be to build the histogram in dt_iop_tonecurve_draw() instead of process() but I haven't found the way to access the input image from there. Any idea ?
@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Feb 11, 2019

I've updated LRGB with Lab UI (presets, color picker, histogram), but open_cl.
That doesn't simplify the code but as @aurelienpierre said the UI is consistent with the rest of the module.
However, locally built histograms are still dancing...

@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Feb 16, 2019

@edgardoh
As @aurelienpierre suggested I find the Lab like histogram quite effective for R, G & B channels.
The problem is that the module is not the right place to make these calculations. I cannot get access to input buffer when executing dt_iop_tonecurve_draw(). On the other hand I cannot control the exclusion between process() and dt_iop_tonecurve_draw().
If I understand properly there are already histogram calculations for each module happening inside the pixelpipe execution. That would be the right place to do it, wouldn't be ? But we need to make the module able to choose the needed histogram. In some way that's what you are doing for the module's input/output colorspaces, but for the UI colorspace. What do you think ?

EDIT: Finally I've found the cause of instability of histogram: process() is called twice, once for preview and once for development. That's enough to get different data. So I think that the current way will work.

phweyland added some commits Feb 16, 2019

@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Feb 19, 2019

@TurboGit, I think the current state is ok to be reviewed.
Both LCh and LRGB independent modes are added, showing histograms in LAB to be coherent with others modes. Open cl is also implemented.

@aurelienpierre

This comment has been minimized.

Copy link
Contributor

aurelienpierre commented Feb 20, 2019

So what you call LRGB is the same curve applied on all RGB channels ?

Nice job for integrating the separate (manual) RGB channels ! I was planning to do it at some point, that's definitely helpful. I will have a look at the color spaces conversions in the next few days.

@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Feb 20, 2019

So what you call LRGB is the same curve applied on all RGB channels ?

In this mode, RGB independent channels, L is equivalent to RGB linked channels. R, G, B independent curves are applied just after.
NB: I'm making some tests adding to RGB linked channels the weighted yellow power norm described in the link you have provided on darktable-dev message Color management in HDR setups. I find the results interesting (not yet in that PR).
In fact we could propose several of these norms which control in some the behavior of the S curve.

@TurboGit. The build fails for some reason I don't understand. Any advice ?

@aurelienpierre

This comment has been minimized.

Copy link
Contributor

aurelienpierre commented Feb 20, 2019

In fact we could propose several of these norms which control in some the behavior of the S curve.

These fall into chrominance preservation modes. In filmic, the only mode used for now is the max(RGB), hence the L infinite norm. I have thought about making it more general and using other norms as well, so that could end up in a general API:

inline float norm_vec(float RGB[4], float norm)
{ 
  // ensure (norm >= 0.0f) in GUI controls for better perf

  // no RGB value shall be < 0 until we discover negative light energy
  // because of this, we can avoid to fabsf(RGB) and speed thins up
  if((RGB[0] < 0.0f || RGB[1] < 0.0f || RGB[2] < 0.0f)) return -1;

  // norm L infinite = max
  if(norm == 0.0f) return fmaxf(RGB[0], fmaxf(RGB[1], RGB[2]); 

  // norm L1 - bypass the powf for performance
  else if(norm == 1.0f) return RGB[0) + RGB[1] + RGB[2];

  // norm L2 = euclidian norm - bypass the powf for performance
  else if(norm == 2.0f) return sqrtf( RGB[0] * RGB[0] + RGB[1] * RGB[1] + RGB[2] * RGB[2]);

  // general Lp norm (pseudo-norm if p < 1) - slow variant
  else 
  {
    return powf( powf(RGB[0], norm) + powf(RGB[1], norm) + powf(RGB[2], norm), 1.0f/norm); 
  }
}
@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Feb 20, 2019

so that could end up in a general API:

Interesting. In the article you've indicated the 2 norms are:
Basic power norm: (R^3+G^3+B^3)/(R^2+G^2+B^2)
Weighted yellow power norm: 0.83743219(1.22R^5+1.20G^5+0.58B^5)/(1.22R^4+1.20G^4+058B^4)

Is the routine you've given above already merged ? Of course we could put together all valuable ones.
Comment: as a lut is applied on the output, it should be scaled to [0,1], right ?

@TurboGit

This comment has been minimized.

Copy link
Member

TurboGit commented Feb 20, 2019

@phweyland :
The issue is with clang only in the external rawspeed submodule.

I have noticed that you have changed the rawspeed submodule here. It should not be done, please make sure the submodule is not touched. Note that this is not the issue anyway, as current master fails with clang.

@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Feb 20, 2019

It should not be done, please make sure the submodule is not touched.

OK. If I understand I should not execute git submodule update, correct ?
Except maybe at each dt new release ? Otherwise how to know when I should do it ?

@TurboGit

This comment has been minimized.

Copy link
Member

TurboGit commented Feb 20, 2019

No, before commit you should always run (to be sure) "git submodule update". This will ensure that the module is set to the proper history before the commit. In this way there is "no change" for this module in your history and then the commit won't contains it.

The issue you have is probably that you have moved to master, did a submodule update (or was done as part of the git pull maybe?) and then back to your branch. Then the module was not back to the point it was on your branch, and so some modifications (change from master) have been committed.

@phweyland

This comment has been minimized.

Copy link
Contributor Author

phweyland commented Feb 20, 2019

@aurelienpierre
I've add the 2 norms I had plus yours to RGB linked channels. Please review the corresponding formulas. Of course we could remove the less relevant ones, but globally that seems promising.

@aurelienpierre

This comment has been minimized.

Copy link
Contributor

aurelienpierre commented Feb 20, 2019

so that could end up in a general API:

Interesting. In the article you've indicated the 2 norms are:
Basic power norm: (R^3+G^3+B^3)/(R^2+G^2+B^2)
Weighted yellow power norm: 0.83743219(1.22R^5+1.20G^5+0.58B^5)/(1.22R^4+1.20G^4+058B^4)

Is the routine you've given above already merged ? Of course we could put together all valuable ones.
Comment: as a lut is applied on the output, it should be scaled to [0,1], right ?

The routine above is just a project for now, nothing done yet. The L1, L2, Lp norms are algebraic vector norms ("real" vector maths), but the power and yellow weighted norms are only empiric ad-hoc formulas, so I'm very suspicious with that.

As for the LUT, theoritically yes, but the curve processing has an exponential interpolation branching for values above 1, and below 0 in some cases (which is idiotic if you ask me, but here we are…).

@aurelienpierre
Copy link
Contributor

aurelienpierre left a comment

I have reviewed the OpenCL variant only

}
case 2: // norm L1 - bypass the powf for performance
{
return (rgb.x + rgb.y + rgb.z) / 3;

This comment has been minimized.

@aurelienpierre

aurelienpierre Feb 20, 2019

Contributor

this is not a norm, but a mean. You should not divide by 3

This comment has been minimized.

@aurelienpierre

aurelienpierre Feb 20, 2019

Contributor

also if you don't check the sign of rgb.x etc., you should use abs(rgb.x) etc.

This comment has been minimized.

@phweyland

phweyland Feb 20, 2019

Author Contributor

this is not a norm, but a mean. You should not divide by 3

I've done but doesn't work.
As an equivalent to luminance it cannot be equal to 3 ...
Here 3 is not for average, but the max of the 3 values: rgb.x max + rgb.t max + rgb.z max.
How else to keep this inside [0,1] ?
Same issue for the other cases.

This comment has been minimized.

@aurelienpierre

aurelienpierre Feb 21, 2019

Contributor

Actually, the tonecurve (unbounded) is able to deal with values > 1 because of the exponential extrapolation. The norm is supposed to be the length of the vector.

}
case 3: // norm L2 = euclidian norm - bypass the powf for performance
{
return sqrt((rgb.x * rgb.x + rgb.y * rgb.y + rgb.z * rgb.z) / 3);

This comment has been minimized.

@aurelienpierre

aurelienpierre Feb 20, 2019

Contributor

same here

}
case 4: // general Lp norm (pseudo-norm if p < 1) - slow variant
{
return pow((pow(rgb.x, norm_exp) + pow(rgb.y, norm_exp) + pow(rgb.z, norm_exp)) / 3, 1.0f/norm_exp);

This comment has been minimized.

@aurelienpierre

aurelienpierre Feb 20, 2019

Contributor

same here, and you really need abs(rgb) in case norm < 1. You could also use the native_powr function instead of pow (much faster).

R = 1.22f * rgb.x * 1.22f * rgb.x;
G = 1.20f * rgb.y * 1.20f * rgb.y;
B = 0.58f * rgb.z * 0.58f * rgb.z;
R *= R; G *= G; B *= B;

This comment has been minimized.

@aurelienpierre

aurelienpierre Feb 20, 2019

Contributor

what you are computing here is 1.22⁴ × R⁴. It looks wrong, are you sure about the formula ? I would expect 1.22 × R⁴.

If it is right, you could just do

const float coeff_R = pow(1.22f, 4);
const float coeff_G = … etc.
const float4 rgb_pow4 = rgb * rgb * rgb * rgb; // the multiplication on full vectors is faster (that's the point of OpenCL)
const float4 rga_pow5 = rgb_pow4 * rgb;

return 0.83743219f * (1.22f * coeff_R * rgb_pow5.x + 1.20f * coeff_G * rgb_pow5.y + 0.58 * coeff_B * rgb_pow5.z ) / (coeff_R * rgb_pow4.x + coeff_G * rgb_pow4.y + coeff_B * rgb_pow4.z);
float
rgb_norm_vect (float4 rgb, int rgb_norm, float norm_exp)
{
switch(rgb_norm)

This comment has been minimized.

@aurelienpierre

aurelienpierre Feb 20, 2019

Contributor

not sure about case 2 and case 3 since they are particular cases of case 4 (with values of norm_exp nice enough to avoid slow calls to pow). I would just merge them into case 4 and check the norm_exp to branch, or even completely ignore them in OpenCL since branches can make it super slow and it doesn't mind the load (so you just use the general case 4).

{
norm = rgb_norm_vect( rgb, rgb_norm, 0.333333f);
norm = lookup_unbounded(table_0, norm, coeffs_0) / norm;
rgb.xyz *= norm;

This comment has been minimized.

@aurelienpierre

aurelienpierre Feb 20, 2019

Contributor

you got that wrong :-)

norm = rgb_norm_vect( rgb, rgb_norm, 0.333333f); // compute the norm == luminance estimator
const float4 rgb_ratios = rgb /norm; // these ratios are the actual colors, independant from the luminance
norm = lookup_unbounded(table_0, norm, coeffs_0); // compute the curve on the luminance
rgb.xyz = (norm * rgb_ratios).xyz; // restore the colors from the original ratios and the new luminance
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment