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

Webrtc: fix RTCSessionDescription #4870

Draft
wants to merge 1 commit into
base: maintenance
Choose a base branch
from

Conversation

havalli
Copy link

@havalli havalli commented Aug 11, 2023

  • Your changes are not possible to do through a plugin and relevant
    to a large audience (ideally all users of OctoPrint)
  • If your changes are large or otherwise disruptive: You have
    made sure your changes don't interfere with current development by
    talking it through with the maintainers, e.g. through a
    Brainstorming ticket
  • Your PR targets OctoPrint's devel branch if it's a completely
    new feature, or maintenance if it's a bug fix or improvement of
    existing functionality for the current stable version (no PRs
    against master or anything else please)
  • Your PR was opened from a custom branch on your repository
    (no PRs from your version of master, maintenance, or devel
    please), e.g. dev/my_new_feature or fix/my_bugfix
  • Your PR only contains relevant changes: no unrelated files,
    no dead code, ideally only one commit - rebase and squash your PR
    if necessary!
  • Your changes follow the existing coding style
  • If your changes include style sheets: You have modified the
    .less source files, not the .css files (those are generated
    with lessc)
  • You have tested your changes (please state how!) - ideally you
    have added unit tests
  • You have run the existing unit tests against your changes and
    nothing broke
  • You have added yourself to the AUTHORS.md file :)

What does this PR do and why is it necessary?

Fixed the connection to the webrtc server.

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

Tested with a mediamtx Webrtc server
Webcam url: "webrtcs://webrtc-server-host/cam/whep"

Any background context you want to provide?

What are the relevant tickets if any?

Screenshots (if appropriate)

image

Further notes

@github-actions github-actions bot added targets maintenance The PR targets the maintenance branch approved Issue has been approved by the bot or manually for further processing labels Aug 11, 2023
@cp2004
Copy link
Member

cp2004 commented Aug 11, 2023

The (incomplete) WebRTC implementation in OctoPrint is only compatible with the aiortc server it was developed for (by another contributor). This change will probably break that and any similar servers that communicate the same way.

Because there is no standard to the WebRTC offer communication, these should use a Webcam Plugin introduced in 1.9.0, which allows you to customise the connection to whichever server you are using. I've written one for camera-streamer.

@havalli
Copy link
Author

havalli commented Aug 11, 2023

There is a standard for the WebRTC offer information. Whep. Yes, it is still a draft. But it fits this scenario exactly. (Just consuming a video stream)
My changes follow this standard. Wouldn't it make sense to put the existing logic in an "aiortc" webcam plugin and implement the Whep standard in the Classic Webcam?

@foosel
Copy link
Member

foosel commented Aug 29, 2023

Are these changes all that is needed to make it Whep compliant? If so I would suggest to add a config option to the plugin, something like negotiate_flavor, and depending on whether this is set to aiortc or whep do one or the other thing.

@havalli
Copy link
Author

havalli commented Aug 31, 2023

In my setup, it works. I think it's more of a starting point. Whep is also still a draft. I like the configuration option, but my Python skills are very bad. Maybe someone else can implement it.

@foosel
Copy link
Member

foosel commented Nov 22, 2023

Converting this to draft for now, as we cannot merge it as is. Requires a negotiate_flavor configuration option from the current point of view, in order to not break compatibility with existing aiortc based setups.

@foosel foosel marked this pull request as draft November 22, 2023 16:29
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 targets maintenance The PR targets the maintenance branch
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

None yet

3 participants