-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add alternate timestamp functionality #170
Conversation
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.
dvr_scan/video_joiner.py
Outdated
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. |
There was a problem hiding this comment.
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
.
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 |
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... |
I reverted the workaround with the last_position_ms since that wasn't the root cause, so accordingly there's no testcase for it. |
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.
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. |
There was a problem hiding this 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.
Two commits add the config options and the ability to use postion_ms from the VideoStream source for event timestamps.