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

[rfc/wip] split device #359

Closed
wants to merge 25 commits into from

Conversation

vicamo
Copy link
Contributor

@vicamo vicamo commented Oct 15, 2020

Here is my initial attempt to try implementing device split. Still in the very early stage, but I'd like to have your opinions before jumping too far or in the wrong direction.

The main idea/goal is to:

  1. remove single device code completely.

    Single device is the root of all evil so far, and there probably has no reason to do this, especially switching device capabilities at runtime. This can, probably has to, be done after the transition. Module parameter exclusive_caps will be removed; assign identical device number to output and capture devices simultaneously becomes illegal.

  2. keep the ability to instantiate multiple devices through module parameters.

    Currently module parameter output_nr is added for this purpose.

  3. rewrite buffer management with video videobuf2.

  4. embed video_device in v4l2_loopback_device

    This can simplify many struct pointer resolutions and memory leaks.

@vicamo vicamo marked this pull request as draft October 15, 2020 17:23
@umlaeute umlaeute linked an issue Oct 15, 2020 that may be closed by this pull request
@vicamo vicamo force-pushed the for-upstream/split-device branch 3 times, most recently from 42b9346 to 2da512a Compare October 19, 2020 08:06
@umlaeute
Copy link
Owner

  • keep the ability to instantiate multiple devices through module parameters. (output_nr)

ACK

  • rewrite buffer management with video videobuf2.

fully ACK ;-)

  • embed video_device in v4l2_loopback_device

I don't care

  • remove single device code completely.

ah well.
let's put it like this:

  • if removing single-device code completely simplifies the code a lot and makes implementation just a lot easier, go for it.
  • if otoh, allowing single-device could be accomplished with just a few more (output_dev != capture_dev) lines i would rather keep it.

Single device is the root of all evil so far, and there probably has no reason to do this,
especially switching device capabilities at runtime.

not sure i understand this

This can, probably has to, be done after the transition.

not sure i know what "this" and "the transition" refer to right now (if "this" is "getting rid single-device support" and the "transition" is "dynamic device management" then you are probably right.
(but then: i think "dynamic device management" is ready to be rolled out; so this transition is basically done)

Module parameter exclusive_caps will be removed;

in all cases: ACK
even if we still support single-device at the end of the day, the exclusive_caps should go away
(in the case of split-devices it is not needed; in the case of single-device it can always be set to false)

assign identical device number to output and capture devices simultaneously becomes illegal.

obviously this is (only) an implication of disallowing single-device.

so: go ahead ;-)

@vicamo vicamo force-pushed the for-upstream/split-device branch 2 times, most recently from c8db769 to 014227c Compare October 23, 2020 16:24
@janakelarsson
Copy link

Is this still WIP or has it stalled?

@vicamo
Copy link
Contributor Author

vicamo commented Jan 15, 2021

Is this still WIP or has it stalled?

Hi, sorry, mostly stalled as we focus on the stable release 0.12.5 to implement camera relay.

@janakelarsson
Copy link

I would contribute but the effort to grok existing code is probably prohibitive.

I am looking forward to not having to restart Zoom each time I pull out the green screen.

@umlaeute
Copy link
Owner

I would contribute but the effort to grok existing code is probably prohibitive.

cool, go ahead ;-)

I am looking forward to not having to restart Zoom each time I pull out the green screen.

however, here zoom works without split-devices; so i wonder why you think you need it.

@janakelarsson
Copy link

I don't have OBS Studio Virtual Cam running all the time, it eats CPU.

Zoom does not detect a newly started Virtual Cam, so I need to restart Zoom. Teams too. After restart they do find the Virtual Cam.

@janakelarsson
Copy link

I looked at the code. Besides one obvious place (line 2307) where "device->" should be "dev->" I cannot help much, apologies.

@wt
Copy link
Contributor

wt commented Mar 2, 2021

What is the point of split devices? Is it so that a different device node is used for the capture dev and the output dev?

@umlaeute
Copy link
Owner

umlaeute commented Mar 2, 2021

@wt yes exactly. see also #322

@vicamo
Copy link
Contributor Author

vicamo commented Mar 16, 2021

Update a few more works on this. Rebased to current devel branch HEAD and picked some useful commits from https://github.com/umlaeute/v4l2loopback/tree/feature/split-device. I have no intention to support any form of joined device as that's just impossible for vb2 framework, and it has bring all kinds problems/limitations to me. If that's unacceptable, then maybe you'd like to close this PR directly.

It's also based on the first commit of #414 that updates clang-formated source, and I think I'll need #415 to correctly handle frame sizes/intervals queries.

@vicamo
Copy link
Contributor Author

vicamo commented Mar 17, 2021

When either vfh->ctrl_handler or vfd->ctrl_handler is non-null, it will
shadow vidioc_queryctl.

Signed-off-by: You-Sheng Yang <[email protected]>
Fix incorrect format introduced in umlaeute#416 commit a572a4f ("report video-devices
allocated at module-load time via sysfs").

Signed-off-by: You-Sheng Yang <[email protected]>
Converting function v4l2loopback_cd2dev and v4l2loopback_getdevice
searches v4l2loopback_index_idr with device number retrieved from
video_device drvdata, but this could be time consuming and creates
unecessary complexity. And since we may need to retrieve
v4l2_loopback_device address from callbacks shared between capture and
output devices, this patch reuses video_device drvdata instead of struct
member offsets.

Signed-off-by: You-Sheng Yang <[email protected]>
Since we have already instantiated an output video device, use its `num`
field instead.

Signed-off-by: You-Sheng Yang <[email protected]>
These ioctls are only called as output role. Omit output/capture checks.

Signed-off-by: You-Sheng Yang <[email protected]>
@vicamo
Copy link
Contributor Author

vicamo commented Mar 19, 2021

  • Rebased on For upstream/devel fixes #418
  • Remove exclusive_caps/announce_all_caps code
  • Pick fixes from feature/split-device branch
  • Return consistent data from enum_framesizes/enum_frameintervals as doc says so
  • Remove vidioc_s_fmt_overlay/vidioc_g_fmt_overlay/vidioc_querystd from output ioctl; they're capture-only,

According to V4L2 documentation:

  Applications can assume that the enumeration data does not change
  without any interaction from the application itself. This means that
  the enumeration data is consistent if the application does not
  perform any other ioctl calls while it runs the frame size
  enumeration.

This change always returns all supported formats.

Signed-off-by: You-Sheng Yang <[email protected]>
Signed-off-by: You-Sheng Yang <[email protected]>
Signed-off-by: You-Sheng Yang <[email protected]>
@umlaeute
Copy link
Owner

* Rebased on #418

your commit style is great (i wish everybody would commit like this).

however, if possible please don't rebase: instead use merge and cherry-pick.
it makes it much easier for me to review your changes.

@vicamo
Copy link
Contributor Author

vicamo commented Mar 23, 2021

* Rebased on #418

your commit style is great (i wish everybody would commit like this).

however, if possible please don't rebase: instead use merge and cherry-pick.
it makes it much easier for me to review your changes.

Hi, sorry for taking your extra time. Like what you have seen here, I'm picky at my own commits and always want to make them compact and self-explaining as possible. However this is still not a complete PR, therefore I mark it as WIP/RFC, and it has many aspects to be polished, re-organized, even re-written. So applies to the git commits, they just cannot meet my own expect yet, and many more works remain to be done at odd moments. I think I will do one more rebase when it's full ready for your review. Thank you.

@alesya-h
Copy link

@vicamo do you think this work will ever be complete? It blocks the release (see #398), it's been over a year since last comment on this thread, and it's been almost 5 months since the last commit in wip/split-device branch in your repo, so I assume development is kinda halted.

@vicamo
Copy link
Contributor Author

vicamo commented May 22, 2022

@vicamo do you think this work will ever be complete? It blocks the release (see #398), it's been over a year since last comment on this thread, and it's been almost 5 months since the last commit in wip/split-device branch in your repo, so I assume development is kinda halted.

Yes, it's mostly halted as I'm more or less looking forward to pipewire/libcamera to serve as a similar role.

@normanr
Copy link

normanr commented Mar 13, 2023

fwiw, it looks like https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/707 tracks the work for pipewire. It sounds like userspace is ~mostly working, but requires applications to update to support it.

@wt
Copy link
Contributor

wt commented Mar 14, 2023 via email

@umlaeute umlaeute modified the milestones: release 0.13, release 0.14 May 3, 2023
@vicamo vicamo closed this Sep 9, 2024
@vicamo vicamo deleted the for-upstream/split-device branch September 9, 2024 15:20
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.

Add support for split output/capture devices.
6 participants