search

chromeos / cros-codecs

BSD 3-Clause "New" or "Revised" License
30 stars 11 forks source link

Dev v4l2 stateless #83

Open semigle opened 4 months ago

semigle commented 4 months ago

This PR provides initial implementation of the v4l2 stateless decoder backend implemented on top of the v4l2r library. It depends on the https://github.com/Gnurou/v4l2r/pull/36.

TODOs:

Gnurou commented 4 months ago

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

1) 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 #85 to fix this.

2) 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 commented 3 weeks ago

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 commented 3 weeks ago

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

  1. 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.
  2. 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 commented 3 weeks ago

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 commented 3 weeks ago

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

  1. 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.
  2. 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.

https://github.com/Gnurou/v4l2r/pull/36#issuecomment-2395453045