backscrub
backscrub copied to clipboard
floe
added cropping of capture/background
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!
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!
Looks already good so far. Care to squash/fixup those commits into one (or few) …
@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.
I have a little error at line 613 deepseg.c and fixed it. I have also removes 2 space within an if query as you recommend it. I have also pushed this to my origin branch.
Thanks for your patience @jjsarton, this is looking good. You might have missed a comment I put on the issue: https://github.com/floe/backscrub/issues/149#issuecomment-1242717372, 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.
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.
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.
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.
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.
size_t images = cv:imcount(path, cv:IMREAD_ANYCOLOR)
I see a couple of issues with this approach:
imcountisn'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?
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
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.
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.
@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,
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.
@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.
@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.
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.
@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