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

Speedup, code cleaning and bugfixes for 'locallab' branch #3292

Closed
heckflosse opened this issue May 18, 2016 · 51 comments

Comments

@heckflosse
Copy link
Collaborator

commented May 18, 2016

@Desmis
Hello Jacques,
I took a look at the 'noise' generator in 'locallab' branch and I really like it.
But there has been at least one bug in your implementation (u2 was never initialized...) and it was a bit slow too ;-)
Here is a patch which uses a combination of your method and this example code. It's a lot faster now by making use of OpenMp and sleef xsincosf/xlogf functions and also gives 'better' results for large noise values. But caution: I eliminated all the unused chroma noise stuff with the patch too.

Ingo

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2016

Hello Ingo

I just test your patch...I think it is better and faster. For me no problem to suppress "chroma noise"

I think you can commit this patch
Thank you again :)

Jacques

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 18, 2016

Hello Jacques,

I made further corrections and committed to locallab

Ingo

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2016

Hello Ingo

I look at your modifications. No problem :)
Thank you.

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 18, 2016

@Desmis
Hello Jacques,
can I also make some changes to 'Contrast_Local' or are you working at this currently?
There is a case of 'false sharing' I would like to solve to get another reasonable speedup.

Ingo

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2016

@heckflosse
Ingo

No problem you can :)

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 18, 2016

@Desmis
Jacques, here is a first patch with a brute force solution to fix the false sharing in ImProcFunctions::Contrast_Local. At my 8-core machine this results in a 8x speedup for that function. A cleaner solution follows later

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 19, 2016

@heckflosse

Hello Ingo

  1. How do you used a patch with GIt...I know how to do when the patch is on my computer "git apply toto.patch", but how to do when this patch is on GitHub ?? Here it is easy I change manually the code

  2. I will be little available until Monday

Jacques

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 19, 2016

Ingo
Ijust test "manualy"...all woks fine :)

@Floessie

This comment has been minimized.

Copy link
Collaborator

commented May 19, 2016

@Desmis

How do you used a patch with GIt...I know how to do when the patch is on my computer "git apply toto.patch", but how to do when this patch is on GitHub ?? Here it is easy I change manually the code

Hi Jacques,

there is a button saying "Raw" in the upper right corner of the patch. You can apply "Save link as..." in your browser on it, then you have the patch on your computer.

HTH,
Flössie

PS: In C++ you don't need to prepend struct variables or parameters with the struct keyword. They are equal to classes. So you can save some space by instead of writing this

void Contrast_Local(float pm, bool locL, struct local_contra lco, float hueref, float dhue, float lumaref, float av, struct local_params& lp, LabImage* original, LabImage* transformed, SHMap* shmap, int sx, int sy, int cx, int cy, int oW, int oH,  int fw, int fh,  LUTf & localcurve, bool locutili, int sk);

you can write this:

void Contrast_Local(float pm, bool locL, const local_contra& lco, float hueref, float dhue, float lumaref, float av, local_params& lp, LabImage* original, LabImage* transformed, SHMap* shmap, int sx, int sy, int cx, int cy, int oW, int oH,  int fw, int fh, LUTf& localcurve, bool locutili, int sk);

And when passing structs or classes, it is most often a good idea to pass them by const &. And if you don't plan to modify LabImage* original, maybe it is better and more readable to pass it as const LabImage* original. Just some remarks...

If you like, I could have a look at this when you and Ingo are done and clean up those things for you.

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 19, 2016

@Floessie

Ok thank you :)

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 19, 2016

Hello all
After these recent developments and improvements, what do we do with this branch ?

  1. nothing, we abandon any changes and is stopped permanently
  2. is maintained as now, between two waters, so not really integrated in RT
  3. we commit this branch to "master'...causing, perhaps, later to incompatibilities

if 3) I plan to use the principle for functions such wavelet (very difficult) or RGB (I think simple), or others (sharpening, etc.)
Jacques

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 19, 2016

@Desmis
Jacques, in my opinion there are some things which need to be solved before thinking about merging locallab into master.

  1. Clean the code and make all the locallab stuff a bit faster (I'm working on both currently)
  2. Find a good solution to provide more than one local area. For this I have to solve 1) first because each local area adds processing time.
  3. We should think about a way to apply the other tools (sharpening etc.) to local areas without duplicating their code and without bloating the gui.
    ...
@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 19, 2016

@heckflosse

Ingo
I am fully aware of 1) is what I mentioned in prior

But for 2) Hombre was working on that since 3 years...with 2 basics principles:
a) works with layers...which today is impossible without substantial modification of code and pipelines
b) suppress sliders and allow for each "control point" a GUI intuitive to build (very very big job)

I agree for 3) but how to do ?

I think we can bypass the obstacle, not by duplicating the same code, but by examining the process that could be implanted with a similar function to "locallab", eg "localrgb", "localsharpening", "local wavelet", in which some functions could have results similar to locallab...(contrast, luma, saturation,...)
But sure it is complex, difficult to maintain, and can leeds if we succeed with a) and b) to incompatibilties.

So in summary it is the proposal "2) is maintained as now, between two waters, so not really integrated in RT" !!
:)

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 19, 2016

@Desmis
Jacques
Currently I have no idea how to solve 2) and 3). I hope to get ideas while cleaning and speeding up the code ;-)

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 19, 2016

@Desmis
Hello Jacques
I just made another serious speedup for locallab. Shall I submit it as a patch file or push it to locallab branch. Latter would be easier for me :)

Ingo

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 20, 2016

@heckflosse

Hello Ingo

do as you wish, no problem for me :)
jacques

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 24, 2016

@Desmis @Floessie I'm still working on cleanup and speedup of locallab branch. Got interrupted by dcraw update and some minor fixes. I hope I can complete the task before the weekend.

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 25, 2016

@heckflosse
Ingo
Is that what you're going to propose, will improve the existing situation (more speed, stability, ...), or is it going to change the system (more than one control point, layers, etc.)

Jacques

@Floessie

This comment has been minimized.

Copy link
Collaborator

commented May 25, 2016

@heckflosse No problem, I'll be on a journey until Sunday, and my cleanups won't be crucial either. :)

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 25, 2016

Regardless of possible improvements (more U points, layers ...), there are at least two ways to treat the points of controls.

  1. That found in some software like Gimp with a lasso to delimit the zone,
  2. or the one we found in Nikon CNX2 with the famous U-points with only a circle.

This second "intelligent" system can only influence certain colors, or brightness, etc.

For me this system (U- points) is significantly greater than the first, and that's the one I tried to start implementing in RT...especially with the "scope" function and variable transitions.

I think working with several developpers must be able to advance the algorithm ... The track of wavelets seems promising :)

@mks9900

This comment has been minimized.

Copy link

commented May 25, 2016

Jacques, as a former user of Capture NX, I really have to say that their U-point technology was the killer feature that made me use it although the program itself was a pain to use otherwise. If a similar function could be implemented in RT, well to say the least it would be awesome! :-)
BR,
Johan

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 25, 2016

Jacques, currently I make speedups and clean the code. I also think I found one bug which I want to correct.
Locallab is already similar to U- points (though currently with the restriction to one point and with a gui, which has to be improved IMHO). It also lacks some features from U - points like displaying the mask...) but that should be doable too.
You will get my full support to enhance the locallab tool!

Ingo

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 25, 2016

Jacques, I got closer to the 'bug' I mentioned above. We have to change the current implementation of picking 'hue' and 'chroma' on the fly (while processing the image) to a 'pick that values immediately in gui'.
Example: Place a locallab control point in gui and reduce Chrominance to -100. Then zoom 100 % to this control point. Works fine! Now pan the preview to move the control point far out of the preview area. In this case the Chroma and Hue values can not be picked from preview area and lead to totally wrong results in preview.

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 26, 2016

@heckflosse

Ingo
OK and thank you for your first remark ("I make speeds up...You will get my full support to enhance the locallab tool!")

For the second, it is a curious bug (and real).
Yes there is this bug, for chroma, but also for lightness and contrast, but only in direct mode. If you select "inverse" or Blur and noise" all seems to works!!
I hope you work on this bug.
Thank you :)

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 26, 2016

@Desmis
Jacques, I found another bug in calculating chromaref

Assume you have 2 pixels with the (a,b) values
(-1,1)
(1,-1)
The current method calculates (I skip the division by 327.68f for simplicity)
avA = (-1 + 1) / 2 = 0
avB = (1 + -1) / 2 = 0
chromaref = sqrt(SQR(avB) + SQR(avA)) = 0

the correct value for chromaref in this case would be
(sqrt(1 + 1) + sqrt(1 + 1)) / 2 = sqrt(2)

or do I miss some thing?

Ingo

Addition: yes, I work on this bugs :)

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 26, 2016

@heckflosse

Hello Ingo
You are true!!

Thank you very much :)

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 26, 2016

@Desmis
Hello Jacques,
here is a preliminary patch you can test. It's a lot faster and the chromaref bug is fixed too. The other bug is still not fixed. I'm working on it and will also make further cleanups.

Ingo

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 27, 2016

@heckflosse

Hello Ingo
you did a lot of work to optimize and simplify :)

I found a bug

  • for the quarter right/top (with lp.lyT and lp.lx) the ellipse is not good and there is interaction with "mouse decay" in lp.ly
    I have look quickly but do not find...

Jacques

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 27, 2016

@heckflosse

Ingo
I think I have probably found the bug...
It is in "fillMaskArrays"...
If you replace by
for(int i = 0; i <= 2; i++) {
lx[i] = lp.lx;
}

for(int i = 4; i <= 8; i++) {
    lx[i] = lp.lxL;
}

for(int i = 0; i < 2; i++) {
    ly[i] = lp.ly;
}

for(int i = 2; i <= 4; i++) {
    ly[i] = lp.lyT;
}

for(int i = 6; i <= 8; i++) {
   ly[i] = lp.ly;

}

All seems to work :)

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 27, 2016

@Desmis
Jacques, I tried your modification but could not detect differences in output before/after. Do you have a RAW file and a pp3 file to reproduce?

Ingo

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 27, 2016

@heckflosse

Hello Ingo

Here one NEF and pp3 with original patch :)
http://jacques.desmis.perso.neuf.fr/RT/_ASC4209.NEF
http://jacques.desmis.perso.neuf.fr/RT/_ASC4209.NEF.pp3
You can see a "scale" in top/right
:)

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 27, 2016

@Desmis
Hello Jacques.

I must be blind. I processed your file before and after your modification and they are absolutely identical!

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 27, 2016

@heckflosse

For me the difference is hugh :)
I join the new patch
http://jacques.desmis.perso.neuf.fr/RT/local2.patch
:)

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 27, 2016

@Desmis
Thanks, now I can reproduce it. Though curious why I couldn't before.

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 27, 2016

@Desmis
Jacques, I will supply a patch with on optimized correction to the bug you detected (thanks for finding the bug!!!) at the weekend.

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2016

@heckflosse

Ingo
OK :)

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 28, 2016

@Desmis

Jacques, here is a new preliminary patch.
It's again faster and more readable than last one, but the important thing is that we now have to call just one function to get information about the zone and the transition factor (calcTransition).
This should make it easier to implement different selections in future.
I hope I didn't introduce any new bugs ;-)

Ingo

@heckflosse heckflosse changed the title Speedup and correction for 'Add Noise' in 'locallab' branch Speedup, code cleaning and bugfixes for 'locallab' branch May 28, 2016

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 29, 2016

@heckflosse

Hello Ingo
All seems to work well.
Good job :)
Jacques

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 29, 2016

@Desmis
Hello Jacques,
I want to change some things:

  1. Currently the accuracy of positioning is only 1/200 of image dimensions. If you work with large images (D800) this leads to big steps when changing position. I would like to set accuracy to 1/2000 instead.
  2. Currently the size of the center region is quite large and depends on image dimensions. It also does not correspond to the size of the center circle in gui which makes it very hard to guess which pixels are picked for calculation of average values. I would like to set a fixed center size of 36x36 pixels for calculation of averages and a circle radius of 18 in gui. Perhaps we can change this later to an user adjustable size.

What do you think?

Ingo

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 29, 2016

@heckflosse

Hello Ingo
I think your 2 propositions are good....And you can :)

jacques

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 29, 2016

@Desmis
Jacques, here is the patch which includes the changes. It's not backwards compatible to coordinates of former version but I think we don't need backwards compatibility currently.

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 29, 2016

@heckflosse

Ingo
I tested quickly...it is more "fine" :)

All seems to work well.
Good job :)

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 29, 2016

@Desmis Ok to push the current version to locallab branch?

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 29, 2016

@heckflosse

No problem , you can :)

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 29, 2016

Pushed

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 30, 2016

@Desmis
Hello Jacques,
I found a solution for this bug.
Do you prefer a patch or a push?

Ingo

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 30, 2016

@heckflosse
No problem, but a push is simpler :)
jacques

@heckflosse

This comment has been minimized.

Copy link
Collaborator Author

commented May 30, 2016

@Desmis I pushed

@Desmis

This comment has been minimized.

Copy link
Collaborator

commented May 31, 2016

@heckflosse

Hello Ingo
Now, in this case, it's working fine :)
Very good job !

Now, if no objections, I will try to implement "Local Retinex"...whith how many controls ? (1 , 2, ...more)
Jacques

@Beep6581

This comment has been minimized.

Copy link
Owner

commented Feb 8, 2017

All locallab issues:
#3546
#3421
#3351
#3292
#2538

@Beep6581

This comment has been minimized.

Copy link
Owner

commented Feb 8, 2017

Merged into #3421

@Beep6581 Beep6581 closed this Feb 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.