| Message ID | 20260312160009.18654-1-david.plowman@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for the patch. Quoting David Plowman (2026-03-12 16:10:37) > Hi everyone > > Here's a version of Barnabas's previous patch (hence "v2") that added > a separate applyControls function, to bypass the request queue. If you > recall, I was a fan of this one. > > I've re-worked it only slightly, replacing the single pending > ControlList with a queue, which is the functionality that was > important to us. To make that explicit, I've renamed "applyControls" > to "queueControls" everywhere. I'm a bit hesitant here. The thing that nags me is that this introduces a second queuing mechanism. I see that it would help the RPi use case (we should get a name for that) and I feel bad that we're still not there, yet. applyControls() was meant as a mechanism to apply controls as fast as possible (basically modifying the current state of the camera) without going through any queue. So multiple calls to this function would all be collated into the next possible point where we can apply controls. Replacing that with a queing mechanism contradicts that target. I still believe that the use case you are targeting can well be fulfilled with the request based model but requires us to split requests from buffers (which is on several todo lists since far too long, but I'm sure it will happen). So introducing another queuing mechanism would likely bring us in a situation where the behavior is even harder to describe. > > I've also tried, in passing, to answer some of the hanging questions: > > * Like requests, I've not allowed controls to be queued before the > camera is running. Don't feel strongly either way, really. > > * I've allowed queueing empty ControlLists. Doing this allows you to > get "another frame like the last one". As said above, that defeats the "set the exposure value on every change event of a slider UI control and the last one wins" use case. Best regards, Stefan > > * I clear out all pending controls when the camera stops. If an > application needs to be sure they have been applied, it should wait > (which kind of leads us on to PFC, but not a subject for this > patch!). Or if it doesn't want to wait, it should re-apply any > in-question values when it re-starts the camera. > > * I'm using the fallback only when the PH explicitly doesn't support > queueControlsDevice. Really, if it's supported it's got to work > every time, even if that only means putting it in a queue in the > PH. Having some ControlLists go into the PH here, and then to have > it fail and put others into the pending queue, just feels entirely > confusing! > > * In the fallback case I'm copying the ControlList so that we can > recover if the request fails to queue. Firstly, I don't think this > is too horrible, and anyway, it's a fallback. If anyone cares about > this they should implement queueControlsDevice. > > TBH, I'm a bit unconvinced by the whole fallback thing. We (RPi) > always queue up lots of requests as soon as we can, and to my > understanding they all go through to our PH pretty much straight away, > which kind of nullifies the whole point. But maybe other PHs behave > differently? Always happy to be corrected on any misconceptions! > > Thanks > David > > David Plowman (1): > libcamera: Add independent queue for ControlLists > > include/libcamera/camera.h | 1 + > include/libcamera/internal/camera.h | 1 + > include/libcamera/internal/pipeline_handler.h | 7 ++ > src/libcamera/camera.cpp | 61 +++++++++++++++ > src/libcamera/pipeline_handler.cpp | 76 +++++++++++++++++++ > 5 files changed, 146 insertions(+) > > -- > 2.47.3 >
Hi Stefan Thanks for the comments. I'm always very happy when people want to discuss this!! On Fri, 20 Mar 2026 at 16:42, Stefan Klug <stefan.klug@ideasonboard.com> wrote: > > Hi David, > > Thank you for the patch. > > Quoting David Plowman (2026-03-12 16:10:37) > > Hi everyone > > > > Here's a version of Barnabas's previous patch (hence "v2") that added > > a separate applyControls function, to bypass the request queue. If you > > recall, I was a fan of this one. > > > > I've re-worked it only slightly, replacing the single pending > > ControlList with a queue, which is the functionality that was > > important to us. To make that explicit, I've renamed "applyControls" > > to "queueControls" everywhere. > > I'm a bit hesitant here. The thing that nags me is that this introduces > a second queuing mechanism. I see that it would help the RPi use case > (we should get a name for that) and I feel bad that we're still not > there, yet. > > applyControls() was meant as a mechanism to apply controls as fast as > possible (basically modifying the current state of the camera) without > going through any queue. So multiple calls to this function would all be > collated into the next possible point where we can apply controls. > Replacing that with a queing mechanism contradicts that target. Yes, I can appreciate that there may be a use-case where you want to apply controls immediately and aren't interested in getting sequences of controlled frames. Rather than just trying to implement one or the other, I'd be fine with making both alternatives available - so long as we all get to bypass the request queue! > > I still believe that the use case you are targeting can well be > fulfilled with the request based model but requires us to split requests > from buffers (which is on several todo lists since far too long, but I'm > sure it will happen). > > So introducing another queuing mechanism would likely bring us in a > situation where the behavior is even harder to describe. The whole business of splitting buffers (or controls) from requests is of course important here. I always struggle with the terminology. If you take buffers out of requests into their own queue, a request queue is really just a control list queue. Or if you take the control lists out of the requests, then the request queue is basically a buffer queue (which is what I had). So it's not terribly clear to me what a "request" ends up being, but I don't think I mind too much what things are called so long as we have the functionality we want. But whatever we do, I think we end up with more than one queue, so defining how they interact is unavoidable. The Kamaros view was at least interesting in that respect - it had a "command" (mostly like a control list) queue (actually more than one!), and then separate buffer queues. Not saying that's the thing to do, but it's one data point. I think I nominated this as a topic for Nice, so really hopeful that folks are up for a discussion and that we can plan some concrete first steps in this area. Thanks! David > > > > > I've also tried, in passing, to answer some of the hanging questions: > > > > * Like requests, I've not allowed controls to be queued before the > > camera is running. Don't feel strongly either way, really. > > > > * I've allowed queueing empty ControlLists. Doing this allows you to > > get "another frame like the last one". > > As said above, that defeats the "set the exposure value on every change > event of a slider UI control and the last one wins" use case. > > Best regards, > Stefan > > > > > * I clear out all pending controls when the camera stops. If an > > application needs to be sure they have been applied, it should wait > > (which kind of leads us on to PFC, but not a subject for this > > patch!). Or if it doesn't want to wait, it should re-apply any > > in-question values when it re-starts the camera. > > > > * I'm using the fallback only when the PH explicitly doesn't support > > queueControlsDevice. Really, if it's supported it's got to work > > every time, even if that only means putting it in a queue in the > > PH. Having some ControlLists go into the PH here, and then to have > > it fail and put others into the pending queue, just feels entirely > > confusing! > > > > * In the fallback case I'm copying the ControlList so that we can > > recover if the request fails to queue. Firstly, I don't think this > > is too horrible, and anyway, it's a fallback. If anyone cares about > > this they should implement queueControlsDevice. > > > > TBH, I'm a bit unconvinced by the whole fallback thing. We (RPi) > > always queue up lots of requests as soon as we can, and to my > > understanding they all go through to our PH pretty much straight away, > > which kind of nullifies the whole point. But maybe other PHs behave > > differently? Always happy to be corrected on any misconceptions! > > > > Thanks > > David > > > > David Plowman (1): > > libcamera: Add independent queue for ControlLists > > > > include/libcamera/camera.h | 1 + > > include/libcamera/internal/camera.h | 1 + > > include/libcamera/internal/pipeline_handler.h | 7 ++ > > src/libcamera/camera.cpp | 61 +++++++++++++++ > > src/libcamera/pipeline_handler.cpp | 76 +++++++++++++++++++ > > 5 files changed, 146 insertions(+) > > > > -- > > 2.47.3 > >