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

Improvements to Dahua/Amcrest module #135

Closed
wants to merge 7 commits into from
Closed

Conversation

ascott18
Copy link
Contributor

  • Support digest auth
  • Cleanup motion detection request when restarting (don't leave multiple requests running at once)
  • Add documentation for Dahua module, noting that it also works for Amcrest.

@keshavdv
Copy link
Owner

keshavdv commented Jan 4, 2022

Thanks for the contribution! Would switching from aiohttp to httpx make this implementation easier since that seems to support both asyncio and digest auth natively?

@ascott18
Copy link
Contributor Author

It may well do. I'm not a Python dev at all and was just looking for the lowest friction path to get this working, which was borrowing from aio-libs/aiohttp#2213.

I did all this work during some time off work I took after Christmas, but I don't forsee having much time to devote to this going forwards.

@keshavdv
Copy link
Owner

In 0c64b09, I switched to an external python module that provides digest auth and async friendly APIs for most functionality which also enabled smart detections on compatible cameras. Do you want to try this version out to see if it does what you need?

@ascott18
Copy link
Contributor Author

Looks like that's working for me. However, the motion detection stream isn't being cleanup up on teardown, so I'm seeing duplicates after the "pretending to upgrade". For each motion event, I see two Trigger motion start for index 0 and two Trigger motion end for index 0.

@keshavdv
Copy link
Owner

Nice catch! Hopefully addressed in 7c7d62f. Do you mind giving this a go?

@ascott18
Copy link
Contributor Author

ascott18 commented Jan 24, 2022

Thanks, that works hang on, still saw a duplicate motion event. Investigating more...

However, one more thing with the switch to the amcrest module - the rstp_url function actually hits the camera's API, so I had to wrap it in a try block in order for the script to not crash when the camera disconnects:

    def get_stream_source(self, stream_index: str) -> str:

        if stream_index == "video1":
            subtype = self.args.main_stream
        else:
            subtype = self.args.sub_stream

        try:
            return self.camera.rtsp_url(channel=self.args.channel, typeno=subtype)
        except CommError:
            raise RetryableError()

@ascott18
Copy link
Contributor Author

ascott18 commented Jan 24, 2022

Ok, a few things I found:

  • I saw the motion event stream get duplicated once, but was unable to reproduce again. It seems to always behave properly under normal operating conditions (doesn't get duplicated on startup during the fake upgrade).
  • I added the try in my previous comment for the rtsp url to prevent crashes on camera disconnect.
  • I had to add httpx.ConnectTimeout as an additional error to catch in the motion event API loop - It looks like there are some paths in the amcrest module that aren't wrapped up in a CommError?
  • One attempt at unplugging the camera saw a motion event get stuck "open". I think if a await self.trigger_motion_stop() was added to Base.close() for good measure it may prevent this.
  • I realized that stop_video_stream is not actually stopping the stream. Since the process is spawned with shell=true, this just aborts the parent shell process without aborting the ffmpeg/clock_sync/nc processes. I came across https://stackoverflow.com/questions/4789837 that explained this. However, I'm not sure that this really warrants changing, since it honestly works OK as it is. Unifi Protect doesn't seem to care that the video stream keeps coming in as the camera pretends to be upgrading.

ascott18@216894c

@keshavdv
Copy link
Owner

Closing this out since I think we've finally addressed those issues as well.

@keshavdv keshavdv closed this Jan 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants