Parameterization of nona "--clip-exposure" option

[Iouri Ivliev]

For now nona has hardcoded dark and bright pixel levels for "--clip-exposure" option. IMHO, it could be helpful to make these levels are modifiable by the user. The patch in the attachment adds two command line options ("--dark-level" and "--bright-level") to set dark and bright pixel levels.

[tmodes (tmodes)]

Thanks for the patch. But I had to rework it:
* strtof is only supported by C++11, but we don't enforce C++11
* strtolvl has nothing to do with ImageRemapper and should therefore moved out of this file
* The variables names m_clipExpMaskUELvl and m_clipExpMaskOELvl (and other) are completely unreadable (UEL is lower level/cutoff, OEL is upper level; what is U and what is O -> please use english words).
* The input should by checked by the calling program and not passed deep into further function and then silently ignored if there are invalid inputs. There is no feedback for the user that something is wrong.
* The default values are used in several places in several different formats:
 m_clipExpMaskUELvl(1/255.0f), m_clipExpMaskOELvl(250/255.0f) vs.
static const std::string uelvlDef("0.00392156862745098"); static const std::string oelvlDef("0.980392156862745");
* We have already enough options. Don't introduce 2 more options for a "minor" feature.

Committed as changeset 7d1e8251d212.

[Iouri Ivliev (lp-0o6vs1)]

Thank you.
As I see, there are several copies of an "HugeBase::Nona::AdvancedOptions" object in many instances of other classes.
I made a minor changes in the "HuginBase::NonaFileOutputStitcher", "HuginBase::Nona::SingleImageRemapper", "HuginBase::Nona::FileRemapper" and "HuginBase::Nona::RemappedPanoImage" classes to hold a const references to a single instance of HuginBase::Nona::AdvancedOptions.
Please look at the attached patch.

[Iouri Ivliev (lp-0o6vs1)]

About changeset 1829ca5c3668.
Comparing a minimum of red, green, and blue values with the LowerLimit (and a maximum of them with a UpperLimit) it makes a trouble. For example, a bright blue sky - rgb(0,6, 0,85, 0,99). It normally exposed but will be cut (by UpperLimit criteria) because of the blue channel "too bright". Another case: a grass in the shade - rgb(0.05, 0.2, 0.001). The same result (by LowerLimit criteria) - exposure normal, but the blue channel "too dark."
As I understand, to solve this problem, LowerLimit should be compared against the maximum and UpperLimit against the minimum:
  if (maxVal < LowerLimit || minVal > UpperLimit)
Attached patch makes this default.
Also it contains quick'n'dirty hack to make (using environment variables LOWER_CUTOFF_VAL and UPPER_CUTOFF_VAL) this conditions tunable.

[tmodes (tmodes)]

> As I see, there are several copies of an
> "HugeBase::Nona::AdvancedOptions" object..

I agree that it would be nice to keep only a reference to one instance. But your implementation makes it more convoluted. You add a completely new class RemappedFileImage for this minor function and add no-op functions to existing classes. (e.g. exposureClipMaskEnabled and related). So now there are two classes RemappedPanoImage and RemappedFileImage with newly the same functionality.

> Comparing a minimum of red, green, and blue values ..
If only one channel has an over- or underflow you will get a color cast when doing exposure compensation. So the correction should be okay.

> Attached patch makes this default.
Have you tested your patch? There is at least one typo:
utils.c: +CutoffVal stringToCutoffVal(const std::string &str)
utils.h: +IMPEX CutoffVal strngToCutoffVal(const std::string &str);

You found another way to pass an option to nona. But this option is completely undocumented, so it is mostly unusable for the common user.

For each pixel you are now calling "switch (cv)..". This can have impact on performance.

And more important: what issue do you want solve which this new switch with so many options?

We have already many options. So adding a new option should add a substantial new function.

[Iouri Ivliev (lp-0o6vs1)]

> Have you tested your patch? There is at least one typo:
Yes of course. It works as expected.

> utils.c: +CutoffVal stringToCutoffVal(const std::string &str)
> utils.h: +IMPEX CutoffVal strngToCutoffVal(const std::string &str);
It's all right. The second line is a declaration and the first one is a definition. "IMPEX" macro be expanded (for Windows) into "__declspec(dllexport)" when building DLL or into "__declspec(dllimport)" when using function from DLL. This clauses are need only for declarations, not for definitions.

> You found another way to pass an option to nona. But this option is completely undocumented, so it is mostly unusable for the common user.
I do not quite understand what you mean by "completely undocumented", but as I wrote above it's a "quick'n'dirty hack". I don't know how to pass these parameters so that it was "The Right Thing". The simplest way is to use environment variables.

> For each pixel you are now calling "switch (cv)..". This can have impact on performance.
Yes you are right. This needs to be optimized.

> And more important: what issue do you want solve which this new switch with so many options?
As wrote above, I have an image with a bright blue sky (rgb(0,6, 0,85, 0,99)) and a grass in the shade (rgb(0.05, 0.2, 0.001)). Also this image contains deep shades (from rgb(0, 0, 0) to rgb(0.002, 0.002, 0.002) and overexposed highlights (from rgb(0,985, 0,985, 0,985) to rgb(1, 1, 1). It's a part of middle row in spherical panorama. So, its deep shades are normally exposed in the bottom row, and highlights in the top one. I want to cut shades and highlights, but keep the grass and a part of normally exposed sky. Of course, I can do it, drawing masks by hand, but I would like to automate it.

But it seems that I have chosen the wrong tool. Now I realized that I needed something else. Namely, the ability to automatically create vector masks by different criteria, so that later to edit them in the Hugins "Masks/Crop" tool, if necessary. Thanks a lot, you have helped me to understand what I want really. And I know how to do that. Sorry that wasting your time.

[tmodes (tmodes)]

I know what the IMPEX macros does (I have implemented it ;-). This was not the point. Slightly reformatted:
> utils.c: + CutoffVal stringToCutoffVal(const std::string &str)
> utils.h: +IMPEX CutoffVal strngToCutoffVal(const std::string &str);

stringToCutoffVal <> strngToCutoffVal

Okay, I got the point. Yes, we were talking from different points: --clip-exposure works *before* the photometric correction (to prevent color cast in correction). But you were referring to a post-photometric correction mask. Modifying the --clip-exposure switch would not help in your use case, because the photometric correction would modify the color values and you would get not the desired result.
Maybe we could change it: --clip-exposure becomes --clip-exposure-before-photometric and we add an additional switch --clip-exposure-after-photometric to work additional after the photometric correction and add additional switch to select the projector.

> Sorry that wasting your time.
You have not wasted my time. Thinking on it and discussing it can improve Hugin for all of us.

PS: If you can use enfuse in your workflow, there is already a similar switch in enfuse
--exposure-cutoff=LOWER-CUTOFF:UPPER-CUTOFF:LOWER-PROJECTOR:UPPER-PROJECTOR
So no need to implement it for yourself.