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.

Sign up for GitHub

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

Jump to bottom

Extend image_name_template to allow timestamp #395

Merged
merged 12 commits into from
May 3, 2024
Merged

Extend image_name_template to allow timestamp #395

merged 12 commits into from
May 3, 2024

Conversation

Veldhoen
Copy link

@Veldhoen Veldhoen commented Apr 26, 2024 •
edited

Fixes #394

@Veldhoen Veldhoen changed the title Added TIMESTAMP_MS to file_path substitutions Extend image_name_template to allow timestamp Apr 26, 2024
@Breakthrough Breakthrough linked an issue Apr 27, 2024 that may be closed by this pull request
Extend image_name_template to allow timestamp #394
Open
@Breakthrough
Copy link
Owner

Breakthrough commented Apr 27, 2024 •
edited

I see in your PR you've already used the $TIMESTAMP_MS format as per the discussion in #394. Would you be able to run yapf to format the resulting code so it passes the formatting checks?

Thank you for the PR!

Breakthrough
Breakthrough requested changes Apr 28, 2024
View reviewed changes
Copy link
Owner

@Breakthrough Breakthrough left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for the PR, please address the following so this can be landed:

  • Rebase ontop of the develop branch
  • Ensure the code is formatted correctly: python -m yapf -i -r scenedetect tests

@Veldhoen
Copy link
Author

Thanks for replying so fast, even though my PR was still a draft. I'll make sure to address the issues you mentioned, hopefully later today.

@Veldhoen Veldhoen marked this pull request as ready for review April 30, 2024 08:04
@Veldhoen
Copy link
Author

Rebased on develop now, and I think the formatting is in line with your standards now.
Also added $TIMECODE like you suggested in the issue, and added both $TIMECODE and $FRAME_NUMBER to test_save_images.

Documentation still needs to be updated to explain these extensions.

@Veldhoen Veldhoen changed the base branch from main to develop April 30, 2024 08:29
Breakthrough
Breakthrough requested changes May 1, 2024
View reviewed changes
Copy link
Owner

@Breakthrough Breakthrough left a comment

Choose a reason for hiding this comment

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

Looks like the test_save_images test is failing on Windows, will approve once fixed. Thank you!

scenedetect/scene_manager.py Outdated
IMAGE_NUMBER=image_num_format % (j + 1),
FRAME_NUMBER=image_timecode.get_frames(),
TIMESTAMP_MS=int(image_timecode.get_seconds() * 1000),
TIMECODE=image_timecode.get_timecode()),
Copy link
Owner

Choose a reason for hiding this comment

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

Timecodes need to be formatted specially as not all operating systems support : in paths. Please replace them with ; as is done when splitting videos:

TIMECODE=image_timecode.get_timecode().replace(":", ";")

This is most likely why test_save_images is failing on the Windows build currently.

tests/test_scene_manager.py Show resolved Hide resolved
@Veldhoen
Copy link
Author

Veldhoen commented May 2, 2024

I applied the requested changes, and extended docstring. Hope this will do!

@Veldhoen Veldhoen requested a review from Breakthrough May 2, 2024 12:32
@Breakthrough Breakthrough merged commit 7629645 into Breakthrough:develop May 3, 2024
35 checks passed
@Breakthrough
Copy link
Owner

Much appreciated, approved.

Thank you for the PR!

@Veldhoen
Copy link
Author

Veldhoen commented May 6, 2024

@Breakthrough When do you plan to release 0.6.4? I created a pre-release of my fork to be able to use the functionality right away, but I'd like to use a proper release at some point, of course.

@Breakthrough
Copy link
Owner

I'm hoping to wrap up the next release by the end of this month. If for whatever reason you need this sooner in production, I can consider pushing a pre-release to PyPI.

@Veldhoen
Copy link
Author

If you can make it by the end of this month, that would be great. Until then we'll be fine with my ad hoc pre-release. Thanks!

@Veldhoen
Copy link
Author

Veldhoen commented Jun 7, 2024

@Breakthrough do you have any updates concerning the release of 0.6.4? Would be much appreciated :)

@Breakthrough
Copy link
Owner

Breakthrough commented Jun 10, 2024 •
edited

All feature work is complete, just finishing up testing and preparing the release. The new version should be out within a day or two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Breakthrough Breakthrough Breakthrough approved these changes

Assignees
No one assigned
Labels
improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extend image_name_template to allow timestamp
2 participants
@Veldhoen @Breakthrough