Perceptual Hash Detector #290
Conversation
There was a problem hiding this comment.
This is awesome! We need to add some tests before merging this. There's also documentation that needs to be updated, but that can be done after in a separate change - I can help with that part too.
Before merging this, we need to:
- Retarget for v0.6.1 branch
- Add unit tests
I'm curious, do you know how well this handles motion? I saw you had used motion correction for the edge detector, so was wondering if that would be applicable here or not as well. This PR also makes me rethink a lot of how to combine various detection methods, since now we have the possibility to incorporate all of HSV deltas, edges, and perceptual hashes. In v0.6.1 I may move the edge detection part back to a separate detector as per your original design, rather than combining it into ContentDetector. We could then use how confident each detector was to improve the final result.
scenedetect/cli/__init__.py
Outdated
| ) | ||
| @click.option( | ||
| '--freq_factor', '-f', metavar='VAL', | ||
| type=click.IntRange(min=1), default=2, show_default=True, help= |
There was a problem hiding this comment.
Here and elsewhere, default values must be None if they correspond to config file values. This was the only way I could find to figure out if the config value or the CLI value should be used.
Instead, you can append USER_CONFIG.get_help_string('detect-hash', 'freq-factor')) in the help text so it still shows the default.
There was a problem hiding this comment.
I think I addressed this in efa4023 (I based this off what you did in detect-threshold, but would love you to confirm.
scenedetect/cli/__init__.py
Outdated
| ) | ||
| @click.option( | ||
| '--size', '-s', metavar='VAL', | ||
| type=click.IntRange(min=2), default=16, show_default=True, help= |
There was a problem hiding this comment.
Should re-use the constant defined in CONFIG_MAP for min in config.py so changing the value there updates all uses.
There was a problem hiding this comment.
I think I addressed this in efa4023 (I based this off what you did in detect-threshold, but would love you to confirm.
scenedetect/cli/__init__.py
Outdated
| ) | ||
| @click.option( | ||
| '--freq_factor', '-f', metavar='VAL', | ||
| type=click.IntRange(min=1), default=2, show_default=True, help= |
There was a problem hiding this comment.
Should re-use the value from CONFIG_MAP for min here and elsewhere.
There was a problem hiding this comment.
I think I addressed this in efa4023 (I based this off what you did in detect-threshold, but would love you to confirm.
scenedetect/cli/__init__.py
Outdated
| ) | ||
| @click.option( | ||
| '--freq_factor', '-f', metavar='VAL', | ||
| type=click.IntRange(min=1), default=2, show_default=True, help= |
There was a problem hiding this comment.
Default values need to be None in order to work with the config file (here and elsewhere). To get the default value to show up, you can append USER_CONFIG.get_help_string('detect-hash', 'freq-factor')) to the help string. Sorry I know this is a very non-obvious thing (gotta love business logic).
There was a problem hiding this comment.
I think I addressed this in efa4023 (I based this off what you did in detect-threshold, but would love you to confirm.
scenedetect/cli/context.py
Outdated
| if self.drop_short_scenes: | ||
| min_scene_len = 0 | ||
| else: | ||
| if min_scene_len is None: | ||
| if self.config.is_default("detect-hash", "min-scene-len"): | ||
| min_scene_len = self.min_scene_len.frame_num | ||
| else: | ||
| min_scene_len = self.config.get_value("detect-hash", "min-scene-len") | ||
| min_scene_len = parse_timecode(min_scene_len, self.video_stream.frame_rate).frame_num |
There was a problem hiding this comment.
Probably time to refactor this into a helper function (e.g. get_min_scene_len("detect-hash") or something) and use that here and in the other detectors.
There was a problem hiding this comment.
I implemented a version of this in 688d67e as a private function of the CliContext class (_get_min_scene_len(command)). I tested it with all the detectors (and found a typo from an earlier commit of mine in the process) and it seems to work for all of them. Let me know if you think this should be done another way.
|
One thing I think might need to be updated is the API docs and the |
|
Finally got this into develop, thanks for your patience! There was some refactoring done to the CLI in the latest version, so it doesn't show up on the CLI yet. Will need to fix that in develop branch, but I'll make sure to do that soon. Thanks a ton for this, amazing work! |
Hi @Breakthrough,
This is a proof of concept for a perceptual hash based detection algorithm I had started working on a while back. I updated things to be compatible with the 0.6.x updates (I think) and implemented things in OpenCV without adding additional dependencies (previously I had PIL and scipy). I also added a cli command and help strings, but have not written any actual documentation yet.
As for the actual algorithm, I re-implemented the phash algorithm from the imagehash library. For a more human readable version of the algorithm, you can see the blog post here.
Performance wise, I chose defaults that worked well on the goldeneye clip and seemed fairly performant.
There might be some formatting issues to make it match the updated codebase (typing for instance). Feel free to change things to make it fit or, point it out and I will try to take a crack at it.