Add save-images option for ignoring a number of frames at start/end.
#129
Conversation
Add `--image-frame-margin` option, which specifies a number of frames to ignore at the beginning and end of scenes when generating images. This is useful if scene cuts may have frames from the previous/next scene in them for some reason, or if there are some transition effects or camera movement between scenes.
scenedetect/cli/context.py
Outdated
| @@ -204,7 +204,7 @@ def _generate_images(self, scene_list, video_name, | |||
| else: | |||
| middle_images = self.num_images - 2 | |||
| for i, (start_time, end_time) in enumerate(scene_list): | |||
| timecode_list[i].append(start_time) | |||
| timecode_list[i].append(start_time + self.image_frame_margin) | |||
There was a problem hiding this comment.
Would prefer bound checks on the timecodes being used to make sure they do not exceed the frame boundaries. Specifically, if the calculated start/end times would exceed the half-way point, I think that should be defined behavior - what that behavior should be, though, I'm not 100% sure on, so any feedback or opinions you have would be welcome.
There was a problem hiding this comment.
Also I know I'm essentially asking you to do work for free, so if you are unable to support any of the requested changes I've made on this PR or any other ones, please don't hesitate to let me know, and I'll take over the rest of the PR accordingly.
Very pleased with the quality of your commits thus far though (very small and targeted changes), sorry I don't have more time to actively support them as much as I used to. Thank you again for your PRs and issue requests, these are definitely things I want to add in v0.5.2 if possible (hurray for non-breaking changes :)!
There was a problem hiding this comment.
See what you think about a393efc. It's a pretty significant rewrite of the code that builds the timecode list for images, but I think it accomplishes what you want by dividing each scene's frames into --num-images chunks and then selecting the appropriate frame from each.
The min and max make sure that the frame for the first and last images never go past the frames for the middle images. Lightly tested with some crazy high values of --image-frame-margin and it seems to work as expected.
There was a problem hiding this comment.
Awesome, have you tested with values of 0 and 1 by any chance? Also I'd like to write some unit tests for this eventually as well, but won't let that keep this from being merged.
Would like to do some testing on this myself before merging, but at a first glance your implementation looks a lot cleaner than mine, thank you! 😄 👍
Rewrite save-images timecode generation as a list comprehension that: * iterates through the scene list * builds a range of frames in each scene * splits that range into evenly-sized chunks, and * selects the appropriate frame from each chunk, using `--image-frame-margin` for the first and last frames, but not going past the frame number of the next image. As a side effect, also adds FRAME_NUMBER to image template (used for debugging)
The rewritten code for save-images throws an IndexError when the number of frames in the scene is less than the number of images. This commit fixes that by padding the list of frames with the last frame in the scene.
There was a problem hiding this comment.
Awesome! Looks great, thanks so much for addressing my comments! I'd like to write some unit tests for this before merging it, but I can handle that (sorry I don't have a CI setup to automate testing yet...). I'll set a deadline of one week for this to be merged, and if it exceeds that time, will open a new issue to create unit tests in the future (I'll also do some manual testing for now, and if it all checks out will merge as-is).
Thank you again so much for your help on this @tonycpsu!
|
After re-reviewing the changes, this will likely be merged as-is, with unit tests being written as part of #88. Thanks again for the PR, will get this merged by end of the week! |
| self.video_manager.seek(image_timecode) | ||
| self.video_manager.grab() | ||
| ret_val, frame_im = self.video_manager.retrieve() | ||
| if ret_val: | ||
| file_path = '%s.%s' % (filename_template.safe_substitute( | ||
| VIDEO_NAME=video_name, | ||
| SCENE_NUMBER=scene_num_format % (i + 1), | ||
| IMAGE_NUMBER=image_num_format % (j + 1)), | ||
| IMAGE_NUMBER=image_num_format % (j + 1), | ||
| FRAME_NUMBER=image_timecode.get_frames()), |
There was a problem hiding this comment.
Need to add this to the documentation, but I can do that before merging.
Add
--image-frame-marginoption, which specifies a number of framesto ignore at the beginning and end of scenes when generating images.
This is useful if scene cuts may have frames from the previous/next
scene in them for some reason, or if there are some transition effects
or camera movement between scenes.