Dev v4l2 stateless #83

semigle · 2024-07-02T09:10:33Z

This PR provides initial implementation of the v4l2 stateless decoder backend implemented on top of the v4l2r library. It depends on the Gnurou/v4l2r#36.

TODOs:

handle video device path (it's temporary set to /dev/video-dec0)
probe media device path (it's temporary set to /dev/video-med0)
handle memory backends other than mmap
handle video formats other than h264
handle queue start/stop at runtime
handle DRC at runtime

This commit provides some extra V4L2 fields to the SliceHeader which are calculated by parser while processing h264 stream.

This commit provides initial implementation of v4l2 stateless device implemented on top of the v4l2r library. This abstraction layer is intended to be used directly by v4l2 backend implementation. TODO: * handle video device path (it's temporary set to /dev/video-dec0) * probe media device path (it's temporary set to /dev/video-med0) * handle memory backends other than mmap * handle video formats other than h264 * handle queue start/stop at runtime * handle DRC at runtime

Initial implementation of v4l2 stateless decoder backend.

Initial implementation of h264 decoder on top of v4l2 stateless backend TODO: * Add support for scaling matrix

…4l2CtrlH264Pps` wrapper types We can achieve the same result through existing `From` implementations.

This RefCell was there to provide interior mutability because some of the methods did not take Self as mutable. With proper mutability parameters, and a use of std::mem::take, it can be removed.

Gnurou

Please see my initial comments - there are a few things I'd like to see if we cannot simplify, notably the weak buffers, so I'll take more time to review the rest.

One general comment, is that we should not use expect or panic in library code except to indicate a bug in our code (and even there, as a last resort). Please define errors using thiserror and return them wherever relevant - this makes the code a bit more verbose (although the ? operator limits this) but also more solid.

Gnurou · 2024-07-08T07:21:17Z

src/codec/h264/parser.rs

@@ -266,6 +266,10 @@ pub struct SliceHeader {
    /// the bottom field of a coded frame specified in clause 8.2.1.
    pub delta_pic_order_cnt: [i32; 2],

+    /// This value is required by V4L2 stateless decode params so it is calculated


It doesn't matter which backend requires this - the parser should be backend-agnostic, so no need to mention V4L2 here.

Makes sense. If it is only needed for V4L2 purpose, it might not be a right place to calculate here in the first place. My todo to check and move this if relevant.

Gnurou · 2024-07-08T07:21:25Z

src/codec/h264/parser.rs

@@ -315,6 +319,10 @@ pub struct SliceHeader {
    /// Decoded reference picture marking parsed using 7.3.3.3
    pub dec_ref_pic_marking: RefPicMarking,

+    /// This value is required by V4L2 stateless decode params so it is calculated


Gnurou · 2024-07-08T07:24:13Z

examples/ccdec-v4l2-stateless.rs

@@ -0,0 +1,187 @@
+// Copyright 2024 The ChromiumOS Authors


I believe we should be able to integrate this into the existing ccdec instead of creating a new program. I did a recent change that made the decoders codec and backend agnostic, so you should be able to plug a V4L2 stateless decoder there without any problem - please let me know if you see any blocker though.

Gnurou · 2024-07-08T07:24:45Z

src/device/v4l2/stateless/controls/h264.rs

+use v4l2r::controls::codec::H264Sps;
+use v4l2r::controls::SafeExtControl;
+
+impl From<&Sps> for v4l2_ctrl_h264_sps {


Nice! That's exactly what the From trait is for.

Gnurou · 2024-07-08T07:28:34Z

src/device/v4l2/stateless/controls/h264.rs

+    }
+}
+
+#[derive(Default)]


I think V4l2CtrlH264Sps and V4l2CtrlH264Pps are unneeded. They only wrap v4l2_ctrl_h264_{sps,pps} temporarily in order to convert a Sps or Pps into a SafeControl. You should be able to achieve the same result with existing methods.

It is a bit difficult to explain though, so I've written a CL on top of your series that shows what I mean - please check if that works, and squash it into this CL if it does.

already done by Alex. I plan to keep it separate for the history.

1af2ce1

Gnurou · 2024-07-08T07:50:14Z

src/device/v4l2/stateless/request.rs

+            buffer,
+        }
+    }
+    fn ioctl<C, T>(&mut self, ctrl: C) -> &mut Self


This does not tell us which ioctl we are doing. add_ctrl is probably a better name.

Gnurou · 2024-07-08T07:50:28Z

src/device/v4l2/stateless/request.rs

+        ioctl::s_ext_ctrls(&self.device, which, &mut ctrl).expect("Failed to set output control");
+        self
+    }
+    fn write(&mut self, data: &[u8]) -> &mut Self {


Maybe write_input_buffer?

Gnurou · 2024-07-08T07:51:28Z

src/device/v4l2/stateless/request.rs

+}
+
+#[derive(Default)]
+enum RequestHandle {


This should probably be named RequestState.

Gnurou · 2024-07-08T07:52:42Z

src/device/v4l2/stateless/request.rs

+}
+
+pub struct V4l2Request {
+    handle: RefCell<RequestHandle>,


Having a RefCell without a Rc usually indicates something is wrong with the mutability of your interface. I think this can be simplified - please see the follow-up CL I've added to your series, and squash it if you agree.

Gnurou · 2024-07-08T07:53:00Z

src/device/v4l2/stateless/queue.rs

+    Init(Queue<Output, QueueInit>),
+    Streaming(Queue<Output, BuffersAllocated<Vec<MmapHandle>>>),
+    #[default]
+    Unknown,


I guess this is more Invalid than Unknown?

Gnurou · 2024-07-09T01:27:14Z

src/device/v4l2/stateless/request.rs

+    {
+        match self {
+            Self::Init(handle) => handle.ioctl(ctrl),
+            _ => panic!("ERROR"),


In order to avoid these runtime panics, you can use something called the typestate pattern to ensure that all state transitions of RequestHandle are valid at compile-time: https://cliffle.com/blog/rust-typestate/

This also makes me think that maybe your Request type could be useful beyond the stateless decoder. Let's see how this turns out, but maybe we can move it into v4l2r to make it useful for other use-cases as well.

Gnurou · 2024-07-09T14:23:27Z

Also your PR has revealed a few design issues with cros-codecs and v4l2r:

The stateless decoders check the number of available output buffers before submitting a picture to the backend. This is needed for VAAPI, but not for V4L2 stateless. I have opened issue decoder/stateless: backends should check the number of available output buffers, if needed. #85 to fix this.
In v4l2r, the lifetime that QBuffer has on its queue is preventing us from working properly - that's why you introduced QBufferWeak, but this also has problems - it's basically a different version of QBuffer, and the current design makes it possible to submit a buffer to a queue different from the one that produced it. I think we can actually fix QBuffer to handle both cases nicely - I'll try and come with a proposal this week.

stevecho321 · 2024-10-31T22:57:27Z

Please see my initial comments - there are a few things I'd like to see if we cannot simplify, notably the weak buffers, so I'll take more time to review the rest.

One general comment, is that we should not use expect or panic in library code except to indicate a bug in our code (and even there, as a last resort). Please define errors using thiserror and return them wherever relevant - this makes the code a bit more verbose (although the ? operator limits this) but also more solid.

Alex, I will be commenting here on some of your comments. I don't expect response as I know you can't reply or contribute here.

I hope to reflect some of these comments here in this fork or in a separate fork I created. I am checking Slawomir's preference here.

It is just for my record and potentially getting Slawomir's comment here if relevant. Hope this is not too disturbing for you and please feel free to ignore/turn off any notification on this. :)

stevecho321 · 2024-10-31T23:36:38Z

Also your PR has revealed a few design issues with cros-codecs and v4l2r:

The stateless decoders check the number of available output buffers before submitting a picture to the backend. This is needed for VAAPI, but not for V4L2 stateless. I have opened issue decoder/stateless: backends should check the number of available output buffers, if needed. #85 to fix this.

In v4l2r, the lifetime that QBuffer has on its queue is preventing us from working properly - that's why you introduced QBufferWeak, but this also has problems - it's basically a different version of QBuffer, and the current design makes it possible to submit a buffer to a queue different from the one that produced it. I think we can actually fix QBuffer to handle both cases nicely - I'll try and come with a proposal this week.

IIRC, I saw the patch for 2. Will add here later.

stevecho321 · 2024-10-31T23:39:04Z

This PR provides initial implementation of the v4l2 stateless decoder backend implemented on top of the v4l2r library. It depends on the Gnurou/v4l2r#36.

TODOs:

handle video device path (it's temporary set to /dev/video-dec0)

probe media device path (it's temporary set to /dev/video-med0)

handle memory backends other than mmap

handle video formats other than h264

handle queue start/stop at runtime

handle DRC at runtime

health check (build) failed, but I can't find details in the link. Will try it again or I will try to reproduce it locally once I first get the build working for vaapi.

https://github.com/chromeos/cros-codecs/actions/runs/9835702975/job/27149880132?pr=83

stevecho321 · 2024-11-02T00:51:10Z

Also your PR has revealed a few design issues with cros-codecs and v4l2r:

The stateless decoders check the number of available output buffers before submitting a picture to the backend. This is needed for VAAPI, but not for V4L2 stateless. I have opened issue decoder/stateless: backends should check the number of available output buffers, if needed. #85 to fix this.

In v4l2r, the lifetime that QBufferhas on its queue is preventing us from working properly - that's why you introduced QBufferWeak, but this also has problems - it's basically a different version of QBuffer, and the current design makes it possible to submit a buffer to a different queue from the one that produced it. I think we can actually fix QBufferto handle both cases nicely - I'll try and come with a proposal this week.

IIRC, I saw the patch for 2. Will add here later.

I think we can actually fix QBufferto handle both cases nicely - I'll try and come with a proposal this week.

Link for the this change.

Gnurou/v4l2r#36 (comment)

semigle and others added 7 commits July 3, 2024 09:26

codec/h264: Add missing V4l2 fields to SliceHeader

8d99b44

This commit provides some extra V4L2 fields to the SliceHeader which are calculated by parser while processing h264 stream.

backend/v4l2: Add v4l2 stateless decoder backend

52f81bd

Initial implementation of v4l2 stateless decoder backend.

decoder/h264: Add support for v4l2 backend

2a66965

Initial implementation of h264 decoder on top of v4l2 stateless backend TODO: * Add support for scaling matrix

examples/ccdec: Add example for v4l2 backend

07dfccd

decoder/stateless/v4l2/h264: remove unneeded V4l2CtrlH264Sps and `V…

1af2ce1

…4l2CtrlH264Pps` wrapper types We can achieve the same result through existing `From` implementations.

decoder/stateless/v4l2: remove RefCell from V4l2Request

f51face

This RefCell was there to provide interior mutability because some of the methods did not take Self as mutable. With proper mutability parameters, and a use of std::mem::take, it can be removed.

Gnurou reviewed Jul 8, 2024

View reviewed changes

Gnurou force-pushed the dev-v4l2_stateless branch from 57c9560 to f51face Compare July 8, 2024 07:56

Gnurou reviewed Jul 9, 2024

View reviewed changes

Gnurou mentioned this pull request Jul 9, 2024

decoder/stateless: backends should check the number of available output buffers, if needed. #85

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dev v4l2 stateless #83

Dev v4l2 stateless #83

semigle commented Jul 2, 2024 •

edited

Loading

Gnurou left a comment

Gnurou Jul 8, 2024

stevecho321 Oct 31, 2024

Gnurou Jul 8, 2024

Gnurou Jul 8, 2024

Gnurou Jul 8, 2024

Gnurou Jul 8, 2024

stevecho321 Oct 31, 2024

Gnurou Jul 8, 2024

Gnurou Jul 8, 2024

Gnurou Jul 8, 2024

Gnurou Jul 8, 2024

Gnurou Jul 8, 2024

Gnurou Jul 9, 2024

Gnurou commented Jul 9, 2024

stevecho321 commented Oct 31, 2024 •

edited

Loading

stevecho321 commented Oct 31, 2024 •

edited

Loading

stevecho321 commented Oct 31, 2024

stevecho321 commented Nov 2, 2024 •

edited

Loading

Dev v4l2 stateless #83

Are you sure you want to change the base?

Dev v4l2 stateless #83

Conversation

semigle commented Jul 2, 2024 • edited Loading

Gnurou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gnurou commented Jul 9, 2024

stevecho321 commented Oct 31, 2024 • edited Loading

stevecho321 commented Oct 31, 2024 • edited Loading

stevecho321 commented Oct 31, 2024

stevecho321 commented Nov 2, 2024 • edited Loading

semigle commented Jul 2, 2024 •

edited

Loading

stevecho321 commented Oct 31, 2024 •

edited

Loading

stevecho321 commented Oct 31, 2024 •

edited

Loading

stevecho321 commented Nov 2, 2024 •

edited

Loading