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

Add FPS Props for Linux Cameras #487

Merged
merged 1 commit into from
May 16, 2023
Merged

Conversation

seanavery
Copy link
Contributor

@seanavery seanavery commented Mar 31, 2023

Description

This PR allows for retrieving and setting FPS properties on Linux cameras. Like FrameSize, Discrete and Stepwise FrameRate returns are supported.

A simple check is added in the fitness function to avoid adding fps comparisons on MacOS and Screen drivers.

Implementation depends on this blackjack/webcam PR.

Update: fps pr for blackjack/webcam has been merged!

Testing

Manual tests

Make sure there are no regressions across platforms (fitness distance is not affected).

  • linux webcam
  • mac webcam
  • linux screen

Go tests

  • Added unit tests for float and FrameRate matching in prop_test.go. Includes cases where FPS is not available.
  • Added unit test for calcFramerate in camera_linux_test.go.
  • No regressions in other prop.go and camera_linux tests.

@seanavery
Copy link
Contributor Author

@edaniels @bazile-clyde Tagging for review.

@bazile-clyde bazile-clyde self-requested a review May 2, 2023 21:12
Copy link
Member

@edaniels edaniels left a comment

Choose a reason for hiding this comment

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

LGTM but can you write a few tests for this to make sure fitness is still working well 🏋️ ?

@seanavery
Copy link
Contributor Author

@edaniels Added tests.

@seanavery seanavery requested a review from edaniels May 4, 2023 18:33
Copy link
Member

@edaniels edaniels left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for adding tests

@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Patch coverage: 19.14% and project coverage change: +0.19 🎉

Comparison is base (8748614) 58.83% compared to head (92b6f82) 59.02%.

❗ Current head 92b6f82 differs from pull request most recent head 4a1bdfe. Consider uploading reports for the commit 4a1bdfe to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #487      +/-   ##
==========================================
+ Coverage   58.83%   59.02%   +0.19%     
==========================================
  Files          63       63              
  Lines        3826     3854      +28     
==========================================
+ Hits         2251     2275      +24     
- Misses       1444     1446       +2     
- Partials      131      133       +2     
Impacted Files Coverage Δ
pkg/driver/camera/camera_linux.go 26.41% <11.62%> (-1.13%) ⬇️
pkg/prop/prop.go 75.83% <100.00%> (+1.83%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

if denominator == 0 {
return 0, errors.New("framerate denominator is zero")
}
// round to three decimal places to avoid floating point precision issues
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the comment here. Saved me a head scratch 😅

pkg/driver/camera/camera_linux.go Show resolved Hide resolved
pkg/prop/prop.go Outdated
// Note this also affect screen caputre as screen.Properties does not fill in the Framerate field.
// cmps.add(p.FrameRate, o.FrameRate)
// skip framerate if not requested by constraints and available in driver props
if p.FrameRate != nil && o.FrameRate > 0.0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

cmps.add already checks if p.FrameRate == nil and will do nothing if it is. I don't think that check is needed here too. It does not check if o.FrameRate is greater than 0.0 though. I think that should be handled by the compare method in the constraint interface itself like FrameFormatConstraint. That appears to be what the other constraints do.

Copy link
Contributor Author

@seanavery seanavery May 9, 2023

Choose a reason for hiding this comment

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

Great point - can safely remove constraint check.

@seanavery seanavery changed the title FPS Props for Linux Cameras Add FPS Props for Linux Cameras May 10, 2023
@seanavery seanavery changed the base branch from master to add-broadcast-test-conditions-with-pause May 16, 2023 16:22
@seanavery seanavery changed the base branch from add-broadcast-test-conditions-with-pause to master May 16, 2023 16:22
@seanavery
Copy link
Contributor Author

Cleaning up git history in preparation for merge.

@bazile-clyde bazile-clyde merged commit 138499b into pion:master May 16, 2023
7 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