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

Add alternate timestamp functionality #170

Merged
merged 8 commits into from
Jul 22, 2024

Conversation

goatzillax
Copy link
Contributor

Two commits add the config options and the ability to use postion_ms from the VideoStream source for event timestamps.

The previous default behavior of calculating time from frame numbers should
be preserved.

The switch kind of smears alternate timekeeping through the scanner to fetch
position_ms from the video source and stuff it into MotionEvent so the rest
of the code should still behave the same.

Might need more testing; was unsure about some of the frame_skip bits.

Fixes Breakthrough#168
position_ms seems to return 0 after reading past the end of a stream.
@Breakthrough Breakthrough added this to the v1.7 milestone Jul 10, 2024
@Breakthrough Breakthrough self-requested a review July 10, 2024 03:42
def read(self, decode: bool = True) -> Optional[numpy.ndarray]:
"""Read/decode the next frame."""

# I get the feeling when you run off the end of a file the position_ms returns 0.
Copy link
Owner

Choose a reason for hiding this comment

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

Could we add a unit test to verify this? You can add one to tests/test_video_joiner.py.

@Breakthrough
Copy link
Owner

This is awesome, thanks so much! Could you add a few short unit tests and fix the code formatting checks to land this?

(can autoformat files by running python -m yapf -i -r dvr_scan/ tests/ from the root of the repo)

@goatzillax
Copy link
Contributor Author

OK, sorry about that. I'll get those cleaned up and add a test.

I found a bug in this implementation; I think position_ms needs to be attached to the DecodeEvent in _decode_thread where the read actually happens. Calling it in the scan actually produces nondeterministically wrong results. I think it should be a straightforward change though...

@goatzillax
Copy link
Contributor Author

I reverted the workaround with the last_position_ms since that wasn't the root cause, so accordingly there's no testcase for it.

@Breakthrough
Copy link
Owner

Thanks for fixing this up! I can add a test after merging if you don't think it needs one. What happened to be the issue?

Breakthrough
Breakthrough previously approved these changes Jul 15, 2024
@goatzillax
Copy link
Contributor Author

Thanks for fixing this up! I can add a test after merging if you don't think it needs one. What happened to be the issue?

The root issue was I forgot the decoding was happening asynchronously on its own thread, so calling the position on the underlying VideoCapture element in (different) scan thread would be out of sync with the frame being worked on. Most of the time it was subtle until the end of the file where it was not so subtle.

I can add that test; seems reasonable.

PTS uses a slightly different time than the frame_num which is
shifted by 1, causing a slight slide in the expected events.
@goatzillax
Copy link
Contributor Author

I added the test and adjusted a few things.

For a motion event that's still going when a file ends, I was adding the the post_even_len since ffmpeg doesn't seem to care if you do that, and that seemed more inclusive to me, but I've changed it to match the existing behavior.

The other thing is the start events are slightly off because frame_num seems to be shifted to after the frame.

Doing something similar for the PTS case might be counterproductive somewhere down the line. I added the testcase and gave it a 1 frame tolerance to compensate.

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.

Thanks for fixing this up. Really appreciate the PR!

Will make sure this gets in for the next release.

@Breakthrough Breakthrough merged commit 78a24ff into Breakthrough:develop Jul 22, 2024
28 checks passed
@Breakthrough Breakthrough modified the milestones: v1.7, v1.6.2 Nov 24, 2024
@Breakthrough Breakthrough mentioned this pull request Dec 8, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants