Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[keyframeselection] Algorithms are rewritten #1213

Closed
wants to merge 12 commits into from

Conversation

servantftechnicolor
Copy link
Contributor

Keyframe selection was not working previously.

This PR tries a new set of algorithms to replace the previous one

  • A simple one which simply perform a regular sampling on the temporal dimension
  • A more complex one which take the frame with the sharper value in a window, and check that this frame has sufficient motion with the previous selection frame.

@fabiencastan
Copy link
Member

fabiencastan commented Sep 5, 2022

#include <opencv2/optflow.h> is not found in the linux CI.
Should we add a module here? https://github.com/alicevision/AliceVision/blob/develop/CMakeLists.txt#L687

@fabiencastan
Copy link
Member

Do we really need highgui here?
keyframe/KeyframeSelector.cpp:20:31: fatal error: opencv2/highgui.hpp: No such file or directory

@mh0g
Copy link
Contributor

mh0g commented Sep 30, 2022

  • simple mode tested and works

std::mt19937 randomTwEngine(rd()); // standard mersenne_twister_engine seeded with rd()
std::uniform_int_distribution<> randomDist(0, std::numeric_limits<int>::max());
return randomDist(randomTwEngine);
std::random_device rd; // will be used to obtain a seed for the random number engine
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make testing unreliable. It's best to supply the source of randomness as a parameter so that in tests fake implementation can be used.

Copy link
Contributor

@cbentejac cbentejac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In simple mode, if maxNbOutFrame is set, the number of extracted frames is always equal to maxNbOutFrame+1.

The "smart" mode seems to be working correctly!

src/aliceVision/keyframe/KeyframeSelector.hpp Outdated Show resolved Hide resolved
@mh0g
Copy link
Contributor

mh0g commented Oct 25, 2022

Tested simple and advanced mode on windows, seems to work as intended.
Note: cmake option ALICEVISION_USE_OPENCV is off by default, to be working off the shelf, cmake needs to be called with -DALICEVISION_USE_OPENCV=ON

mh0g
mh0g previously approved these changes Oct 25, 2022
Copy link
Contributor

@mh0g mh0g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on windows, seems to work as intended
image

@cbentejac cbentejac force-pushed the dev/keyframeSelection branch 3 times, most recently from 724a040 to 3a988db Compare November 23, 2022 10:09
@cbentejac cbentejac force-pushed the dev/keyframeSelection branch from 8969a23 to 7b2f0d1 Compare December 6, 2022 17:31
{
ALICEVISION_LOG_ERROR("Cannot initialize the FeedProvider with " << path);
throw std::invalid_argument("Cannot while initialize the FeedProvider with " + path);
maxstd = std::max(maxstd, std_2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, with the max here, one pixel will give the score for the whole image? If that's the case, this does not sound reliable.

servantftechnicolor and others added 10 commits December 19, 2022 10:32
Address PR reviews:
- give more explicit names to some variables
- harmonize coding style
- avoid some useless computations
- If _maxOutFrame is not 0 and _minFrameStep is superior to 1, stop
ignoring _minFrameStep, take it into account and always enforce it
- Ensure that there will be no more than _maxOutFrame selected frames
when _maxOutFrame is not 0
Use the same indentation (4 spaces) for both the KeyframeSelector
header and source files.
@cbentejac
Copy link
Contributor

Replaced by #1343.

@cbentejac cbentejac closed this Jan 25, 2023
@cbentejac cbentejac deleted the dev/keyframeSelection branch August 22, 2023 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants