Add new option to set a fixed size for brackets.

ff2000 commented 5 years ago

Bracketed sets will not get computed automatically. Sets get built in order of input files.

NOTE: This is the basic functionality. Probably you want a different character than "-i" for the option. Also this currently requires "-B/--batch", if it stays the help text needs to be updated. (IMO it's still batch processing, so it made sense for me). And IMO there is a check needed if the number of files is a multiple of the set size. Program should exit in that case IMO.

[option parsing in general could be safer: options that expect an integer currently increment argc and do not decrement it if the value is not an integer which will eat the next option. IMO in such cases the program should exit and not keep processing the files.]

Beep6581 commented 5 years ago

And IMO there is a check needed if the number of files is a multiple of the set size. Program should exit in that case IMO.

@ff2000 did you mean the program should exit if that is not the case?

ff2000 commented 5 years ago

@Beep6581 Yes, that's what I wanted to say. Or would you just put the last remaining photos in a new set? As merging RAWs takes some time I think it's better to just abort instead of creating bad HDRs.

Beep6581 commented 5 years ago

I agree. If there are remaining photos, that means there is a problem. I would definitely not put the last photos in a new set.

P.S. I wrote a Bash script which does exactly that and have been using it for years. It allows me to specify the number of images per set, then takes all input images and groups them accordingly. I originally wrote it for tufuse/enfuse, which were not capable of grouping images automatically based on metadata like HDRMerge can. It eliminates the need for relying on metadata, which sometimes can fail, for example when shooting quick brackets using my old Pentax K10D sometimes it was not possible to figure out which image belongs to which set based on metadata, as the creation time's precision was 1 second, and if you happened to shoot more than one set in that second, then you had a metadata problem. Using the script eliminated that problem.

ff2000 commented 5 years ago

Yes, that's true, but as mentioned here it requires scripting skills: https://discuss.pixls.us/t/feature-request-hdrmerge-batch/10032/5

Would you probably also like to have implemented some sort of batch-processing/editing in the gui? On import the user can select if the files are for one hdr, should be grouped automatically or by a fixed number for the set size.

I now abort (throw a logic_error). IMO should be done in the bad case for "-i", "-b", "-w", "-g", "-r", too.

Beep6581 commented 5 years ago

@ff2000 sorry for the delay, I haven't tested this yet but will do so the next time I process some shots.

@heckflosse @Floessie any objections code-wise?

Floessie commented 5 years ago

@ff2000 :+1:

Here I'd prefer a space after if and catch.

". use" --> ". Use"

Best, Flössie

fanckush commented 4 years ago

I’m not familiar with the files you changed Can someone explain to me what Is this PR is about? Is it ~~refreshing~~ to CLI batch mode by any chance?

Edit: is it referring to CLI batch mode by any chance?

ff2000 commented 4 years ago

hdrmerge groups the files to create an HDR by looking at the EXIF time. Works most of the time but sometimes fails. E.G. missing/bad EXIF data. Per user request on pixls forum I implemented an option where the user can just say "I bracketed my shots, each HDR should be created with 5 shots. Here are 15 RAW files. HDR - Merge!"

How should the batch mode be refreshed in your opinion?

Am So., 15. Sept. 2019 um 21:38 Uhr schrieb Imad abdulkarim < notifications@github.com>:

I’m not familiar with the files you changed Can someone explain to me what Is this PR is about? Is it refreshing to CLI batch mode by any chance?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jcelaya/hdrmerge/pull/168?email_source=notifications&email_token=AAPB3TG5JPA5MPR5SXPB3MTQJZ6L3A5CNFSM4GHMY572YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6XWPDY#issuecomment-531589007, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPB3TFLE67EQ7WUW53TELDQJZ6L3ANCNFSM4GHMY57Q .

fanckush commented 4 years ago

@ff2000 I made a an autocorrect typo in my previous comment. but it's all good now I read the changes again and I understand what this PR is about. I think what you did is the awesome and very handy 👍 ; usually bracketed sets have the same amount image per set.

@Beep6581, could you add this to the v0.7 milestone?

ff2000 commented 4 years ago

Before merging there IMO are some changes that should be considered, as mentioned in the OP of this PR:

Want a better option than "-i" - I've choosen this to get something working, "i" was obvious as a programmer ;)
Currently requires "-B" to be set. If this should remain the help text needs to be changed.
Furthermore don't throw an exception, qt doesn't like it (in gui applications). It throws out a lot of text and the actual error gets hard to find. With kde support libs installed it also launches KCrash, which is not needed and will confuse users...

fanckush commented 4 years ago

maybe the -i to -S and --bracket-size?
yes make -B optional , I'd simply add generalOptions.batch = true; here: https://github.com/jcelaya/hdrmerge/pull/168/commits/882c813d7b3d53ef2613550d57b78e3de2cfcd2e#diff-d77d2011812fe9fdf1e2aa611ef8b3fbR196 and be done with it :)

Regarding the expiations part, I'm not sure I lack the technical skills for that part, if you could solve it that would be great. v0.6 will be released soon and v0.7 will come after once we get some bugs fixed so adding this to v0.7 looks like the right thing to do in my opinion.

ff2000 commented 4 years ago

-S sounds great, should it be upper or lower case? If @Beep6581 @Floessie @heckflosse agree I will remove the dependency on -B. (As it is still batch-processing I made it neccessary to pass -B)

Concerning exceptions: I introduced a throw logic_error when "images per bracket" doesn't divide "all images passed". That will go again as it is not really user friendly.

Beep6581 commented 4 years ago

This is a good feature. However, the list of switches is becoming confusing, so we should take the opportunity to clean them up.

I am not a fan of mixing lowercase with uppercase switches.

HDRMerge master:

    -B|--batch    Batch mode: Input images are automatically grouped into bracketed sets,
                  by comparing the creation time. Implies -a if no output file name is given.
    -b BPS        Bits per sample, can be 16, 24 or 32.
    -g gap        Batch gap, maximum difference in seconds between two images of the same set.
    --single      Include single images in batch mode (the default is to skip them.)

HDRMerge ff2000:fixed_bracket_sets:

    -i NUM_IMAGES    Fixed number of images per bracket set. use together with -B.

I will refer the -i as "images per group", or IPG.

RawTherapee:

  -b<8|16|16f|32>  Specify bit depth per channel.

Both HDRMerge and RawTherapee use -b for bit-depth, so let's keep that.

"Batch mode" has become vague. Now there are two batch modes - one groups images based on a timestamp, the other based on a user-defined count of images per group. Or maybe even three batch modes, if you consider that the default behavior is to assume all input images belong to one group.

Q1: -B|--batch gets an image creation date. What is .timestamp, where does it come from? Is it based only on Exif? If so, which tag, DateTimeOriginal? https://github.com/jcelaya/hdrmerge/blob/master/src/ImageIO.cpp#L76 https://www.libraw.org/docs/API-datastruct-eng.html#libraw_imgother_t https://sno.phy.queensu.ca/~phil/exiftool/TagNames/EXIF.html

Q2: Furthermore, please confirm: when using the new IPG mode, are -g and --single ignored?

Assuming the answers to the two questions above are correct, instead of "batch mode" and -B|--batch and -i, which are quite vague now, how about we change that to:

--group <all|auto|manual>
            Determines how the input images are grouped.
    all     - Process all input images as a single group. Default. Short: --ga.
    auto    - Group images automatically based on their time of creation (Exif DateTimeOriginal). Short: --gt.
    manual  - Group images manually based on a number of images per group. Short: --gm.

--ipg <#>   Images per group.
            Default: 3.
            Only used when grouping manually.
            The total number of images passed to HDRMerge must be a multiple of this number.

--gap <#>   Maximum gap in seconds between chronologically adjacent images.
            If the gap is larger, a new group is crearted.
            Default: 3.
            Only used when grouping automatically or all.

--single    Include single images if specified.
            Default: not specified.
            Only used when grouping automatically.

Isn't that more clear?

ff2000 commented 4 years ago

@Beep6581 thanks for the thoughtful reply. I have nothing against changing command line interface. Do you think it's better to break it now or wait till the 1.0 release? 0.6 and 0.7 were planned as bugfix releases if I understood it correctly, so probably better keep things as they are ATM?

Concerning the naming: How would you like

--set-size,-s instead of --igp
--single,-1

to get memorizable long/short option names?

A related issue is hardening the command line parsing. I really would like to port it to boost::program_options as it makes things more robust and easier to use and extend. If that is OK we should think of compatible ways to specify cmdline args. (-g a vs. --ga).

I could do the boost porting (if that's desired) and change the CLI. How should I proceed?

Beep6581 commented 4 years ago

@ff2000 0.* implies "unstable/feature-incomplete" while 1.0 implies "stable/feature-complete", so we should definitely break things sooner rather than later, especially since my proposed changes might require changes in your branch.

I prefer --igp because it's unmistakably clear and anyone who uses the command-line will have had to have read the --help stuff anyway, while --set-size is ambiguous because the word "set" can be a noun or verb with many meanings.

ff2000 commented 4 years ago

What about --bracket-size (short: -s)? I just realized that we both used --igp instead of your proposed --ipg ;) If it's too long use the short version. Or wait for bash/zsh completions.

And would it be OK to depend on boost? It's already used for tests. Compare this https://www.boost.org/doc/libs/1_65_0/doc/html/program_options/tutorial.html with these https://github.com/jcelaya/hdrmerge/blob/master/src/Launcher.cpp#L156 https://github.com/jcelaya/hdrmerge/blob/master/src/Launcher.cpp#L237

Beep6581 commented 4 years ago

Why not.

Floessie commented 4 years ago

@ff2000 While I like the -s suggestion ("igp" is a TLA usually used for "integrated GPU", thus the typos), why pull in a dependency on boost, when Qt itself has infrastructure for command line parsing?

ff2000 commented 4 years ago

@Floessie thank you very much for the hint! I did a lot of Qt programming some time ago and back then this did not yet exist, so I used boost. I will use the Qt framework instead now.

ff2000 commented 4 years ago

Options are read through QCommandLineParser now. I'm currently transferring all the help text over. E.g. looks like this:

-    QCommandLineOption batchOpt({"B","batch"}, tr("batch"));
+    QCommandLineOption batchOpt({"B","batch"},
+            tr("Batch mode: Input images are automatically grouped into bracketed sets,") + " " +
+            tr("by comparing the creation time. Implies -a if no output file name is given.")
+            );
     parser.addOption(batchOpt);

-    QCommandLineOption bracketSizeOpt("s", tr("bracket-size"), "integer", "-");
+    QCommandLineOption bracketSizeOpt("s",
+            tr("Fixed number of images per bracket set. Use together with -B. "
+               "Creation time and --single option will be ignored."),
+            "integer", "-1");
     parser.addOption(bracketSizeOpt);

I wanted to preserve the old translations as far as possible. The bracketSizeOpt IMO is easier to maintain. If it is OK to break translations I would use multiline-strings for all help texts.

[I do this step-by-step to keep commits minimal and make possible issues I introduce easily bisectable. 1) add new option 2) move to QCommandLineParser 3) refactor the command line options as proposed by @Beep6581]

Beep6581 commented 4 years ago

Don't worry about preserving translations.

ff2000 commented 4 years ago

Porting to QCommandLineParser should be complete now. Next step: refactor batch cmdline options. As I have issues with power supply and internet ATM this might take some days. Feel free to start reviewing if you like!

ff2000 commented 4 years ago

Refactoring cmdlineargs done.

./hdrmerge --help
Usage: ./hdrmerge [options] [raw_files...]
Merges raw_files into an HDR DNG raw image.
If neither -o nor --group options are given, the GUI will be presented.
If similar options are specified, only the last one prevails.

Options:
  -o <file>                       Sets <file> as the output file name. If not
                                  set it falls back to
                                  '%id[-1]/%iF[0]-%in[-1].dng'.
                                  The following parameters are accepted, most
                                  useful in batch mode:
                                  - %if[n]: Replaced by the base file name of
                                  image n. Image file names are first sorted in
                                  lexicographical order. Besides, n = -1 is the
                                  last image, n = -2 is the previous to the last
                                  image, and so on.
                                  - %iF[n]: Replaced by the base file name of
                                  image n without the extension.
                                  - %id[n]: Replaced by the directory name of
                                  image n.
                                  - %in[n]: Replaced by the numerical suffix of
                                  image n, if it exists.For instance, in
                                  IMG_1234.CR2, the numerical suffix would be
                                  1234.
                                  - %%: Replaced by a single %.
  -m <file>                       Saves the mask to <file> as a PNG image.
                                  Besides the parameters accepted by -o, it also
                                  accepts:
                                  - %of: Replaced by the base file name of the
                                  output file.
                                  - %od: Replaced by the directory name of the
                                  output file.
  -l, --loglevel <verbose|debug>  The level of logging output you want to
                                  recieve.
  --no-align                      Do not auto-align source images.
  --no-crop                       Do not crop the output image to the optimum
                                  size.
  -G, --group <all|auto|manual>   Determines how the input images are grouped.
                                  - all, a: Process all input images as a single
                                  group. Default.
                                  - auto, t: Group images automatically based on
                                  their time of creation (Exif
                                  DateTimeOriginal).
                                  - manual, m: Group images manually based on a
                                  number of images per group.
  -s, --bracket-size <integer>    Fixed number of images per bracket set. Only
                                  used when grouping manually. Creation time and
                                  --single option will be ignored. The total
                                  number of images passed to HDRMerge must be a
                                  multiple of this number.Default: 3
  -g, --gap <double>              Maximum gap in seconds between
                                  chronologically adjacent images. If the gap is
                                  larger, a new group is crearted. Only used
                                  when grouping automatically or all.Default: 3.
  --single                        Include single images when specified. Only
                                  used when grouping automatically. Off by
                                  default.
  -h, --help                      Displays this help.
  -b <16|24|32>                   Bits per sample.
  -w <integer>                    Use custom white level.
  -r <integer>                    Mask blur radius, to soften transitions
                                  between images. Default is 3 pixels.
  -p <full|half|none>             Size of the preview. Default is none.

Arguments:
  files                           The input raw files

Summed up changes:

I dropped the "-a" option, -Ga does the same.
-v and -vv now are -lv (--loglevel=verbose) and -ld (--loglevel=debug). The old way was confusing as debug could be seen as something different than verbose, e.g. look at the command used in #192: luckily it was -v -vv ;)
Also hdrmerge doesn't do any work (besides printing an error + the help) now if errors are detected in the options/parameters. Falling back to defaults might not be what the user wants.

Questions:

Should I rename bracket-size to group-size?
I'm not entirely happy with the "integer" and "double" descriptions for values. "floating point" and "whole number" are better but IMO too long. Opinions/solutions welcome :)

fanckush commented 4 years ago

i personally think bracket-size is clearer. "group" may refer to the whole set of images but with "bracket" you can't misunderstand the meaning.

ff2000 commented 4 years ago

That's also my thought, but as we now have the "group" option I had to think about it. And now that you mention it: I think the [n] in -o also should be clarified: is it the n'th image of a bracket or of all images passed?

ff2000 commented 4 years ago

Thx @Floessie for the feedback. I will const-qualify as much as I can and fix the other things. Glad you like it!

ff2000 commented 4 years ago

Removed the GROUPING prefix. Also made two minor improvements.

One more thing could be to not use abbreviations to more comply to #188 (e.g. bitOpt -> bitOption, invalidParamHandler -> invalidParameterHandler).

ff2000 commented 4 years ago

@fanckush do you see any more abbreviations (besides tr)?

Anybody also gave it some real testruns on a set of RAW files? The tests I did succeeded but I may be doing just the right things ;)

ff2000 commented 4 years ago

Commits squashed. Each commit compiles and converts as expected - I hope I didn't miss something... At least the final state is where the review ended ;)

jcelaya / hdrmerge

Add new option to set a fixed size for brackets. #168