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 virtual camera can only be started once on Linux #7078

Conversation

shoffmeister
Copy link
Contributor

@shoffmeister shoffmeister commented Aug 15, 2022

Description

Fix #4808 by applying insights found at floe/backscrub#133 (comment)

At the same time, apply more logging and improve error handling to said module such that any future challenging behaviour can be identified more easily.

How Has This Been Tested?

  • Build OBS from git, install as portable, on Fedora 36
  • Start OBS, load scene collection
  • Click "Start Virtual Camera"
  • Click "Stop Virtual Camera"

--> works; now demonstrating that this fixes the reported problem, do

  • Click "Start Virtual Camera"
  • Click "Stop Virtual Camera"

... and this now works.

Video does work, too - tested with Firefox 103 and the Slack (https://slack.com/) web interface, device configuration -> OBS.

Types of changes

Fixes #4808

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@shoffmeister
Copy link
Contributor Author

There are reports that some fixes for the underlying challenge in the v4l2loopback module exist, and that those fixes have been applied in various repository, possibly in some distributions.

As I am writing this, no changes can be found upstream (https://github.com/umlaeute/v4l2loopback), and neither are they applied to Fedora 36 (which is in fact running upstream).

@tytan652
Copy link
Collaborator

tytan652 commented Aug 15, 2022

All commit messages are properly formatted and commits squashed where appropriate.

You commits are not properly formatted, check the contributing document link in the description.

And don't close this PR to open a new one, just amend your commits and forced push.
Edit: We had this many times.

@tytan652
Copy link
Collaborator

tytan652 commented Aug 15, 2022

For the code formatting, you'll need clang-format 13 and at least the format-code.sh script in the CI folder to format your code.

Edit: Stupid written 14, it's thirteen

@shoffmeister
Copy link
Contributor Author

@tytan652 - before I invest more time into this, I'd like to receive an indication whether this is wanted.

From Discord I understand that you do not want to see this merged, hence I am rather hesitant to spend any more time on this.

@tytan652
Copy link
Collaborator

If you have the bug with the latest version of the module it change everything on how I see it.

And I just gave my point of view, I'm not the one in charge for what is merged or not.

@kkartaltepe
Copy link
Collaborator

This was already resolved upstream with umlaeute/v4l2loopback#477

@shoffmeister
Copy link
Contributor Author

My local system is Fedora 36, it is up-to-date, and I have this package installed:

rpm -qa v4l2loopback
v4l2loopback-0.12.7-1.fc36.noarch

This package has been built via https://koji.rpmfusion.org/koji/buildinfo?buildID=23032 which points to https://pkgs.rpmfusion.org/cgit/free/v4l2loopback.git/

Version 0.12.7 is also what is published on https://github.com/umlaeute/v4l2loopback/tags as the most current release.

My running driver (debug=99) logs as

[33875.719261] /tmp/akmodsbuild.xR0XYRW0/BUILD/v4l2loopback-kmod-0.12.7/_kmod_build_5.18.17-200.fc36.x86_64/v4l2loopback.c:1943[v4l2_loopback_open]

This specific version is supposed to contain plenty of fixes, including the linked PR above.

I can assure you - on my system, as it is, OBS 27.2.4 (obs-studio-27.2.4-4.fc36.x86_64) has "can only be started once" behaviour.

I had a very quick gander at trying to fix OBS 27.2.4, but ran into known problems there (#5622) which simply did not appear for when working with OBS git. So, for the sake of simplicity and my own sanity, I worked on OBS git, and this pull request allows me to start/stop the OBS git virtual camera any number of times, thanks to the second commit in this PR.

(Plus, as it happens, it is much more helpful in debug and warning logs, which is the first commit of this PR).

@kkartaltepe
Copy link
Collaborator

This specific version is supposed to contain plenty of fixes, including the linked PR above.

Im afraid im not sure I follow, given the information provided i reach the opposite conclusion.

@shoffmeister
Copy link
Contributor Author

shoffmeister commented Aug 16, 2022

My local testing with a very much up-to-date Fedora 36

  • uname -a == Linux fedora 5.18.17-200.fc36.x86_64 #1 SMP PREEMPT_DYNAMIC
  • rpm -qa v4l2loopback == v4l2loopback-0.12.7-1.fc36.noarch

, where v4l2loopback-0.12.7-1.fc36.noarch would include the changes referred to, suggests that

  • OBS git master == virtual camera can only be started once; unwanted behaviour
  • this PR == virtual camera can be started and stopped an arbitrary number of times

The key material difference in this PR is 07fc858; the other commit is diagnostics, maintainability and (removal of) resource leakage that I applied in my quest to track down the problem.

I have not root caused the unwanted behaviour of OBS git master to a specific implementation detail in v4l2loopback-0.12.7-1.fc36.noarch (or https://github.com/umlaeute/v4l2loopback/releases/tag/v0.12.7)

@shoffmeister
Copy link
Contributor Author

shoffmeister commented Aug 16, 2022

For the record, I have now tested consumption of video through the OBS virtual camera on

  • Fedora 36 (Linux fedora 5.18.17-200.fc36.x86_64 #1 SMP PREEMPT_DYNAMIC)
  • KDE -> Wayland

for the following applications:

  • Microsoft Teams "1.5.00.10453 (64-bit)" off the Microsoft official repository (video appears mirrored in preview; do note that Microsoft Teams apparently only is happy with 720p output resolutions; 1080p does not work, and reportedly has never worked)
  • Chromium (chromium-freeworld) 104.0.5112.79 (Discord, Slack web site) off rpmfusion.org repositories
  • Firefox (103.0.2) (Slack web site) off the main Fedora repositories

All applications appeared to exhibit no regression relative to OBS git master.

I have not tested X11, but would not expect any regression there, either - after all, this PR only changes the way OBS talks to v4l2loopback and enables output streaming there; input sourcing is where X11 and Wayland are different.

@kkartaltepe
Copy link
Collaborator

Sorry I think we are speaking past each other. As I said there is a commit in upstream with the fix, and as you said you are on the latest release. But if you look at the repository you will see that no released tag includes the fix mentioned and this is confirmed looking at the commit log.

Of course you could go around and convince every tool using v4l2 to update with some patches and hope that they trickle down to you soon enough. But I think you would agree the best solution for everyone would be to ask for upstream to release a version with the fix, maybe next best would be to ask for your package manager to include a backport, and failing all else you can ask every tool you use to include a patch. It doesnt seem like either the first two have been tried yet so that is what im trying to suggest.

@shoffmeister
Copy link
Contributor Author

Upstream formally releasing a fix is optimal. The next step then is that all these upstream changes need to trickle down into distributions, and from distributions onto user systems.

I tend to recollect reading that the problem was introduced around version 0.12.5 of v4l2loopback, so roughly around April 2020. Any distribution using a plain release version of v4l2loopback starting with 0.12.5 is broken, to this date. (Some distros actively patching v4l2loopback is a different story)

So, for example, right now Arch Linux will have been broken, and continues to be broken, since April 2020 - at least when looking at https://archlinux.org/packages/community/any/v4l2loopback-dkms/

It is indeed a decision for the OBS project to make whether they want to carry the stream-on/off change in talking to v4l2loopback.

(IMHO) Semantically this change (07fc858) even makes sense (beware: I have barely any experience with v4l2). From https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-streamon.html?highlight=vidioc_streamon:

The VIDIOC_STREAMON and VIDIOC_STREAMOFF ioctl start and stop the capture or output process during streaming (memory mapping, user pointer or DMABUF) I/O.

The v4l2loopback module does not offer any particular API documentation of its own. The next best source, the V4L2 generic documentation has this example of an application driving a video grabber device: https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/v4l2grab.c.html?highlight=vidioc_streamon. This example uses stream on/off (see highlighting). In the OBS context, the v4l2loopback module acts in a role of a video grabber device.

@WizardCM WizardCM added Bug Fix Non-breaking change which fixes an issue Linux Affects Linux labels Aug 20, 2022
@shoffmeister shoffmeister force-pushed the 4808_virtual-camera-can-only-be-started-once branch from 07fc858 to 4196abb Compare September 5, 2022 12:22
@kkartaltepe
Copy link
Collaborator

kkartaltepe commented Oct 15, 2022

Can you leave the debug information in the 2nd commit to #7395 and just leave 4196abb here and we can try and merge this for 28.1 (also fixup the format complaints)

@jp9000
Copy link
Member

jp9000 commented Oct 15, 2022

Hi there, make sure not to check the I have read the contributing document checkbox if you haven't actually read the contributing document. I unchecked it for you.

I also unchecked the All commit messages are properly formatted and commits squashed where appropriate checkbox because the commit messages are not properly formatted.

@shoffmeister shoffmeister force-pushed the 4808_virtual-camera-can-only-be-started-once branch from 4196abb to b93ae85 Compare October 16, 2022 18:01
@norihiro
Copy link
Contributor

norihiro commented Oct 17, 2022

I have two suggestions for your change.

  • Code format need to be fixed. Please run command below.
clang-format -i plugins/linux-v4l2/v4l2-output.c
  • Is it necessary to check the error by ioctl(...) == -1? Other lines with ioctl have ioctl(...) < 0 so that I feel the error check condition should be the same as other lines so that it is easier for other people to read the code.

@kkartaltepe kkartaltepe force-pushed the 4808_virtual-camera-can-only-be-started-once branch from b93ae85 to 15ac97a Compare October 20, 2022 02:17
This implements a suggestion made in
floe/backscrub#133 (comment) to
address what appears to be a problem in the v4l2loopback driver.
And better aligns OBS' usage to the v4l2 kernel interface.

fixes obsproject#4808
Co-authored-by: Norihiro Kamae <norihiro@nagater.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue Linux Affects Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Virtual Camera will start one time and will not restart until reboot
7 participants