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

Feature: automatic camera resolution configuration #6810

Merged

Conversation

skrashevich
Copy link
Contributor

No description provided.

…les with detect in the CameraConfig class in config.py
…aConfig class and add optional parameter to retrieve video duration in get_video_properties function
@netlify
Copy link

netlify bot commented Jun 15, 2023

Deploy Preview for frigate-docs canceled.

Name Link
🔨 Latest commit 59e7ec1
🔍 Latest deploy log https://app.netlify.com/sites/frigate-docs/deploys/649909e213aeb10008e933d2

Copy link
Sponsor Collaborator

@NickM-27 NickM-27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the fps should be set to 5 even if the source stream is higher, this is by design as the default recommended fps for most users. Not all users actually set their cameras to this though.

frigate/config.py Outdated Show resolved Hide resolved
@skrashevich
Copy link
Contributor Author

I believe the fps should be set to 5 even if the source stream is higher, this is by design as the default recommended fps for most users

this PR don't touch fps setting

@NickM-27
Copy link
Sponsor Collaborator

Gotcha, my bad

@NickM-27
Copy link
Sponsor Collaborator

Also just a reminder once things are decided the configuration/index docs should be updated. In this case the getting started guide should also be updated to remove the recommendation to set the width / height

@skrashevich
Copy link
Contributor Author

And i don’t have idea what’s best to do if several streams are available by URL. To take the first, or the best?

@NickM-27
Copy link
Sponsor Collaborator

I'm not familiar with cameras that have multiple streams available on the exact same URL. In that case maybe log a warning that auto detection found multiple resolutions. I think it's safer to pick the lowest quality stream to ensure cpu isn't taxed too much

@skrashevich skrashevich force-pushed the 230615-feature-camera-autoconf branch from 0505874 to 4954b26 Compare June 15, 2023 01:00
@skrashevich skrashevich force-pushed the 230615-feature-camera-autoconf branch from 4954b26 to a1aed04 Compare June 15, 2023 01:05
@skrashevich skrashevich force-pushed the 230615-feature-camera-autoconf branch from 4793b82 to 2fff932 Compare June 15, 2023 01:17
frigate/config.py Outdated Show resolved Hide resolved
Co-authored-by: Nicolas Mowen <nickmowen213@gmail.com>
@skrashevich skrashevich force-pushed the 230615-feature-camera-autoconf branch 6 times, most recently from 7f539e5 to e96150f Compare June 15, 2023 02:58
@skrashevich skrashevich force-pushed the 230615-feature-camera-autoconf branch from e96150f to 747006c Compare June 15, 2023 04:24
frigate/config.py Outdated Show resolved Hide resolved
Co-authored-by: Blake Blackshear <blakeb@blakeshome.com>
@blakeblackshear
Copy link
Owner

Earlier versions did autodetect the resolution like this. I remember there being enough issues with the reliability of cv2.VideoCapture that I decided it was more hassle than it was worth. It's worth trying again.

docs/docs/configuration/camera_specific.md Outdated Show resolved Hide resolved
docs/docs/configuration/cameras.md Outdated Show resolved Hide resolved
docs/docs/configuration/index.md Outdated Show resolved Hide resolved
docs/docs/development/contributing.md Outdated Show resolved Hide resolved
docs/docs/guides/getting_started.md Outdated Show resolved Hide resolved
skrashevich and others added 2 commits June 15, 2023 15:42
Co-authored-by: Nicolas Mowen <nickmowen213@gmail.com>
@skrashevich skrashevich force-pushed the 230615-feature-camera-autoconf branch from aeb3a7b to b90eba9 Compare June 15, 2023 12:58
@skrashevich
Copy link
Contributor Author

skrashevich commented Jun 15, 2023

Earlier versions did autodetect the resolution like this. I remember there being enough issues with the reliability of cv2.VideoCapture that I decided it was more hassle than it was worth. It's worth trying again.

I can modify get_video_properties function to use ffprobe as fallback, if cv2 will fail to determine stream

@NickM-27
Copy link
Sponsor Collaborator

I can modify get_video_properties function to use ffprobe as fallback, if cv2 will fail to determine stream

this is what I am doing for the get_duration part as well since I found some segment that cv2 could not get the duration of

Copy link
Sponsor Collaborator

@NickM-27 NickM-27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ffprobe should be used to get width / height when cv2 fails. also a couple other docs changes

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think the top level "minimal config" should just have the detect config removed.

and regardless, the specific detect config needs to e updated.

detect:
# Optional: width of the frame for the input with the detect role (default: shown below)
width: 1280
# Optional: height of the frame for the input with the detect role (default: shown below)
height: 720

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but this lines in the 'Full configuration reference' part

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first part is specifically in the Minimal Configuration part and the width / height is not part of a minimal configuration

Screen Shot 2023-06-19 at 13 45 44 PM

The second part needs to be updated. The defaults are no longer 1280 x 720 the default is to auto detect with a fallback of 1280x720.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. on your screenshot lines 3-24. But in this thread we are talking about lines 195-199 (see first message)

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, but for lines 195-199 they need to be updated because

The defaults are no longer 1280 x 720, the default is now to auto detect with a fallback of 1280x720.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, but for lines 195-199 they need to be updated because

I think, here only needs to change the wording of the comment, about autoconfiguration and fallback. And I'm definitely not the best man for that :)

@skrashevich
Copy link
Contributor Author

I think ffprobe should be used to get width / height when cv2 fails. also a couple other docs changes

At the moment, this PR does not affect the get_video_properties function

@skrashevich skrashevich force-pushed the 230615-feature-camera-autoconf branch from f82776f to 745a6d1 Compare June 22, 2023 23:11
@@ -961,8 +961,16 @@ def runtime_config(self, plus_api: PlusApi = None) -> FrigateConfig:
if "detect" in input.roles:
try:
stream_info = get_video_properties(input.path)
camera_config.detect.width = stream_info["width"]
camera_config.detect.height = stream_info["height"]
camera_config.detect.width = (
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's simplify this code and move this logic outside of the try / except and have the except clause set stream_info to {"width": 0, "height": 0}

that way the values are only set to the config in one place

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that I should manually generate an exception in get_video_properties if one of dimensions is zero?

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is what I am suggesting

                        try:
                            stream_info = get_video_properties(input.path)
                        except Exception:
                            stream_info = {"width": 0, "height": 0}

                        if stream_info["width"] == 0 or stream_info["height"] == 0:
                            logger.warn(
                                f"Error detecting stream resolution automatically for {input.path} Applying default values."
                            )

                        camera_config.detect.width = (
                           stream_info["width"]
                            if stream_info["width"] > 0
                            else DEFAULT_DETECT_DIMENSIONS["width"]
                        )
                        camera_config.detect.height = (
                            stream_info["height"]
                            if stream_info["height"] > 0
                            else DEFAULT_DETECT_DIMENSIONS["height"]
                        )

Comment on lines +962 to +980
stream_info = {"width": 0, "height": 0}
try:
stream_info = get_video_properties(input.path)
except Exception:
logger.warn(
f"Error detecting stream resolution automatically for {input.path} Applying default values."
)
stream_info = {"width": 0, "height": 0}

camera_config.detect.width = (
stream_info["width"]
if stream_info.get("width")
else DEFAULT_DETECT_DIMENSIONS["width"]
)
camera_config.detect.height = (
stream_info["height"]
if stream_info.get("height")
else DEFAULT_DETECT_DIMENSIONS["height"]
)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
stream_info = {"width": 0, "height": 0}
try:
stream_info = get_video_properties(input.path)
except Exception:
logger.warn(
f"Error detecting stream resolution automatically for {input.path} Applying default values."
)
stream_info = {"width": 0, "height": 0}
camera_config.detect.width = (
stream_info["width"]
if stream_info.get("width")
else DEFAULT_DETECT_DIMENSIONS["width"]
)
camera_config.detect.height = (
stream_info["height"]
if stream_info.get("height")
else DEFAULT_DETECT_DIMENSIONS["height"]
)
stream_info = {"width": DEFAULT_DETECT_DIMENSIONS["width"], "height": DEFAULT_DETECT_DIMENSIONS["height"]}
try:
stream_info = get_video_properties(input.path)
except Exception:
logger.warn(
f"Error detecting stream resolution automatically for {input.path} Applying default values."
)
camera_config.detect.width = stream_info["width"]
camera_config.detect.height = stream_info["height"]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you will get width and height values from previous loop iteration, if get_video_properties() fails

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something. I am initializing stream_info with the defaults before calling get_video_properties, so I don't see how the previous loop could carry over.

@blakeblackshear blakeblackshear merged commit ce3a544 into blakeblackshear:dev Jul 14, 2023
11 checks passed
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.

None yet

3 participants