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

Expose device list update func #463

Merged
merged 9 commits into from
Jan 14, 2023
Merged

Conversation

bazile-clyde
Copy link
Collaborator

Description

This PR exports the logic executed at startup to find and register cameras. This will allow us to discover cameras connected after our application has started.

@at-wat
Copy link
Member

at-wat commented Jan 10, 2023

Do you think we can expose setup function in other drivers as well?
It would be better to have unified interface to update device list.

@codecov
Copy link

codecov bot commented Jan 10, 2023

Codecov Report

Base: 58.16% // Head: 58.21% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (b4118d2) compared to base (6ac1424).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #463      +/-   ##
==========================================
+ Coverage   58.16%   58.21%   +0.04%     
==========================================
  Files          62       62              
  Lines        3679     3683       +4     
==========================================
+ Hits         2140     2144       +4     
  Misses       1412     1412              
  Partials      127      127              
Impacted Files Coverage Δ
pkg/driver/camera/camera_linux.go 30.43% <100.00%> (+0.76%) ⬆️
pkg/driver/screen/x11_linux.go 12.69% <100.00%> (+2.86%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@bazile-clyde
Copy link
Collaborator Author

Do you think we can expose setup function in other drivers as well?
It would be better to have unified interface to update device list.

@at-wat Done! I skipped the dummy.go files in audiotest and videotest. I also skipped vncdriver.go since there's no func init. Let me know if I should not have skipped any of those.

@at-wat
Copy link
Member

at-wat commented Jan 12, 2023

@bazile-clyde thanks!

I think it would be better in future to have another API that mediadevices.MediaDevices can automatically call them to update the device list.
For this, Initialize functions would be better to have godoc comments to mention this API is experimental.
What do you think?

@bazile-clyde
Copy link
Collaborator Author

bazile-clyde commented Jan 12, 2023

@bazile-clyde thanks!

I think it would be better in future to have another API that mediadevices.MediaDevices can automatically call them to update the device list. For this, Initialize functions would be better to have godoc comments to mention this API is experimental. What do you think?

That makes sense. I don't have strong opinions either way so I added it. Thanks for the quick turn-around btw!

@at-wat at-wat changed the title expose inital setup Expose device list update func Jan 13, 2023
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

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.

Could you revert import block orders to keep them like

import (
  "stdlib1"
  "stdlib2"

  "nonstdlib1"
  "nonstdlib2"
)

@bazile-clyde
Copy link
Collaborator Author

Could you revert import block orders to keep them like

import (
  "stdlib1"
  "stdlib2"

  "nonstdlib1"
  "nonstdlib2"
)

Done!

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 85597da into pion:master Jan 14, 2023
@bazile-clyde bazile-clyde deleted the export-discover branch January 17, 2023 15:43
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

2 participants