-
Notifications
You must be signed in to change notification settings - Fork 153
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
Module video4linux #249
Module video4linux #249
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made some general comments if you could tell more about your specific issues I could give you some more specific comments
We are probably also going to want a bunch of |
Ok, I have corrected these problems. Thanks for your help! |
I recommend you look up about rust ownership and lifetime it is a key component of the language. Once you get your head around it you can start to see its advantages and also how to work around things like your current issue.
Have you tried just storing them as |
src/v4l/mod.rs
Outdated
.add_stream() | ||
.unwrap(); | ||
let main_camera = arc_cam.clone(); | ||
s.spawn(move |_| camera_loop(&*main_camera, Stream::Main, &mut outputs, true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove your &*
this will deference then reference the variable (then your compiler will deterrence it again because the type of the function sig is Device
not &Device
. Also try to give the stream
without an &
too. So that your structure can take ownership but I'm not sure how v4l.add_stream
works. Check the doc to see if it returns a reference or not. If it returns a reference we have to work out something different.
src/v4l/v4lt.rs
Outdated
|
||
pub(crate) struct V4ltOutputs<'a> { | ||
device: Device, | ||
stream: &'a mut MmapStream<'a>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First I think we need to remove the &mut
at the front. You seem to have ownership of it at this point.
However I suspect that there might be another issue in that the MmapStream
has a lifetime parameter. This usually means that there is a reference in it that points to data else where. The compiler needs to be certain that the lifetime of 'a
does not die. Which it cannot be certain of because of it running on a different thread. This will be an issue.
Have a look at the PR I made against your branch. 0ry0n#1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very interesting feature! Thank you for implementing it and thanks @QuantumEntangledAndy for the initial code review. I hadn't even considered doing something like this but I think it is a good idea.
A few points of feedback. I think the largest one is the code duplication in the wrapper module, and perhaps also (bikeshedding?) the name.
src/v4l/v4lt.rs
Outdated
// After we have created the device stream we cannot | ||
// edit the height/width etc | ||
// So first we pull packets from the camera until we have | ||
// enough data to setup the height etc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way you can query metadata without parsing the video itself? There might be a camera Xml you can fetch that will tell you what its current settings are. (I've seen something similar but I can't recall if it's available or active settings.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This data is also available in
<StreamInfoList>
xml (msg id: 146)<Compression>
xml (msg id: 56)- Initial login (msg id 1)
But this MediaInfo packet was the easiest as the StreamInfoList
and Compression
aren't currently programmed in
src/v4l/mod.rs
Outdated
@@ -0,0 +1,270 @@ | |||
/// | |||
/// # Neolink V4l |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file appears to be about 95% copy-paste from the default stream command. The camera drive and setup loops are the same. Can we break those out into a common module and pass a "sink" trait to that module? I think the Gstreamer module is already kinda structured that way, so it would be an easy change to the existing code.
src/v4l/mod.rs
Outdated
/// # Neolink V4l | ||
/// | ||
/// This module serves the v4l streams for the | ||
/// `neolink v4l` subcommand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I object slightly to v4l
as a name. Presumably Windows users would like something like this too and a hypothetical implementation using native Windows APIs would not be named v4l
. What's a good name for the feature, not the mechanism?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as a reference: Macos has CoreMediaIO and windows has DirectShow for virtual camera. Neither of which seem to have any high level crates, it's just sys level FFI stuff at the moment, so we'd have to roll it ourselves.
We could perhaps get similar functionality though with just a fifo exposing the current streams.
I do kinda agree that the name should be generic rather than v4l
(video 4 linux). Maybe virtual_camera
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something pithy. virtcam
or native
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
virtcam
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 virtcam
it is!
src/config.rs
Outdated
@@ -73,6 +79,26 @@ pub(crate) struct CameraConfig { | |||
#[validate(range(min = 0, max = 31, message = "Invalid channel", code = "channel_id"))] | |||
#[serde(default = "default_channel_id")] | |||
pub(crate) channel_id: u8, | |||
|
|||
#[cfg(target_os = "linux")] | |||
// Maximum number of devices allowed by v4l2loopback (8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like this limit is actually a hard limit, since it's possible to create additional devices:
Can you move this check to runtime, and fail with a nice error message if the user tries to create a device with a number that the kernel turns out not to like?
And one last thing, don't forget to add the command to the readme under "additional commands"! |
I thought about introducing a module for v4l support in order to send the video payload to a /dev/video* device created via v4l2loopback.
This way you would get the video stream without using the "heavy" gstreamer, useful when you need the video stream locally. Example: (raspberry: neolink -> NVR software).
Unfortunately I don't know rust :(. I tried to implement it trying to follow the structure of the rtsp module, but there is some problem.