jjsarton commented 1 year ago

The arguments passed to backscrub are checked by the function strncmp(). strncmp() should be replaced with strcmp(). Wrong arguments are not always recognized and may produce erroneous messages.

For example:

´´´ backscrub --cg 960x640 -vg 640x480


```-vg`` will be recognized as ```-v``` and we will get the error message ```Failed to initialize vcam device.```

phlash commented 1 year ago

Agreed, we have entered the 'too many switches' world, and the tolerant parser I started with (allowing people to write full argument names such as -video) is now too lax.

jjsarton commented 1 year ago

On my testing code (not the cropping branch) I have replaced strncmp() with strcmp() so that the wrong arguments are detected. Furthermore, I check if the value argument begin with '-' (value forgotten). The "world"switch" may be realized so that -[-v][i[e[d[o]][-]g[e[o[m[e[t[r[y]]]]]]] may be recorgnized as correct. and -? | -[-]he[l|p]] may also be implemented, this is different from -h wich schall remain or be -[-]hei[g[h[t]]]

jjsarton commented 1 year ago

I have reworked the parsing. I have the functions:

static void usage(int exitcode, bool syntaxOnly, const char *message);
static void usage(int exitcode, bool syntaxOnly);
static void usage(int exitcode);
static char *message(const char *format, ...);
inline bool _hasValidArgument(int arg, int argc, char **argv) ;

and a macro:

#define hasValidArgument _hasValidArgument(arg, argc, argv)

This simplify error check and we have less code within the parsing loop

        } else if (strcmp(argv[arg], "-v") == 0) {
            if (hasValidArgument) {
                vcam = argv[++arg];
            }
...
        } else if (strcmp(argv[arg], "--cg") == 0) {
            if (hasValidArgument) {
                capGeo = geometryFromString(argv[++arg]);
                if (capGeo->first < 1 || capGeo->second < 1) {
                    usage(1, true, message("--cg wrong geometry %s\n", argv[arg]));
                }
            }
...

For parsing I check if no parameter was passed and is so call usage(0, false).

usage( exitcode, syntaxOnly, message) do the following:

print the syntax
if  ! syntayOnly print the parameter list with explanation
if  message provided print the message
if exitcode > -1 exit with exit code

hasValidArgument check first if a further parameter is passed and call usage() with an appropriate message then check if the next attribute begin with a '-', if so call usage().

The function message will build a string unsing va_list and vsnprintf.

I have notified, that inappropriate variable of type size_t are used for the parsed parser (geo, w, h, t...)

The variable ccam and vcam are initialized with /dev/video0 and /dev/video1! These values are, as far I have enough knowledge from V4l2 never be okay. I think that it is better to initialize with nullprt and look after the argument parsing is done if these required parameter was passed to basckscrub.

BenBE commented 1 year ago

Instead of

static void usage(int exitcode, bool syntaxOnly, const char *message);
static void usage(int exitcode, bool syntaxOnly);
static void usage(int exitcode);
static char *message(const char *format, ...);

use optional arguments and std::string:

static void usage(int exitcode, bool syntaxOnly = false, const std::optional<std::string>& message = {});
static std::string message(const std::string& format, ...);

That way you avoid the memory leak you introduce with allocating a string you pass around and never freeing it …

jjsarton commented 1 year ago

@BenBe Can you explain to me how I will allocate memory when I don't do this?

BenBE commented 1 year ago

There's no need to allocate memory, because that's what std::string does for you internally when you set a size.

jjsarton commented 1 year ago

@BenBE, warum kompliziert, wenn es einfach geht!

BenBE commented 1 year ago

@jjsarton Weil rohe Pointer in C++ schlechter Stil sind.

FWIW: Bitte weiter auf Englisch, da nicht jeder in der Gruppe hier Deutsch beherrscht.

jjsarton commented 1 year ago

@BenBE The German comment was only for you. Please look on the actual file deepseg.c at lines: 73, 246, 307, 368, 596. nullptr is used!

BenBE commented 1 year ago

@BenBE The German comment was only for you. Please look on the actual file deepseg.cc at lines: 73, 246, 307, 368, 596. nullptr is used!

73: Enforced by underlaying API (std::stoi), as we don't care about that value
246: Empty function pointer (should normally use std::function, but uses pointers due to C compat; library interface)
307: Forced by API (getenv returns nullptr if the variable does not exist)
368: Artifact from initial version; could be cleaned to std::optional<std::string> at some point; memory is not owned, thus no need to free
596: That's a managed std::shared_ptr

Or in short: Either forced by API, intentional due to constraints on exported API for C compat or an artifact, where memory is not owned anyway. And the last location is actually a std::shared_ptr, thus managed by RAII and not a plain pointer (even though nullptr is used as a literal.

Anyway, what's so bad about the proposed change of function signature for usage() and message()?

jjsarton commented 1 year ago

Within these functions I use only *argv[] and normal C strings. Need to use std::string, all should be converted to c++ string and again to c string. Within messages() I use a static local buffer with a size of 81 chars, if the message to be printed out, it will be shortened to 80 bytes. We use also printf(), this is not very C++ like. I would say use the best of both world. You may also provide me with a working example.

BenBE commented 1 year ago

Passing pointers to static buffers is also kinda evil …

jjsarton commented 1 year ago

proposed changes for deepseg.cc

Options

All options begin with a single "-" ffmpeg. Old options (--vg, --cg) work also

Added the option -verbose, -version. -max-fps as well -dd which is the same as -d -d

-version

The version print out the same as the first printf which are at the begin of the main function.

A little change: /usr/local/backscrub is replaced with a pointer to backscrub.

The opencv version shall also be printed out.

-max-fps

While using the stream of an hdmi grabber attached to an usb port, we may have high fps (50 or 60) this consume process time and we normally don't need this if we use webRTC.

-max-fps 30 will give 25 or 30 fps according to the grabber fps 50 or 60.

stderr, stdout

Only errors shall be printed out with stderr.

Warning eg. geometry aspect ratio are printed out on stdout

No option or 0-? -help --help -version output is stdout

Missuses of size_t

size_t is for some variables not useful and annoying. This is for example the case for the geometry options. The concerned size_t are replaced with int.

Timing output in debug mode

Timing output while debug is greater than 1 is annoying. We need more informations but not the timing information, we will check visually what we get or read the informations printed out if debug level is greater than 1.

Crash with debug level 2

There are two reason for crash:

1) --vg ist for example set to 150x150, the picture or video is to big! 2) Pressing 'q' terminate backscrub while imshow() do anythings. The simplest way to eliminate this is raise(SIGSTOP).

Development file

I have ability to resize first the stream from the camera (test option -rf). With this I have made test and if We resize first and do processing we may need up to 4 time less CPU time.

The input size was 1920x1080 and the output size 640x360. We may have such input size if we use a HDMI grabber.

jjsarton commented 1 year ago

Passing pointers to static buffers is also kinda evil …

If you don't know what you do.

static char *message(const char *format, ...) {
    static char buf[81];
    std::va_list args;
    va_start(args, format); 
    vsnprintf(buf, sizeof(buf), format, args);
    va_end(args);
    return buf;
}

This is okay, buf is static within the function. message() don't need to be reentrant, I use this function only one time and usage() is called and backscrub will always terminate.

BenBE commented 1 year ago

proposed changes for deepseg.cc

Options

All options begin with a single "-" ffmpeg. Old options (--vg, --cg) work also

Objection: All long options require double dashes (--), short options use single dash (-).

Added the option -verbose, -version. -max-fps as well -dd which is the same as -d -d

Should thus be --verbose, --version, --max-fps. -dd is the same as -d -d.

(getopt behaviour)

-version

The version print out the same as the first printf which are at the begin of the main function.

A little change: /usr/local/backscrub is replaced with a pointer to backscrub.

/usr/local/backscrub should be /usr/local/bin/backscrub according to FHS.

The opencv version shall also be printed out.

ACK.

-max-fps

While using the stream of an hdmi grabber attached to an usb port, we may have high fps (50 or 60) this consume process time and we normally don't need this if we use webRTC.

-max-fps 30 will give 25 or 30 fps according to the grabber fps 50 or 60.

ACK.

stderr, stdout

Only errors shall be printed out with stderr.

Warning eg. geometry aspect ratio are printed out on stdout

No option or 0-? -help --help -version output is stdout

ACK.

Missuses of size_t

size_t is for some variables not useful and annoying. This is for example the case for the geometry options. The concerned size_t are replaced with int.

If quantities are counted, size_t should be preferred.

Timing output in debug mode

Timing output while debug is greater than 1 is annoying. We need more informations but not the timing information, we will check visually what we get or read the informations printed out if debug level is greater than 1.

Maybe split timing to an explicit --debug-timing option?

Crash with debug level 2

There are two reason for crash:

--vg ist for example set to 150x150, the picture or video is to big!

Pressing 'q' terminate backscrub while imshow() do anythings. The simplest way to eliminate this is raise(SIGSTOP).

Both situations should be handled gracefully without a hard crash. Implement fixes as necessary.

This should be a separate PR from this one.

Development file

I have ability to resize first the stream from the camera (test option -rf). With this I have made test and if We resize first and do processing we may need up to 4 time less CPU time.

Sounds interesting, separate PR.

The input size was 1920x1080 and the output size 640x360. We may have such input size if we use a HDMI grabber.

Overall:

long options with double dash, short options one letter only with single dash.
Bug fixes to main as applicable
New/change behaviour to experimental.

jjsarton commented 1 year ago

proposed changes for deepseg.cc

command line options

Should thus be --verbose, --version, --max-fps. -dd is the same as -d -d. ( getopt behaviour)

--vg and --ch shall be --video-geometry and --camera-geometry (of course, both “deprecated” options will remain).

You are right, this like getopt.

-version

/usr/local/backscrub should be /usr/local/bin/backscrub according to FHS.

What is FHS ?

Objection:

Unix and Linux programs don't report the full path, only the binary name.

If I install this on /opt or /usr/local or within my home directory is not relevant and for the later case worse.

Missuses of size_t

If quantities are counted, size_t should be preferred.

I think., that this is already done. I have only replaced the important wrong uses,

Timing output in debug mode

Maybe split timing to an explicit --debug-timing option?

For testing, I had an option --no-timing and deleted it. I can insert the --debug-timing option (a bool debugTiming = true) for respecting the old behaviour or initialized to false.

Crash with debug level 2

Both situations should be handled gracefully without a hard crash. Implement fixes as necessary.

This should be a separate PR from this one.

May be, but the functions are already implemented within my development file.

Development file

Sounds interesting, separate PR.

Why more work as needed?

Testing

I have performed a lot of tests, I think, that involved people can also perform a check and then if there are no major problems, we can continue with a PR and fix possibly typo and so on.

BenBE commented 1 year ago

FH = Filesystem Hierarchy Standard

Re Full Name: If you report the path, it's not just /usr/local/; if only the basename is intended, only the basename should be shown.

Re Split PRs: BEcause 1 PR = 1 Set of related Changes. Makes reviewing stuff much easier and less error-prone. Also allows for better testing and forces to separate changes from each other.

jjsarton commented 1 year ago

Re Full Name: If you report the path, it's not just /usr/local/; if only the basename is intended, only the basename should be shown.

Only the base name is of interest and not the full path! Above I have mean /usr/local/bin, it was a typo, but I wrote the basename as output, so this was to understand. Much easier, I am not sure! More Work, since some modifications are trivial. The major part is parsing. The total part changes as per diff concern only 17 blocks including typo corrections from old code! I have tested the code, and it works better as the old code!

jjsarton commented 1 year ago

Test version

The development code I have is not checked in and is only for my own usage.

Modifications—Improvements

With the introduction of the --cg and --vg (capture and virtual Geometry), it is possible to have different aspect ratio for the camera and virtual devices. With this version, the Pixel Aspect Ratio is not always respected. This code contain the necessary code for avoiding such effect.

OpenCV can use different backends, under Linux usually ffmpeg and or gstreamer. Importunately, some Linux don't include the ffmpeg support in their openCV package. We shall take, as far as possible, account to gstreamer. A few fixes are included.

Using an HDMI grabber as camera input may result on stream with a height resolution (1920x1080) at a height frame rate. Processing of the data may need a lot of CPU time. For this case, we have the ability to set the maximal frame rate we shall process

    fps = (int)cap.get(cv::CAP_PROP_FPS);
    fpsDivisor = 1;
    if (maxFps > 0) {
        fpsDivisor = ((fps+maxFps-1) / maxFps);
    }
    ...
    int skip = fpsDivisor;
    while(true) {
        // grab new frame from cam
        cap.grab();
        ...
        cap.retrieve(raw);
        ...
        if (raw.rows == 0 || raw.cols == 0) continue; // sanity check
        if (skip < fpsDivisor) {
            skip++;
            continue;
        } else {
            skip = 1;
        }
        ...

Crash while using the preview debug window

There are two reason for crashes.

The first concern the size of the video and mask miniature which can be displayed within the preview window. They have a fixed width and therefore if the virtual video device size is to small, backscrub will crash. In order to resolve this is easy, check if the miniature fit into the displayed and allow, don't allow this.

        // background as pic-in-pic
        if (showBackground && pbk) {
            cv::Mat thumb;
            grab_thumbnail(pbk, thumb);
            if (!thumb.empty()) {
                int h = thumb.rows*160/thumb.cols;
                if ((h < raw.rows*3/4 || thumb.cols < raw.cols/2) && h > 50) {
                    cv::Rect cr = bs_calc_cropping(thumb.cols, thumb.rows, 160, h);
                    thumb(cr).copyTo(thumb);
                    cv::Rect r = cv::Rect(0, 0, thumb.cols, thumb.rows);
                    cv::Mat tri = test(r);
                    thumb.copyTo(tri);
                    cv::rectangle(test, r, cv::Scalar(255,255,255));
                }
            }
        }
        // mask as pic-in-pic
        if (showMask) {
            if (!mask.empty()) {
                cv::Mat smask, cmask;
                int mheight = mask.rows*160/mask.cols;
                if ( mheight < raw.rows*3/4 || mask.cols < raw.cols/2) {
                    cv::resize(mask, smask, cv::Size(160, mheight));
                    cv::cvtColor(smask, cmask, cv::COLOR_GRAY2BGR);
                    cv::Rect r = cv::Rect(raw.cols-160, 0, 160,mheight);
                    cv::Mat mri = test(r);
                    cmask.copyTo(mri);
                    cv::rectangle(test, r, cv::Scalar(255,255,255));
                    cv::putText(test, "Mask", cv::Point(raw.cols-155,115), cv::FONT_HERSHEY_PLAIN, 1.0, cv::Scalar(0,255,255));
                }
            }
        }
        cv::imshow(DEBUG_WIN_NAME, test);

The second crash occurs if we press the key q too soon. For such a case, openCV will process the data passed by the previous code, and this is not handled if we terminate immediately. The solution is also quite easy, the openCV error handler make the job we want. Backscrub will gracefully.

        cv::imshow(DEBUG_WIN_NAME, test);

        auto keyPress = cv::waitKey(1);
        switch(keyPress) {
            case 'q':
                std::raise(SIGINT);
                break;

Command line option parsing

One of the bigger error is mostly in front of the computer, so it is necessary to check the given options and values before really start the job.

The old code was not ideal and the output too expansive. Printing out the program information was removed from the main() function and replaced by a few functions.

While we are debugging we get, for each processed frame timing information, this is very nice if we will check the performance and the CPU time we need for processing. This is annoying ff we want only the debug output. A new option --debug-timing allows to output the timing if wanted.

Established options

Type	Option	Comment
Mnemonic	-h	(help) also a flag
Verbose	`--`help	the same as above. This is a long option
Mnemonic	-v value
Verbose	`--`value-required=value	Note the =
Mnemonic	-d	(debug) level set to 1
Mnemonic	-d -d	(debug) level de
Mnemonic	-dd	(debug) level set to 2 (not --dd)

backscrub strange options

Type	Option	Comment
Mnemonic	-?	(help) Windows/Unix mismatch
Unknown	--cg value	verbose (long option) or mnemonic?

The words mnemonic and verbose reflect better what is mean. The single letter options are mnemonic, the long options are verbose.

If we consider --cg as long (verbose) option the syntax should be, for example --ch=640x360.

I think that we shall have more letter mnemonic options.

Syntax message printed out by backscrub

 backscrub [-?] [-d] [-p] [-c <capture>] [-v <virtual>] [--cg <width>x<height>]

This is wrong, this shall be:

backscrub [-?] [-d] [-p] [-c capture] [-v virtual] ...

The only problem is with --cg, --vg. This may be written as [--cg WIDTHxHEIGHT] which is more readable as [--cg widthxheight]

Printing out version information

static void printVersion(const char *name, FILE *out) {
    fprintf(out, "%s version %s (Tensorflow: build %s, run-time %s)", name, _STR(DEEPSEG_VERSION), _STR(TF_VERSION), bs_tensorflow_version());
    fprintf(out, " (OpenCV: version %s)\n", CV_VERSION);
    fprintf(out, "(c) 2021 by floe@butterbrot.org & contributors\n");
    fprintf(out, "https://github.com/floe/backscrub\n");
}

This print out the same as the old code and the version of the used OpenCV.

Check for required parameters

inline bool _hasValidArgument(const char *name, int arg, int argc, char **argv) {
    if (arg+1 >= argc) {
        usage(name, 1, true, message("Option %s require a value", argv[arg]));
    }
    if ( *argv[arg+1] == '-') {
        usage(name, 1, true, message("Option %s require a value found: %s", argv[arg],argv[arg+1]));
    }
    return true;
}

#define hasValidArgument _hasValidArgument(name, arg, argc, argv)

If an Option require a value, there must be at least one more argument. If this is not the case, we call the function usage() with a description of the user mistake.

If the value found begin with a - (we don't allow options beginning with this). We also call usage()

Formatted Messages

static char *message(const char *format, ...) {
    static char buf[81];
    std::va_list args;
    va_start(args, format); 
    vsnprintf(buf, sizeof(buf), format, args);
    va_end(args);
    return buf;
}

This is a helper function which allows us to pass formatted messages to our usage() function.

Silent Start

This can't be done with the following, we may eliminate the output. For the tensor data, error messages will also been deleted from output. This can't be build in!

    // Dirty 1 !!!
    if ( !verbose && !debug) {
        // redirect stderr to /dev/null and preserve STDOUT
        // we will set stderr later to STDOUT!
        freopen("/dev/null","w",stderr);
    }
    CalcMask ai(*s_model, threads, aiw, aih);
    // Dirty 2 !!!
    if ( !verbose && !debug) {
        fclose(stderr);
        fdopen(2,"w");
    }

floe / backscrub

Argument parsing #155

proposed changes for deepseg.cc

Options

-version

-max-fps

stderr, stdout

Missuses of size_t

Timing output in debug mode

Crash with debug level 2

Development file

proposed changes for deepseg.cc

Options

-version

-max-fps

stderr, stdout

Missuses of size_t

Timing output in debug mode

Crash with debug level 2

Development file

proposed changes for deepseg.cc

command line options

-version

Missuses of size_t

Timing output in debug mode

Crash with debug level 2

Development file

Testing

Test version

Modifications—Improvements

Crash while using the preview debug window

Command line option parsing

Printing out version information

Check for required parameters

Formatted Messages

Silent Start