-
Notifications
You must be signed in to change notification settings - Fork 13
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 Agent to support RTSP compatible IP cameras #605
Conversation
Just a note: this agent does require the |
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 so much for writing this agent! Here's my initial review with some requests. Mainly name changes and core functionality requests.
I'm also wondering if the classes CircularMediaBuffer
and MotionDetector
should become external classes since they can also be used by the ACTiCameraAgent
and any other similar agents in the future. Though this can be a discussion with others.
This is great. Thanks for reviewing @davidvng, and thanks for the PR @tskisner!
This would be good to do. If these will be used in the functions that @davidvng suggested get pulled out for use in the ACTi camera agent then I don't mind them going in the main Since I assume you're busy with other things now that you're back from the site @tskisner, let's go ahead and merge this as is (with added requirements to the |
This is now ready for re-review @davidvng. |
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.
tests/agents/test_camera_rtsp.py
Outdated
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.
Is it possible to use the FakeCamera
class and test_mode
to test the agent here? This may become an integration test instead. Thoughts @BrianJKoopman?
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.
This sounds good, but I don't think we should let it hold up this PR at this point if it's not straightforward.
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.
Agreed, it is more important in my opinion to get this merged, so that at least there is a starting point for future development / extension. I just rebased this branch and dealt with the setup.py to pyproject.toml transition. Another example of why we should merge this if it does no harm.
motion_start (str): ISO time (HH:MM:SS+-zz:zz) to start motion detection. | ||
motion_stop (str): ISO time (HH:MM:SS+-zz:zz) to stop motion detection. |
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.
Using +-zz:zz here
might be a problem since we don't want to rely on remembering and having to change this config when daylight savings happens. I think we may just want to force these arguments to only accept HH:MM:SS
and tell users it must be UTC or do handling differently in _in_motion_time_range._dt_convert
.
for testing. Default is False. | ||
|
||
Notes: | ||
Individual frames are written to a circular disk buffer. Metadata |
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.
Can you include video to this note?
cap = cv2.VideoCapture(self.connection) | ||
if not cap.isOpened(): | ||
self.log.error(f"Cannot open RTSP stream at {self.connection}") | ||
return False, "Cannot connect to camera" |
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.
Does this connection have any conflicts with the already open connection in acq
?
message = { | ||
"block_name": "cameras", | ||
"timestamp": timestamp, | ||
"data": { | ||
"address": self.address, | ||
"path": path, | ||
}, | ||
} |
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.
Can you add movement
bool to this? We may want to monitor on Grafana and trigger alarms if the cameras see movement.
_ = cap.grab() | ||
success, image = cap.retrieve() |
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.
Does this grab
have to be within the loop or can it just grab at the end? From my understanding, this is grabbing 200 images for example but only retrieving the last one.
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.
This is a consequence of the way these cameras work. You have to continuously grab images and extract the ones you want.
blur (int): The odd number of pixels for blurring width | ||
threshold (int): The grayscale threshold (0-255) for considering | ||
image changes in the blurred images. | ||
dilation (int): The dilation iterations on the thresholded image. | ||
min_frac (float): The minimum fraction of pixels that must change | ||
to count as a detection. | ||
max_frac (float): The maximum fraction of pixels that can change | ||
to still count as a detection (rather than a processing error). |
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.
Can you add these arguments as optional kwargs to the agent? We may want to change these if the camera FOV, room size, etc cause motion detection to fail.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
315e1ed
to
be84975
Compare
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.
Made some small packaging/docs changes. Will squash and merge when checks complete.
This agent adds support for acquiring snapshots and short recordings from cameras that can stream the RTSP protocol.
Description
This new agent records frames to disk (default every 10s) and passes them through a motion detection algorithm. If there is motion, a 60 second recording is taken. Recordings can also be triggered directly as an agent Task. Both images and video are stored to disk in circular buffers which by default hold the most recent 2 days of snapshots and 120 minutes of triggered videos. Use of this agent requires the opencv / cv2 and imutils packages.
Note that each agent instance handles exactly one camera. This enables using different settings for different camera placements.
Motivation and Context
This agent was created to support the Dahua security cameras at the site. These are interior cameras designed to capture unauthorized access or theft.
How Has This Been Tested?
This agent has been tested on a local system (laptop) at the site. It has been run for several hours and successfully detected motion in one of the containers as people went in and out. Note that I only tested this running as a local agent on the host system. I verified that the CPU and memory use are not too dramatic. There is a brief spike at the end of a recording when the list of frames is encoded to mp4. Here is an example of motion detection, which draws a green box around the areas where a change was detected:
Types of changes
Checklist: