Added option to completely skip RTSP Discovery in autodiscover() #47
Conversation
src/framegrab/cli/autodiscover.py
Outdated
| @@ -23,7 +23,7 @@ | |||
| @click.option( | |||
| "--rtsp_discover_modes", | |||
| type=click.Choice(PREVIEW_RTSP_COMMAND_CHOICES, case_sensitive=False), | |||
| default="light", | |||
| default="disable", | |||
There was a problem hiding this comment.
Might be better to set it to disable since we don't want to guess passwords by default.
There was a problem hiding this comment.
I think we do want to guess passwords in autodiscover. Without the password, we cannot connect to the grabber, which is the point of the autodiscover, right? I guess there is the question of how thorough we are with our password guessing; maybe that should be an option?
There was a problem hiding this comment.
Yea, the mode light should be alright but other modes such as complete_fast and complete_slow may cause you to exceed the number of attempts before the camera blocks you.
There was a problem hiding this comment.
Looks pretty good, but I have a few comments/questions.
Should this also be added to the CLI? I think the behavior should be similar to the function, default behavior is to not search for rtsp cameras. Then maybe there is a flag they can set to search for rtsp cameras or not. Or maybe they specify a class AutodiscoverMode?
Also, I wonder if class AutodiscoverModes should be class AutodiscoverMode (singular). When i see a line like this, it makes me think that I am passing in a list of modes, but it's really just a string.
def autodiscover(warmup_delay: float = 1.0, rtsp_discover_modes: Union[AutodiscoverModes, None] = None) -> dict:
src/framegrab/cli/autodiscover.py
Outdated
| @@ -23,7 +23,7 @@ | |||
| @click.option( | |||
| "--rtsp_discover_modes", | |||
| type=click.Choice(PREVIEW_RTSP_COMMAND_CHOICES, case_sensitive=False), | |||
| default="light", | |||
| default="disable", | |||
There was a problem hiding this comment.
I think we do want to guess passwords in autodiscover. Without the password, we cannot connect to the grabber, which is the point of the autodiscover, right? I guess there is the question of how thorough we are with our password guessing; maybe that should be an option?
src/framegrab/cli/autodiscover.py
Outdated
| type=click.Choice(PREVIEW_RTSP_COMMAND_CHOICES, case_sensitive=False), | ||
| default="light", | ||
| default="disable", |
There was a problem hiding this comment.
Nit, but 'disable' is a weird choice for the default. 'disable' implies the default should be to search for rtsp urls and we're going out of our way to turn it off. 'off' or 'none' would make more sense to me
There was a problem hiding this comment.
Yea I agree, off would be a better naming for this.
README.md
Outdated
| @@ -174,7 +174,7 @@ The table below shows all available configurations and the cameras to which they | |||
| In addition to the configurations in the table above, you can set any Basler camera property by including `options.basler.<BASLER PROPERTY NAME>`. For example, it's common to set `options.basler.PixelFormat` to `RGB8`. | |||
|
|
|||
| ### Autodiscovery | |||
| Autodiscovery automatically connects to all cameras that are plugged into your machine or discoverable on the network, including `generic_usb`, `realsense` and `basler` cameras. Default configurations will be loaded for each camera. Please note that RTSP streams cannot be discovered in this manner; RTSP URLs must be specified in the configurations or can be discovered using a separate tool below. | |||
| Autodiscovery automatically connects to all cameras that are plugged into your machine or discoverable on the network, including `generic_usb`, `realsense`, `basler`, and ONVIF supported `RTSP` cameras. Default configurations will be loaded for each camera. Note that discovery of RTSP cameras will be disabled by default but can be enabled by setting `rtsp_discover_mode`. Refer to [RTSP Discovery](#rtsp-discovery) section for different options. | |||
There was a problem hiding this comment.
should just be rstp (lowercase) here. Should match the actual input type that is used when creating the grabber.
src/framegrab/grabber.py
Outdated
| light: Only try first two usernames and passwords ("admin:admin" and no username/password). | ||
| complete_fast: Try the entire DEFAULT_CREDENTIALS without delays in between. | ||
| complete_slow: Try the entire DEFAULT_CREDENTIALS with a delay of 1 seconds in between. | ||
| Defaults to AutodiscoverModes.light. | ||
| Defaults to ip_only. |
There was a problem hiding this comment.
Should 'Defaults to ip_only' be removed? It default to off, right?
This PR added a option to turn off RTSP Discovery as in some cases it will take some time to search for cameras in the local network.