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

Implement AsRawRef for Decoder and Encoder to allow usage in ioctl #43

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sacovo
Copy link

@sacovo sacovo commented Jan 8, 2025

In order to use the v4l2 controls like force_key_frame, video_bitrate, ..., the fd of the opened device is needed. Since the device is a private field of Encoder and Decoder AsRawFd needs to be implemented using the internal device.

This PR implements AsRawFd for Encoder and Decoder by calling the same method on the underlying Device.

@Gnurou
Copy link
Owner

Gnurou commented Jan 9, 2025

Hi! Thanks for the PR. (nit: do you mean AsRawFd instead of AsRawRef?)

Could you elaborate a bit on how you plan to use this new impl? The decoder and encoder devices are high-level abstractions, so exposing the device FD too easily is something I tried to avoid with them. I wonder if we cannot do what you need through additional dedicated methods for setting the controls.

If using the raw FD is inevitable, then I'd suggest adding a as_device() method that returns a reference to the Device, which already implements AsRawFd - this at least makes the process a bit more explicit.

@sacovo
Copy link
Author

sacovo commented Jan 9, 2025

Yes, I meant AsRawRef.

My use case is on Raspberry Pi Zero, where I use the hardware encoder to encode a video stream. And in order to set the bitrate or other encoder settings I need those calls. But this could also be done with additional methods for the encoder and decoder. I need access to the fd of the encoder instance, because the hardware encoder returns a unique fd for every time it is opened, so there can be multiple independent instances with the same hardware device.

@sacovo
Copy link
Author

sacovo commented Jan 10, 2025

Would you prefer something like this?

impl<S> Encoder<S>
where
    S: EncoderState,
{
    pub fn set_ext_controls<I: AsV4l2ControlSlice>(
        &self,
        controls: I,
    ) -> Result<(), ExtControlError> {
        ioctl::s_ext_ctrls(&*self.device, ioctl::CtrlWhich::Current, controls)
    }

    pub fn get_ext_controls<I: AsV4l2ControlSlice>(
        &self,
        controls: I,
    ) -> Result<(), ExtControlError> {
        ioctl::g_ext_ctrls(&*self.device, ioctl::CtrlWhich::Current, controls)
    }

    pub fn try_ext_controls<I: AsV4l2ControlSlice>(
        &self,
        controls: I,
    ) -> Result<(), ExtControlError> {
        ioctl::try_ext_ctrls(&*self.device, ioctl::CtrlWhich::Current, controls)
    }
}

Setting the bitrate on an encoder would then look like this:

        encoder.set_ext_controls(&mut SafeExtControl::<VideoBitrate>::from_value(25_000))?;

@Gnurou
Copy link
Owner

Gnurou commented Jan 25, 2025

Sorry, took some time to come back to this.

So for stateful decoding and encoding, we normally have a very limited set of controls. Many are read-only like V4L2_CID_MIN_BUFFERS_FOR_CAPTURE, and IIUC writable controls are mostly for controlling encoding parameters. Would it make more sense to just expose them through methods or decoding state structures? For instance, setting the bitrate could be done by instantiating an e.g. EncodingParameters struct from the encoder to get the current parameters or the stream, calling a setBitrate method to change that one parameter (with other methods also available to change other parameters), and then commit that object to the encoder, which is when S_EXT_CTRLS is effectively called behind the scene?

I.e. present an interface that is high-level enough to hide the controls since the set of controls for a stateful decoder or encoder is well-defined and rather limited. This is my current view of the problem, but feel free to challenge it if you think it doesn't make sense.

@sacovo
Copy link
Author

sacovo commented Jan 26, 2025

Thanks for coming back to this @Gnurou!

I'm not too versed in encoders and decoders, and I'm mostly interested in setting the bitrate of the encoder.

I think for that the approach you describe would work, and if the amount of available controls is small and well known, it would make sense to spell them out like you describe.

I should be able to implement something along your description in the next few weeks.

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.

2 participants