Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Update webcam module to set the camera framerate #390

Merged
merged 5 commits into from
Apr 4, 2022
Merged

Update webcam module to set the camera framerate #390

merged 5 commits into from
Apr 4, 2022

Conversation

f-fl0
Copy link
Contributor

@f-fl0 f-fl0 commented Mar 30, 2022

Description

This PR updates the webcam module to ba064708e1652773bdb71e0605a1819ef4fad2e7 which allows to set (and get) the video camera capture framerate based on the FrameRate value specified in prop.Media.

@codecov
Copy link

codecov bot commented Mar 30, 2022 •
edited
Loading

Codecov Report

Merging #390 (a21eab0) into master (7026126) will decrease coverage by 0.04%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master     #390      +/-   ##
==========================================
- Coverage   47.16%   47.12%   -0.05%     
==========================================
  Files          67       67              
  Lines        4380     4386       +6     
==========================================
+ Hits         2066     2067       +1     
- Misses       2189     2194       +5     
  Partials      125      125
Impacted Files Coverage Δ
pkg/driver/camera/camera_linux.go 26.31% <0.00%> (-0.80%) ⬇️
pkg/prop/prop.go 72.10% <100.00%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7026126...a21eab0. Read the comment docs.

at-wat
at-wat reviewed Mar 30, 2022
View reviewed changes
pkg/driver/camera/camera_linux.go Outdated
@@ -179,6 +179,11 @@ func (c *camera) VideoRecord(p prop.Media) (video.Reader, error) {
return nil, err
}

err = c.cam.SetFramerate(float32(p.FrameRate))
Copy link
Member

Choose a reason for hiding this comment

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

How it behaves when user doesn't explicitly specify frame rate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I will check that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some tests. When the framerate is not specified the constraint seems to be ignored.

Copy link
Member

Choose a reason for hiding this comment

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

p.FrameRate seems to be 0 when user doesn't set frame rate.
I think it's better not to call SetFramerate in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in a21eab0

@f-fl0
Copy link
Contributor Author

f-fl0 commented Mar 30, 2022

15973f6961cbe56d44372d74f46abe88598265a1 failed in CI because of the following error:

=== RUN   TestThrottle
    throttle_test.go:36: Number of pushed images is expected to be 20, but pushed 27
--- FAIL: TestThrottle (0.54s)
FAIL

It seems irrelevant to the changes introduced in this PR.

@f-fl0 f-fl0 requested a review from at-wat March 30, 2022 08:51
at-wat
at-wat approved these changes Apr 4, 2022
View reviewed changes
Copy link
Member

@at-wat at-wat left a comment

Choose a reason for hiding this comment

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

LGTM

@at-wat at-wat merged commit 3b23160 into pion:master Apr 4, 2022
@f-fl0 f-fl0 deleted the set-camera-framerate branch April 4, 2022 06:12
@f-fl0 f-fl0 mentioned this pull request Apr 5, 2022
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@at-wat at-wat at-wat approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@f-fl0 @at-wat