added cropping of capture/background #153

jjsarton · 2022-09-01T11:04:49Z

Added cropping

Fixes #149

BenBE

Couldn't test things yet, but those are what I noticed while skimming over.

app/background.cc

app/deepseg.cc

lib/libbackscrub.cc

app/deepseg.cc

jjsarton · 2022-09-01T11:56:24Z

I will review my code and hopefully fix the little issues.

BenBE

capGeo.value().first/capGeo.value().second can be shortened to capGeo->first/capGeo->second

app/deepseg.cc

BenBE · 2022-09-02T08:44:31Z

app/background.cc

        frm = pbkd->frame;
    } else {
        // resize still image as requested into out
-        cv::resize(pbkd->raw, out, cv::Size(width, height));
+        cv::Rect_<int> crop = calcCropping(pbkd->raw.cols,pbkd->raw.rows,width,height);


Suggested change

cv::Rect_<int> crop = calcCropping(pbkd->raw.cols,pbkd->raw.rows,width,height);

cv::Rect crop = calcCropping(pbkd->raw.cols, pbkd->raw.rows, width, height);

I have updates my cropping branch accordingly. Your suggestion is within my code

app/background.cc

app/deepseg.cc

jjsarton · 2022-09-02T19:02:54Z

On my fork jjsarton/backscrub the origin branch contain the latest code including your suggestions or the revised code.

I thing that your suggestion concerning line 680 to 687 (grab_background) may not be changed we pass 2 variables and don't need to use a class in order to put them the variables into a class and extracting them later!

app/deepseg.cc

jjsarton · 2022-09-03T11:24:52Z

Github is very slow!
I had committed the suggested changes and compared the own source with my branch. All modifications what not take into account!
I have pushed my own file to github, looked again on the push request and seen that some suggestion (which what no more shown while I was within the commit process) were already present. Sorry!

BenBE · 2022-09-03T12:21:25Z

Looks already good so far. Care to squash/fixup those commits into one (or few) …

jjsarton · 2022-09-03T12:26:10Z

I have to learn how to deal with GitHub! I hope that I will learn this.

jjsarton · 2022-09-04T07:24:50Z

@BenBE
I have done more checks and had to correct a little bug within app/background.cc (line 195). The fix is committed within my cropping branch.

app/background.cc

phlash · 2022-09-10T11:58:33Z

app/background.cc

@@ -183,11 +185,17 @@ int grab_background(std::shared_ptr<background_t> pbkd, int width, int height, c
    if (pbkd->video) {
        // grab frame & frame no. under mutex
        std::unique_lock<std::mutex> hold(pbkd->rawmux);
-        cv::resize(pbkd->raw, out, cv::Size(width, height));
+        cv::Rect crop = calcCropping(pbkd->raw.cols, pbkd->raw.rows, width, height);


We could lift this calculation out of the processing loop to a structure variable, as sizes will not change during operation.

Size will not change, the frame content will change. Mofifying this implies:
if ! rect_calculated
crop = calc_cropping()
else
crop = get_stored_crop_value()
...
I don't think that the calculation will take a lot ot time more than filling the cv::Rect from already calculated variables.

phlash · 2022-09-10T11:59:06Z

app/background.cc

        frm = pbkd->frame;
    } else {
        // resize still image as requested into out
-        cv::resize(pbkd->raw, out, cv::Size(width, height));
+        cv::Rect crop = calcCropping(pbkd->raw.cols, pbkd->raw.rows, width, height);


..as above, could be removed in favour of a state variable, calculated just once.

I must think about that.

I must review this part.

I have put the variable bg_stored into the structure background_t.
The code is now:

} else { if (!pbkd->bg_stored) { // resize still image as requested into out cv::Rect crop = bs_calc_cropping(pbkd->raw.cols, pbkd->raw.rows, width, height); // Under some circumstances we must do the job in two steps! // Otherwise this resize(pbkd->raw(crop), pbkd->raw, ...) may fail. pbkd->raw(crop).copyTo(pbkd->raw); cv::resize(pbkd->raw, pbkd->raw, cv::Size(width, height)); pbkd->bg_stored = true; } out = pbkd->raw; frm = 1; } return frm;

For the video processing the code remain to the old code, we will probably every time read a new picture from the video source.

Please note that the changes are not effective now, the tests for video or still image don't work as required.
I propose to detect according to the value cnt which has the value -1 for still image and a greater value for video.
I have also changed the corresponding code.

Don't worry too much about this, it's a minor performance enhancement to avoid a couple of floating point ops per frame, when we are running a whole inference model too.. I'd rather the code was readable than maximum speed 😄

jjsarton · 2022-09-10T16:48:12Z

I have pushed the modified code to my cropping branch.

phlash · 2022-09-11T13:58:10Z

app/background.cc

@@ -18,6 +20,7 @@ struct background_t {
    int frame;
    double fps;
    cv::Mat raw;
+    int bg_stored;


bool perhaps as it's used only for true/false?

You are right, I have modified this. Initializing was done with false, setting with true.

phlash · 2022-09-11T14:00:14Z

app/background.cc

@@ -143,7 +147,7 @@ std::shared_ptr<background_t> load_background(const std::string& path, int debug
        //  if: can read 2 video frames => it's a video
        //  else: is loaded as an image => it's an image
        //  else: it's not usable.
-        if (pbkd->cap.read(pbkd->raw) && pbkd->cap.read(pbkd->raw)) {
+        if (cnt > -1) {


If I recall, I chose this current method as cnt could be > 1 for some image files (multiple resolutions?) but they would not play as a video... please test with all the variations in backgrounds folder 😄

I will perform the tests. Using fps which shall be greater than 0.00000 for a video may be better.

Unfortunately, my system don't work with animated.gif. Due to this I can't check if the fps which would be processed on systems allowing this will return a frame rate of 45 Fps (value obtained while converting the file via ffmpeg to a stream.

Jpg file may contain a thumbnail image, do reading twice work, and the file is recognized as video. Reading 3 times will give the expected result, but this shall not be the right way.

The test for picture/video is now based on the fps. I think that this shall always work.

I'll grab your PR and check what I find here in the next day or so - thanks.

OK, I've checked the behaviour as the current PR has it (fps > 0), and it detects all the backgrounds as videos, but does not fail, since the video loop logic resets the position on each request... we may be able to simplify all this to assume video at all times?

I am surprised!
The backsrcrub binary for the cropping branch work as expected, a video is recognized as video and pictures as pictures.

For total_landscaping.jpg I get as output:

background properties: vid: no fcc: 00000000 () fps: 0.000000 cnt: -1

What tell your system?

I use Fedora 36 XFCE with opencv 4.5.5. What do you use?
May be that we check for fps > 0.001 in order to have the right condition.

I have tried the following on my development environment:

int w1,w2=0; if(pbkd->cap.read(pbkd->raw)) { w1=pbkd->raw.cols; if (pbkd->cap.read(pbkd->raw)) { w2=pbkd->raw.cols; } } if(w2 == w1) {

This is basically the same as the old condition, but I assume that the width of 2 consecutive images has to be the same.
The final test will pass if we have a video, fail if we have an image, even width thumbnails (must have a lower size).

The detection was okay on my system.

According to opencv cap.get(cv::CAP_PROP_FRAME_COUNT) return the 'Number of frames in the video file'
The first intention to use the count as flag for video or still image, what okay. Asking for CAP_PROP_FPS shall also work.

Work with images as if they were a video is not a great idea. If the image has a big size, reading, ... will take more time as
Retrieving the thumb as now realized within background.cc (Your suggestion).

@phlash If I recall, I chose this current method as cnt could be > 1 for some image files (multiple resolutions?) but they would not play as a video... please test with all the variations in backgrounds folder smile
This was your statement, this should not be true if I see the definition for CAP_PROP_FRAME_COUNT.
For my image, which is recognized as video (old code) I get -1 as value. Am image is not considered as frame.

@jjsarton I'm running on Debian stable (11), OpenCV 4.5.1, here's what I get for each background type:

animated.gif => vid: yes, fps: 45.0.., cnt: 36

background_bauhaus.png => vid: yes, fps: 25.0.., cnt: -2147483648

rotating_earth.webm => vid: yes, fps: 30.0.., cnt: 916

total_landscaping.jpg => vid: yes, fps: 25.0.., cnt: 1

..so this looks like we are heavily dependant on unstable OpenCV behaviour 😞, and the reason I chose to ignore both fps and cnt, instead attempting to load two frames in sequence.

app/background.cc

phlash · 2022-09-11T14:08:09Z

Thanks for your patience @jjsarton, this is looking good. You might have missed a comment I put on the issue: #149 (comment), which refers back to @BenBE asking for the cropping behaviour to be controlled by a switch, as it's opinionated behaviour that others may not want.

jjsarton · 2022-09-11T18:09:38Z

Pushed the actual revised version.

BenBE · 2022-09-11T19:12:39Z

Could you squash all the fixup and correction commits into their respective commit of where they should have gone in the first place? You can/should keep commits that introduce/change behavior though. If I'm not too mistaken you should end up at around 5-ish commits. Makes the whole commit history much more reasonable.

FWIW: Each (intermediate) commit should compile on its own.

jjsarton · 2022-09-13T11:24:56Z

Could you squash all the fixup and correction commits into their respective commit of where they should have gone in the first place? You can/should keep commits that introduce/change behavior though. If I'm not too mistaken you should end up at around 5-ish commits. Makes the whole commit history much more reasonable.

FWIW: Each (intermediate) commit should compile on its own.

I will not do this (not meaning full). My cropping directory respect the suggestion done unless there was not okay.
The only point we have to discuss (including the comments) is the recognizing of video or still image.
I can't get animated.gif working (GIF is a deprecated format) and can't test if we should get the expected 45 fps on a system which allows to process gif file with backscrub.

BenBE · 2022-09-14T07:11:16Z

Could you squash all the fixup and correction commits into their respective commit of where they should have gone in the first place? You can/should keep commits that introduce/change behavior though. If I'm not too mistaken you should end up at around 5-ish commits. Makes the whole commit history much more reasonable.
FWIW: Each (intermediate) commit should compile on its own.

I will not do this (not meaning full). My cropping directory respect the suggestion done unless there was not okay. The only point we have to discuss (including the comments) is the recognizing of video or still image. I can't get animated.gif working (GIF is a deprecated format) and can't test if we should get the expected 45 fps on a system which allows to process gif file with backscrub.

Unfortunately I'm not sure what you were trying to say and how that related to the comment of mine that you quoted.

jjsarton · 2022-09-18T07:41:37Z

I have configured and installed my own opencv version. With this version ffmpeg work and unfortunately, still picture are recognized as video. In order to solve this I modified (on my test version) background.cc, load_background).
After initializing pbkd I have inserted:

 size_t imgages = imcount(path, cv::IMREAD_ANYCOLOR);

The test video or still picture is now

if (!images) {

I have tested this against the original installed opencv with don*t support ffmpeg and my own compiled opencv. For both the video detection worked.

I propose that I implement this within my cropping branch. We can also delete the comments concerning the detection or add some if you tell me what I should insert.

phlash · 2022-09-19T11:37:32Z

size_t images = cv:imcount(path, cv:IMREAD_ANYCOLOR)

I see a couple of issues with this approach:

imcount isn't available in OCV 4.5.1 (current on Debian stable), it appears in 4.5.3+
the implementation (https://github.com/opencv/opencv/blob/4.5.3/modules/imgcodecs/src/loadsave.cpp#L661) reads the whole input, assuming a file, which may hang for video streams or other URL backgrounds..

My existing implementation is a reduced form of this function, reading two frames to detect a video source. May I suggest we leave the existing mechanism alone, to get your work here merged?

jjsarton · 2022-09-19T15:54:10Z

Improvements:

Detect kind of background file

The detection video or still image ist done with:

the
number of images returned by cv::imcount() which is always
0 for video and gif file
the mumber of frames for the possible video file

We have the following cases:

File type	Frame count	Image count
Video	> 1	0
Animated GIF	> 1	0
image	< 0	> 0
Static GIF	1	0
Not usable	< 1	0

Opencv with ffmpeg

No further changes needed

Opencv with gstreamer

Gstreamer don't respect the loop indicator, without the check if
we have read the last frame, the stream will be terminated.

Within the read thread the frame number is checked against the
number of frame for the "video". If we have read all frames the
stream position is set to 0. The result is not checked, if we read
past the last frame, the stream is closed and the corresponding
check for read error is always present.

Not fixed behaviour

Gstream may return a wrong frame rate for some GIF files.
Gstream don't accept some files wich work well with ffmpeg.

This must not been fixed, the user shall use a correct file.

Tests

Test were performed with various more files as such delivered with
backscsrub.
One version was tested with the opencv package provided for fedora 36:

Opencv version: 4.5.5
Video I/O:
    DC1394:                      YES (2.2.6)
    GStreamer:                   YES (1.20.0)
    v4l/v4l2:                    YES (linux/videodev2.h)
    Intel Media SDK:             YES (/usr/lib64/libmfx.so -lstdc++ /usr/lib64/libdl.a /lib64/libva.so /lib64/libva-drm.so)

The other version was an own compiled openvc:

Opencv version: 4.6.0-dev
Video I/O:
    DC1394:                      NO
    FFMPEG:                      YES
      avcodec:                   YES (59.18.100)
      avformat:                  YES (59.16.100)
      avutil:                    YES (57.17.100)
      swscale:                   YES (6.4.100)
      avresample:                NO
    GStreamer:                   YES (1.20.3)
    v4l/v4l2:                    YES (linux/videodev2.h)

Attached the result of git status against the last commit.
background.cc.txt

jjsarton · 2022-09-19T18:16:28Z

I think that my code can remain how it is with the following exceptions:
instead of reading the number of images with imcount(), images is set to 0.
at the proper place I will set images to 1 if cnt < 0 (before video detection)
I have made a test on both backscrub version with all my backgrounds and had no error.

jjsarton · 2022-09-20T08:40:33Z

We may conditionaly include cv::imcount().

If we have this after the #include lines, we have knowledge about the used opencv version.

#if CV_VERSION_MAJOR < 4 ||\
     CV_VERSION_MAJOR == 4 && CV_VERSION_MINOR < 5 ||\
     CV_VERSION_MAJOR == 4 && CV_VERSION_MINOR == 5 && CV_VERSION_REVISION < 3
# define HAVE_IMCOUNT 0
#else
# define HAVE_IMCOUNT 1
#endif

The modified code will be:

#if HAVE_IMCOUNT
        size_t images = cv::imcount(path.c_str());
#else
        size_t images = 0;
#endif
        pbkd->cap.open(path, cv::CAP_ANY);    // explicitly ask for auto-detection of backend
        if (!pbkd->cap.isOpened()) {
            if (pbkd->debug) fprintf(stderr, "background: cap cannot open: %s\n", path.c_str());
            return nullptr;
        }
        pbkd->cap.set(cv::CAP_PROP_CONVERT_RGB, true);
       ...
#if !HAVE_IMCOUNT
        if ( cnt < 0 )
             images = 1;
#endif

With this, the output messages are ok if the OCV version is high enough.

If OCV support only gstreamer and the version is too old, line

                if (pbkd->debug) fprintf(stderr, "background: imread cannot open: %s\n", path.c_str());

will be reached if a video file is not recognized, but opened (cnt==-1).

We can also modify the text to "background: cannot open: %s\n" if we don't like the erroneous output, and don't use the conditional code.

phlash · 2022-09-20T12:06:58Z

@jjsarton Thank you for the effort investigating the behaviour of OpenCV with various input files.. can I ask you to move that work (and related code changes) to a fresh PR against issue #158 please? It's not directly connected to your work here on croppping, and I'd like to get this work completed for the benefit of others, then we can spend more time on the format issue(s). Many thanks,

jjsarton · 2022-09-20T13:39:43Z

A lot to do!
What shall be the base for this PR?
I will modify the file deepseg.c after the first PR is integrated within backscrub. There are some crashes while using the debug mode level 2. After this reworking, the argument parser will also be the subject of a new PR, but first if a discussion tell us what we have to do.
I will prefer that Bug fixes and detection optimization remain within cropping. Writing/modifying code mean also testing it. Test shall occur on different environment, so that any developer with another system may also detect possible bugs.

BenBE · 2022-09-20T13:48:40Z

@jjsarton I personally prefer small and concise PRs with focussed changes. Currently this PR consists of >20 commits which I already asked previously to squash down into a more sensible set of commits. As this was not done yet I refrained from touching anywhere near the merge button for this PR. ;-)

If you have crashes or other bugs that are unrelated to the core issue of an PR, they should be separate. There's no problem with having multiple PRs in parallel aiming at specific aspects of the code. In fact: Doing so makes integrating them actually easier. If there are some dependencies for the order of PRs, just state them. Blowing up one PR over weeks doesn't make merging it any easier.

Marking this as draft for now, until the commit squashing has been resolved.

jjsarton · 2022-09-20T13:53:47Z

@BenBE I Know, I had to learn, and my approach was not the best. The next PR I will prepare will only cover a problem and the corresponding code, so that the chance that there will not be a need to request many changes.

BenBE · 2022-09-20T14:11:00Z

There's absolutely no problem if a PR accumulates many comments for changes that need to be done. In fact with any non-trivial change this is completely natural.

But once those are resolved would you rather read a commit history of 24 changes partially reverting each other or a concise set of about 5-10 commits "telling a story" of what needed to be changed to get to the solution?

Thus please have a look at rewriting your commit history with git rebase and combine all those commits that fix typos or revert previous changes so the history of the commits in this PR is more manageable.

jjsarton · 2022-09-21T13:50:44Z

@BenBE I has already wrote a reply to a comment like: # 153 I will definitely do this, this IMHO not a good idea.

I have joined diff files against the floe/main and the (not pushed code) I would like to have.

The reason is that backscrub shall work on different distributions, not only Debian or Debian based.

Some Distribution have integrates ffmpeg within OCV, others don't do this.

The diff files contain all changes. deepseek.cc (6 changes), libbackscrub.h (1 change) and libbackscrub.cc (2 changes). There include only the changes we have discussed.

For background.cc, there are actually 6 changes. The diff is performed against a not pushed version and contain, if possible,
a good file type detection (cv::imcount() used if provided), one fix for GIF files while using gstreamer and finally the discussed suggestions.

The file type detection as realized with the main sources is definitively worse. Reading a still image 2 times is possible with
gstreamer so that there are detected as video!.

The only subject of changes is normally (I assume that the other file are as wanted) background.cc.

background.cc.txt
deepseg.cc.txt
libbackscrub.cc.txt
libbackscrub.h.txt
sources.tar.gz
test.py.gz

added cropping of capture/background

127009b

BenBE requested changes Sep 1, 2022

View reviewed changes

BenBE reviewed Sep 1, 2022

View reviewed changes

app/deepseg.cc Outdated Show resolved Hide resolved

Jean-Jacques added 2 commits September 1, 2022 17:22

Suggestions mostly added, a little bugfix

65627e8

Suggestions mostly added, a little bugfix

7b41672

jjsarton force-pushed the cropping branch from dc251fb to 7b41672 Compare September 2, 2022 05:30

Jean-Jacques added 3 commits September 2, 2022 08:13

fix typo

cb0fe4f

typo correction

3eeb58b

fix typo

01e9a75

BenBE requested changes Sep 2, 2022

View reviewed changes

Jean-Jacques added 3 commits September 2, 2022 20:31

Syntax modified

f81e26e

Syntax modified

112e979

correction

22472ad

Jean-Jacques added 2 commits September 2, 2022 21:15

coorection as suggested

7dc9bcc

coorection as suggested

6894b3e

BenBE requested changes Sep 3, 2022

View reviewed changes

suggestion processed

1769f7e

jjsarton force-pushed the cropping branch from 89ab577 to 1769f7e Compare September 3, 2022 11:14

bug fixed

e9a5242

Jean-Jacques added 2 commits September 4, 2022 09:35

bug fixed

0f62a57

bug fix

81c1c83

BenBE reviewed Sep 4, 2022

View reviewed changes

app/background.cc Outdated Show resolved Hide resolved

Jean-Jacques added 2 commits September 4, 2022 13:24

bug fix

4c5f72c

type

ffb9883

phlash reviewed Sep 10, 2022

View reviewed changes

Fix and suggestion considered

44dbb6a

little cleanup

4c6431e

phlash reviewed Sep 11, 2022

View reviewed changes

app/background.cc Outdated Show resolved Hide resolved

little corrections

975b96f

little corrections

481cb47

phlash mentioned this pull request Sep 20, 2022

crash with model body-pix-float-050-8.tflite #151

Open

BenBE marked this pull request as draft September 20, 2022 13:49

jj.sarton@jjsarton.de added 2 commits October 1, 2022 13:03

bettet video/image detection, gstreamer support foranimated gif

f80a8a6

better crop handling

ea1e350

	cv::Rect_<int> crop = calcCropping(pbkd->raw.cols,pbkd->raw.rows,width,height);
	cv::Rect crop = calcCropping(pbkd->raw.cols, pbkd->raw.rows, width, height);

added cropping of capture/background #153

Are you sure you want to change the base?

added cropping of capture/background #153

Conversation

jjsarton commented Sep 1, 2022 • edited by BenBE

BenBE left a comment

Choose a reason for hiding this comment

jjsarton commented Sep 1, 2022

BenBE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjsarton commented Sep 2, 2022

jjsarton commented Sep 3, 2022

BenBE commented Sep 3, 2022

jjsarton commented Sep 3, 2022

jjsarton commented Sep 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjsarton Sep 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjsarton commented Sep 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phlash Sep 14, 2022 • edited

Choose a reason for hiding this comment

phlash commented Sep 11, 2022

jjsarton commented Sep 11, 2022

BenBE commented Sep 11, 2022

jjsarton commented Sep 13, 2022

BenBE commented Sep 14, 2022

jjsarton commented Sep 18, 2022

phlash commented Sep 19, 2022

jjsarton commented Sep 19, 2022

Improvements:

Detect kind of background file

Opencv with ffmpeg

Opencv with gstreamer

Not fixed behaviour

Tests

jjsarton commented Sep 19, 2022

jjsarton commented Sep 20, 2022 • edited

phlash commented Sep 20, 2022

jjsarton commented Sep 20, 2022

BenBE commented Sep 20, 2022

jjsarton commented Sep 20, 2022

BenBE commented Sep 20, 2022

jjsarton commented Sep 21, 2022

jjsarton commented Sep 1, 2022 •

edited by BenBE

jjsarton Sep 10, 2022 •

edited

phlash Sep 14, 2022 •

edited

jjsarton commented Sep 20, 2022 •

edited