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

Fix WebcamProviderPlugin.take_webcam_snapshot usage in timelapse #5014

Open
foosel opened this issue May 13, 2024 · 0 comments
Open

Fix WebcamProviderPlugin.take_webcam_snapshot usage in timelapse #5014

foosel opened this issue May 13, 2024 · 0 comments
Labels
approved Issue has been approved by the bot or manually for further processing bug Issue describes a bug done Done but not yet released ✋ heads-up Issue requires a heads-up in the change log
Milestone

Comments

@foosel
Copy link
Member

foosel commented May 13, 2024

Problem

The timelapse code currently calls that with the full webcam object:

self._logger.debug(
f"Going to capture {filename} from {self._webcam.config.name} provided by {self._webcam.providerIdentifier}"
)
snapshot = self._webcam.providerPlugin.take_webcam_snapshot(self._webcam)

It should however be using the webcam name only.

See also this discussion on Discord:

bchanudet — 05/09/2024 11:39 PM
Hey there, I don't know if that's the right channel but I was wondering about the real prototype of the take_webcam_snapshot() of the WebcamProviderPlugin class. The docs say the first parameter webcamName should be a string, but in timelapse.py it sends the whole ProvidedWebcam instance instead.

I haven't found any other place to confirm which it is really:

  • the classic Webcam plugin doesn't use the parameter
  • in tornado.py, it seems that it sends the webcam name as the first parameter, so a string?

bchanudet — 05/09/2024 11:46 PM
I'm asking because in OctoRant I send a string, but MultiCam is expecting a ProvidedWebcam instance, thus our two plugins cannot work together for now. But I don't know if I should fix it, or if MultiCam should fix it. I haven't yet found an other plugin that use the WebcamProviderPlugin Mixin to collect more evidence towards one or the other prototype.

foosel — Today at 11:14 AM
string is correct, the implementation in timelapse.py is wrong, that shoudl have been webcam.name basically. The idea behind that IIRC was that a webcam provider plugin may provide multiple webcams, so this parameter allows to tell it which webcam it should take the snapshot from. [...]

Solution

Fix timelapse implementation. But since some third party implementation seem to have used that as a "do it like this" approach, also add a big heads-up about this for plugin authors.

@foosel foosel added bug Issue describes a bug approved Issue has been approved by the bot or manually for further processing labels May 13, 2024
@foosel foosel added this to the 1.11.0 milestone May 13, 2024
@foosel foosel added the ✋ heads-up Issue requires a heads-up in the change log label May 16, 2024
foosel added a commit that referenced this issue May 16, 2024
We were using the full webcam object instead of just its name as
documented.

Closes #5014
@foosel foosel added the done Done but not yet released label May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Issue has been approved by the bot or manually for further processing bug Issue describes a bug done Done but not yet released ✋ heads-up Issue requires a heads-up in the change log
Projects
Status: Done
Development

No branches or pull requests

1 participant