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

Move webcam to plugin #4628

Merged
merged 82 commits into from
Jan 17, 2023
Merged

Move webcam to plugin #4628

merged 82 commits into from
Jan 17, 2023

Conversation

crysxd
Copy link
Contributor

@crysxd crysxd commented Aug 21, 2022

What does this PR do and why is it necessary?

This PR moves the webcam code into a plugin "Classic webcam" and allows other plugins to provide one or more webcams in the same way. This is a big PR and I expect much discussion, please don't put high priority on this :)

Configuration

Every webcam has a set of configurations as well as a template that will be loaded into the Control tab on the web interface. The webcam settings now allow the user to select a default webcam that is used for backwards compatibility as well as for timelapse creation.

The webcam configuration consists of three elements: Basic attributes, extras and compatibility settings. All values are optional.

Basic attributes are similar to what existed before and contain flip, rotate and the snapshot URL. A name and display name are added to identify the webcam. Extras are a dictionary with webcam specific data, e.g. WebRTC server or anything else needed for the frontend to display the webcam. Compatibility data are all fields that were removed from the old webcam configuration: Stream URL, cache buster, stream timeout, stream ratio, WebRTC server.

The compatibility data of the default webcam (if it provides any) is used to populate the old settings field for /api/settings. This allows old clients, e.g. the Cura plugin or apps, to keep functioning with no changes. In a future release this mapping might be removed, but it also allows webcam to offer a "common ground solution" that can be used by clients that don't want to support many different webcam solutions.

Timelapse

The timelapse code (Python and web) was updated to rely on the default webcam for taking the timelapse. It's also feasible to add a webcam setting to the timelapse configuration in the future.

Settings

The webcam section of /api/settings is now composed using global settings and the webcams provided by plugins. No fields were removed ensuring full backwards compatibility. The webcams and defaultWebcam fields were added. As described above, the values of the default webcam are used to populate the old fields like snapshotUrl and streamUrl. In a future release of OctoPrint this mapping can be removed

"webcam": {
    // Not changed
    "bitrate": "10000k", 
    "timelapseEnabled": true,
    "watermark": true,
    "webcamEnabled": true,
    "ffmpegVideoCodec": "libx264",
    "snapshotSslValidation": true,
    "ffmpegThreads": 1,
    "ffmpegCommandline": "{ffmpeg} -framerate {fps} -i \"{input}\" -vcodec {videocodec} -threads {threads} -b:v {bitrate} -f {containerformat} -y {filters} \"{output}\"",
    "ffmpegPath": "/opt/homebrew/bin/ffmpeg",
    "snapshotTimeout": 5,

    // Mapped from defaultWebcam (if value is provided there)
    "flipH": true,
    "flipV": false,
    "rotate90": false, 
    "snapshotUrl": "http://100.64.161.19/webcam/?action=snapshot", 

    // Mapped from defaultWebcam > compat (if value is provided there)
    "cacheBuster": false,
    "streamRatio": "16:9",
    "streamTimeout": 5, 
    "streamUrl": "http://100.64.161.19/webcam/?action=stream", 
    "streamWebrtcIceServers": "stun:stun.l.google.com:19302", 
    
    // Added
    "defaultWebcam": "classic",
    "webcams": [
      {
        "compat": {
          "cacheBuster": false, 
          "stream": "http://100.64.161.19/webcam/?action=stream", 
          "streamRatio": "16:9", 
          "streamTimeout": 5, 
          "streamWebrtcIceServers": "stun:stun.l.google.com:19302"
        }, 
        "displayName": "Classic Webcam", 
        "extras": {
          "cacheBuster": false, 
          "stream": "http://100.64.161.19/webcam/?action=stream", 
          "streamRatio": "16:9", 
          "streamTimeout": 5, 
          "streamWebrtcIceServers": "stun:stun.l.google.com:19302"
        }, 
        "flipH": true, 
        "flipV": false, 
        "name": "classic", 
        "provider": "classicwebcam", 
        "rotate90": false, 
        "snapshot": "http://100.64.161.19/webcam/?action=snapshot"
      }, 
      {
        "compat": null, 
        "displayName": "Dummy Webcam", 
        "extras": null, 
        "flipH": false, 
        "flipV": false, 
        "name": "dummy", 
        "provider": "classicwebcam", 
        "rotate90": false, 
        "snapshot": null
      }
    ]
  }
Classic Webcam plugin

The webcam implementation from OctoPrint was moved into the classicwebcam plugin (name up for debate). This caused a lot of code additions and removals, but basically code from the Control tab and settings got moved into the plugin. The plugin migrates the old settings into the new format by moving data from the global settings space into the plugin's settings.

The logic to unload the webcam stream was changed to use the intersection API. This allows the webcam to be started and stopped when the user changes the selected webcam.

How was it tested? How can it be tested by the reviewer?

Tested on my local test instance. To test, simply check out the branch. All data is migrated automatically.

Any background context you want to provide?

This is a preparation in providing a standalone WebRTC plugin. As WebRTC is highly diverse, this setup allows different WebRTC plugins for different servers or even completely different streaming technologies in the future. It also allows plugins like multicam to show their webcams easily in the control tab.

What are the relevant tickets if any?

/

Screenshots (if appropriate)

Screen.Recording.2022-08-21.at.11.43.36.mov

Further notes

Advice required: I had to use the force=True flag when migrating the settings, I'm not sure why and if there are any implications. See classicwebcam/__init__.py line 95 and below.

Open Tasks (from discussion below)

  • Force=true should be removed. I'll dig a bit to see why it's needed
  • Unify loading/unloading of webcam
  • Snapshot function (and adding setting to timelapse config?)
  • Convert settings from list to map
  • Check impact on existing plugins
  • Fix comments from 6.
  • Move WebcamSubWizard to plugin
  • Remove mock webcam after review

@github-actions github-actions bot added the targets maintenance The PR targets the maintenance branch label Aug 21, 2022
@foosel
Copy link
Member

foosel commented Nov 21, 2022

Ok, see 077702b (also did a small bugfix in 673a986). And of course I managed to break the tests, so there's also fa0fd53 🙈

A couple of TODOs from that refactoring:

  • Currently the settings overlay is only created once during platform initialization. We should probably reinit it when the default webcam changes during runtime (which means the deprecation code also needs a bit of work still, as right now we can't recognize overlays for deprecated settings path and thus can't remove and possibly re-add the paths). Alternatively a change of the default webcam would require a server restart. Do we want that? Or do we maybe just say we want that for now and tackle that at a later date? It's possible, it's just some more adjustments on the overlay stuff (which I already got an idea about) and we need some fancy way to react to a change on the default webcam that doesn't mean pulling all the specifics right back into the settings layer. Maybe some "this path changed" callback feature in the settings is in order.
  • The split between snapshot and stream responsibilities vs the compat layer still makes me a bit 🤔. Right now there's one overlay that covers both the settings. But maybe that needs to be split between both roles. Would require a second overlay. Not an issue though.

Overall I think this is a cleaner approach, since it doesn't throw a bunch of webcam specifics into the plugin settings (or the normal settings for the matter) but keeps it contained in ONE spot during platform init and in a settings overlay. Deprecation handling is also done in a non-webcam-specific way, and longterm this option in the settings might also help with future deprecations. And there's way less compat.foo if compat else bar stuff and instead we just utilize the information we already have from the compat config.

Webcam, WebcamCompatibility and RatioEnum have
nothing to do with the config anymore but are
rather WebcamProviderPlugin specific, so let's move
them into their own octoprint.schema.webcam package.
@foosel
Copy link
Member

foosel commented Nov 21, 2022

Ok, also couldn't resist to do 823d214 because I really couldn't see why something that isn't part of the config format anymore (or ever was) was pushed into octoprint.schema.config, so now it's in octoprint.schema.webcam ^^

@foosel foosel added this to the 1.9.0 milestone Nov 21, 2022
@crysxd
Copy link
Contributor Author

crysxd commented Nov 22, 2022

The webcam is split now and I also added a small hint that the settings of this webcam are in the plugin's settings. I also found that there is no change needed to the settings overlay (much much nicer btw!) as the snapshotUrl is part of the compat settings and already migrated.

Screenshot 2022-11-22 at 08 04 28

@crysxd
Copy link
Contributor Author

crysxd commented Nov 22, 2022

From my understanding the last open topic is this one: #4628 (comment)

What shall we do there?

@foosel
Copy link
Member

foosel commented Nov 22, 2022

Replied in the thread.

@crysxd
Copy link
Contributor Author

crysxd commented Nov 24, 2022

Done :) Resolved the thread above. From my side, this would be it for now. I will test everything again on Saturday just to make sure nothing broke while addressing the change requests.

@foosel
Copy link
Member

foosel commented Dec 7, 2022

FYI, still recovering from a nasty sinusitis. 1.9.0rc1 is off the table for 2022 because of that. I'll see if I can still get around to give this a final review (and hopefully merge it) before leaving the office for the year, but since I also have to prep some stuff for the 10 year anniversary show and take care of some other stuff that needs to be done this month, let alone am not 100% fit still, I can't promise it. Rest assured though it's scheduled for 1.9.0rc1 and pretty much number 1 on the deadline-less part of the Todo list.

@crysxd
Copy link
Contributor Author

crysxd commented Dec 7, 2022

No stress! I'll be still around next year :) Enjoy the holidays and make sure to recover!

If anyone is trying to set stuff from the webcam
compat layer, we now get a log entry, or multiple
if needed.

Long term we might want to return an error on
the API
Also use API methods of plugin manager instead of
accessing internal control properties directly.
@foosel
Copy link
Member

foosel commented Jan 17, 2023

I did some final touch-ups, but I think now this can be merged.

Since we will no longer support configuring things like the stream URL and similar on the REST API, I've added some logging to the API endpoint when incoming settings data contains affected settings. That way there will be at least a paper trail as to why the settings aren't being written. We can remove it sometime down the road, or just leave it in.

In any case, this change in behaviour will require a heads-up in the release notes for REST API consumers/users. I don't know if there are any third party clients out there that offer configuration of webcam settings, or plugins that for some reason do that through the settings endpoint, but we should give them a warning just in case.

@foosel foosel added the ✋ heads-up Issue requires a heads-up in the change log label Jan 17, 2023
@foosel foosel merged commit 58086a2 into OctoPrint:maintenance Jan 17, 2023
@foosel foosel added the improvement Improving functionality, behaviour, UX, ... label Jan 17, 2023
@crysxd
Copy link
Contributor Author

crysxd commented Jan 17, 2023

Oh wow! Thanks for your amazing help on this one! :)

I think the most important ones are the plugins interacting with the camera in some form and I guess apps. Technically consuming the webcam should continue to work as usual but configuration does change.

MikeRatcliffe added a commit to MikeRatcliffe/OctoPrint-FullScreen that referenced this pull request May 29, 2023
Since OctoPrint/OctoPrint#4628 we need to use `#classicwebcam_container` otherwise all controls and text information provided by this plugin is `display:none`.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 18, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
approved Issue has been approved by the bot or manually for further processing ✋ heads-up Issue requires a heads-up in the change log improvement Improving functionality, behaviour, UX, ... targets maintenance The PR targets the maintenance branch
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants