Improvements to Dahua/Amcrest module

ascott18 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/aio-libs/aiohttp/pull/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 commented 2 years ago

In 0c64b096cd193cb1eff78894fd8b16af1869531f, 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 commented 2 years ago

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 commented 2 years ago

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

ascott18 commented 2 years ago

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 commented 2 years ago

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.

https://github.com/ascott18/unifi-cam-proxy/commit/216894cb91755c89c8f369638ce5b116d92d234c

keshavdv commented 2 years ago

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

keshavdv / unifi-cam-proxy

Improvements to Dahua/Amcrest module #135