| Message ID | 20260629163017.863145-1-barnabas.pocze@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabas Thanks for working on all this, very excited to see progress on this topic! Unfortunately I'm about to disappear on holiday for 2 weeks, so this reply is going to turn into a long list of questions that I've not had a huge amount of time to think about, so apologies in advance for that. On Mon, 29 Jun 2026 at 17:30, Barnabás Pőcze <barnabas.pocze@ideasonboard.com> wrote: > > This is the first RFC changeset for splitting requests and buffers. It largely > includes https://patchwork.libcamera.org/cover/26964/, but many of those are > already reviewed. > > The "interesting" changes start with "test: fence: Disable temporarily". > > --- > > # introduction > > The basic idea is that requests no longer have buffers when they are submitted, > and the camera keeps track of a set of buffers for each stream, and buffers will > be attached to requests from this pool in an unspecified order > > A new function, `Camera::addBuffer(stream, buffer, fence)` is added to fill the pool; > this can be called in the "Running" state. `Camera::stop()` empties the pool via the > `bufferCompleted` event. Another new function, `Request::enableStream(stream, bool)` > is used to set which streams a request wants frames from. I guess I'm vaguely expecting the buffer pool mechanism to be to a large extent separate from the camera system. So the camera system will obviously be a client of it, but otherwise they're not really intertwined. Is that likely to be the case here? Or maybe it's not possible. I'm also vaguely feeling an application might want to control the lifetime of the pool and contents. For example, when the camera is stopped, maybe you want to leave your buffers in the pool so that you can simply restart it? Wasn't sure if that kind of behaviour is envisaged here. > > Pipeline handlers can use `Camera::Private::acquireBuffer(stream)` to retrieve a > buffer for the given stream. If this buffer is not acceptable, then > `Camera::Private::rejectBuffer(buffer)` can be used to reject it. Otherwise > `completeBuffer(request, buffer)` is used normally to attach it to a request. > > This new behaviour is opt-in for pipeline handlers for now, with no opt-in, > the `PipelineHandler` base class fills each request with buffers before > calling `queueRequestDevice()`. One thing I raised a while back was whether there are applications that want to target specific buffers at particular requests. I'm thinking of Android here, though Android remains a giant mystery to me. But maybe another application might have a reason to do that? The thought was to leave *optional* buffers in request. If it's there, you use it and otherwise the camera system goes to the buffer pool to get one. It sort of feels to me like it should be doable, but is it useful? I used to wonder whether an application might want to hold back a number of buffers so that it could do a burst capture, guaranteeing to have a buffer for every request. But I'm not so convinced by that, presumably you can check if you have enough buffers in the pool. Though having optional buffers would certainly ease migration to the new scheme! > > > # discussion points > > ## adding buffers before `Camera::start()` > > Currently buffers can only be added in the "Running" state. They could > be added in "Configured" as well, but there is no "unconfigure" function, > and putting it into `stop()` would be a bit asymmetric. Indeed, I've always wanted to queue requests before starting the camera! > > ## empty pool notification > > Currently there is no mechanism for the application to know how many > buffers it should keep in the pool. I think a signal to notify the application > when it should add more buffers for a given stream could be added. But the > main idea was to expose some kind of minimum buffer count quantity. > > ## requirements towards application wrt. buffers > > With this prototype, an application is essentially required to always process *all* > buffers in *all* completed requests, otherwise it might lose buffers. (This > applies to cancelled requests as well.) Is that reasonable? I think it sounds OK to expect applications to manage buffers, at least to an extent. As I remarked above, I think maybe applications will want to manage buffers for their own convenience. > > ## returning unused buffers > > Currently this is done from `Camera::stop()` with the `bufferCompleted` signal > where `request == nullptr`. This works, but may not be the best interface. > > ## returning rejected buffers > > Pipeline handlers can reject buffers. But how should that be signalled to the > application. Currently this is also done via `bufferCompleted` with `request == nullptr`. > But this is not ideal because there isn't really a way to distinguish it from > an unused buffer since both are returned with `FrameMetadata::Status::FrameCancelled` > because `FrameError` requires some other fields to be valid. > > ## extending `completeBuffer()` with stream parameter > > Initially I planned to add a stream parameter to `completeBuffer()`, but most pipeline handlers > are not trivially convertible, especially something like `imx8-isi`. So I simply went ahead > with storing the associated stream in the `FrameBuffer` itself for now. I still think having > `completeBuffer(req, stream, buf)` is worthwhile, but it is not a "natural" change at the moment. It does feel a bit strange to me that buffers should know about streams, and so on. I think I was expecting buffers/pools to be largely independent of such things. But I accept there might be a good reason. > > ## fence timeout > > Previously a hard-coded timeout was used for fences. This is now removed. > Should there be a hard-coded fixed timeout for fences? > > > # TODO > > # converting more pipeline handlers > > Only the uvcvideo pipeline handler is converted to use the buffer pool > directly as an experiment, but more should be converted to really see > if this public api is a good fit for pipeline handlers. Especially something > like `imx8-isi` since that has a bit different architecture from the others. > > # minimum buffer count > > The applications need to know how many buffers should be kept in the pool > for a given stream for continuous streaming. This should be a property or > similar. Yes, I guess so. In practice we always allocate way more than the minimum, simply because that avoids frame drops. > > # `FrameBuffer::Private::request()` > > After all pipeline handlers are migrated, the request member of `FrameBuffer` > should probably be removed to fully decouple the two. One other slightly unusual use case I'd like to add into the mix is one I remember hearing on a Kamaros call. There's a stream of frames coming back from a camera, but then there's a second stream, made from the same sensor images, but which is generated only occasionally (so runs at a lower framerate). Does the buffer pool mechanism give us a way to handle this? When a request completes there could be 3 outcomes - 1. here's your buffer, 2. sorry, there wasn't a buffer in the pool, and now 3. there wasn't actually a frame generated for this stream on this occasion. The final thing I'd like to discuss is how this relates to control queues. Of course, strictly speaking the two subjects are independent, but nonetheless I have a couple of questions. * The buffer/request split will obviously break the existing API and cause a certain amount of work. If we're going to have control queues, are we going to break everything again later, or do it all at once? Just wondering... * If you took control lists out of requests you might find there's almost nothing left in them. Wanted to check we're OK with that just in case we were to end up there! Anyway, apologies for the big dump of questions and then running away. Look forward to hearing more on the subject! Thanks David > > --- > > Barnabás Pőcze (54): > apps: cam: Simplify buffer reuse > libcamera: request: Disassociate buffer when cancelling > libcamera: pipeline: Replace open-coded request cancellation > libcamera: pipeline: mali-c55: Remove `setRequest()` calls > libcamera: pipeline: virtual: Make copy of request's buffer map > libcamera: request: completeBuffer(): Emit `bufferCompleted` here > libcamera: pipeline_handler: completeBuffer(): Inline and `static` > v4l2: v4l2_camera: Avoid a level of indirection > v4l2: v4l2_camera: Remove repated index checks > v4l2: v4l2_camera_proxy: Remove `bufferCount_` > v4l2: v4l2_camera: Use actually allocated buffer count > v4l2: v4l2_camera: Use buffer cookie for indexing > v4l2: v4l2_camera: Rename `Buffer` to `CompletedBuffer` > v4l2: v4l2_camera: Always clear pending requests > v4l2: v4l2_camera: Clear completed requests when stopping > v4l2: v4l2_camera: Provide buffers one by one > test: fence: Disable temporarily > libcamera: request: Remove `ReuseBuffers` > libcamera: framebuffer: request(): Move to private type > libcamera: framebuffer: Store associated Stream > libcamera: camera: Add `StreamData` > libcamera: camera: Add buffer pool > libcamera: request: addBuffer(): Remove impl > libcamera: request: Remove fence support > libcamera: request: Store count of pending buffers > libcamera: request: doCancelRequest(): Remove > libcamera: pipeline_handler: Move constructor options to a separate > type > libcamera: pipeline_handler: Acquire buffers if not using pool > libcamera: request: enableStream(): Add > libcamera: camera: acquireBuffer(): Add > libcamera: camera: rejectBuffer(): Add > libcamera: pipeline_handler: buffersAddedDevice(): New virtual > function > libcamera: pipeline_handler: Use `std::deque` > libcamera: pipeline_handler: completeRequest(): Return request count > libcamera: pipeline: uvcvideo: Use buffer pool prototype > libcamera: request: completeBuffer(): Emit `bufferCompleted` last > libcamera: camera: bufferCompleted: Pass `Stream` as well > libcamera: camera: queueRequest(): Adjust buffer map empty error > message > libcamera: camera: queueBuffer(): Reject if it has buffers > apps: cam: Use camera buffer pool > app: lc-compliance: Use camera buffer pool > apps: lc-compliance: Add buffer pool tests > test: Use camera buffer pool > apps: qcam: Use camera buffer pool > v4l2: Use camera buffer pool > py: Use camera buffer pool > gstreamer: Use camera buffer pool > libcamera: request: Remove `ReuseFlag` > android: Update imported files > android: camera_request: Add helper for buffer conversion > android: camera_device: Move fence restoration into > `prepareToReturn()` > android: Use camera buffer pool > libcamera: request: Remove `addBuffer()` > test: fence: Enable > > .../guides/application-developer.rst | 9 - > Documentation/guides/pipeline-handler.rst | 10 +- > .../libhardware/include/hardware/camera3.h | 360 +- > .../include/hardware/camera_common.h | 304 +- > .../libhardware/include/hardware/gralloc.h | 99 +- > .../android/metadata/camera_metadata_hidden.h | 11 +- > .../android/metadata/system/camera_metadata.h | 41 +- > .../metadata/system/camera_metadata_tags.h | 633 +- > .../metadata/system/camera_vendor_tags.h | 10 +- > .../android/system/core/include/android/log.h | 450 +- > .../core/include/cutils/native_handle.h | 62 +- > .../system/core/include/system/camera.h | 4 +- > .../core/include/system/graphics-base-v1.2.h | 36 + > .../core/include/system/graphics-base.h | 1 + > .../system/core/include/system/graphics.h | 10 +- > include/libcamera/camera.h | 5 +- > include/libcamera/framebuffer.h | 1 - > include/libcamera/internal/camera.h | 46 +- > include/libcamera/internal/framebuffer.h | 6 + > include/libcamera/internal/pipeline_handler.h | 31 +- > include/libcamera/internal/request.h | 24 +- > include/libcamera/request.h | 10 +- > src/android/camera3_hal.cpp | 3 + > src/android/camera_capabilities.cpp | 3 + > src/android/camera_device.cpp | 287 +- > src/android/camera_device.h | 4 + > src/android/camera_hal_manager.cpp | 2 +- > src/android/camera_ops.cpp | 16 +- > src/android/camera_request.cpp | 53 +- > src/android/camera_request.h | 11 +- > src/android/metadata/camera_metadata.c | 183 +- > .../metadata/camera_metadata_tag_info.c | 5478 ++++++++++++++++- > src/apps/cam/camera_session.cpp | 36 +- > src/apps/cam/camera_session.h | 2 +- > src/apps/lc-compliance/helpers/capture.cpp | 23 +- > src/apps/lc-compliance/tests/capture_test.cpp | 261 + > src/apps/qcam/main_window.cpp | 82 +- > src/apps/qcam/main_window.h | 1 - > src/gstreamer/gstlibcamerasrc.cpp | 136 +- > src/libcamera/camera.cpp | 164 +- > src/libcamera/fence.cpp | 15 +- > src/libcamera/framebuffer.cpp | 57 +- > src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 5 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +- > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 7 +- > src/libcamera/pipeline/mali-c55/rzg2l-cru.cpp | 3 +- > src/libcamera/pipeline/mali-c55/rzg2l-cru.h | 3 +- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +- > .../pipeline/rpi/common/pipeline_base.cpp | 16 +- > .../pipeline/rpi/common/pipeline_base.h | 2 +- > src/libcamera/pipeline/simple/simple.cpp | 17 +- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 72 +- > src/libcamera/pipeline/vimc/vimc.cpp | 11 +- > src/libcamera/pipeline/virtual/virtual.cpp | 44 +- > src/libcamera/pipeline_handler.cpp | 260 +- > src/libcamera/request.cpp | 290 +- > src/py/cam/cam.py | 20 +- > src/py/examples/simple-cam.py | 13 +- > src/py/examples/simple-capture.py | 20 +- > src/py/examples/simple-continuous-capture.py | 32 +- > src/py/libcamera/py_main.cpp | 65 +- > src/v4l2/v4l2_camera.cpp | 150 +- > src/v4l2/v4l2_camera.h | 25 +- > src/v4l2/v4l2_camera_proxy.cpp | 118 +- > src/v4l2/v4l2_camera_proxy.h | 3 - > test/camera/buffer_import.cpp | 22 +- > test/camera/camera_reconfigure.cpp | 22 +- > test/camera/capture.cpp | 24 +- > test/camera/statemachine.cpp | 4 +- > test/fence.cpp | 214 +- > 70 files changed, 9182 insertions(+), 1283 deletions(-) > create mode 100644 include/android/system/core/include/system/graphics-base-v1.2.h > > -- > 2.54.0
Hi I have tried to answer the questions, let me know if I'd missed anything. 2026. 07. 03. 14:37 keltezéssel, David Plowman írta: > Hi Barnabas > > Thanks for working on all this, very excited to see progress on this topic! > > Unfortunately I'm about to disappear on holiday for 2 weeks, so this > reply is going to turn into a long list of questions that I've not had > a huge amount of time to think about, so apologies in advance for > that. > > On Mon, 29 Jun 2026 at 17:30, Barnabás Pőcze > <barnabas.pocze@ideasonboard.com> wrote: >> >> This is the first RFC changeset for splitting requests and buffers. It largely >> includes https://patchwork.libcamera.org/cover/26964/, but many of those are >> already reviewed. >> >> The "interesting" changes start with "test: fence: Disable temporarily". >> >> --- >> >> # introduction >> >> The basic idea is that requests no longer have buffers when they are submitted, >> and the camera keeps track of a set of buffers for each stream, and buffers will >> be attached to requests from this pool in an unspecified order >> >> A new function, `Camera::addBuffer(stream, buffer, fence)` is added to fill the pool; >> this can be called in the "Running" state. `Camera::stop()` empties the pool via the >> `bufferCompleted` event. Another new function, `Request::enableStream(stream, bool)` >> is used to set which streams a request wants frames from. > > I guess I'm vaguely expecting the buffer pool mechanism to be to a > large extent separate from the camera system. So the camera system > will obviously be a client of it, but otherwise they're not really > intertwined. Is that likely to be the case here? Or maybe it's not > possible. The buffer pool mechanism is implemented in the "Camera" and related types. And not in e.g. a separate `BufferPool` type. This was deemed the simpler approach. But there are no technical obstacles as far as I can tell. Or do you mean something else? > > I'm also vaguely feeling an application might want to control the > lifetime of the pool and contents. For example, when the camera is > stopped, maybe you want to leave your buffers in the pool so that you > can simply restart it? Wasn't sure if that kind of behaviour is > envisaged here. I don't think that option was discussed, but could be implemented. Nonetheless then `configure()` has to clear the pool in any case, which is technically a bit more complicated to do. > >> >> Pipeline handlers can use `Camera::Private::acquireBuffer(stream)` to retrieve a >> buffer for the given stream. If this buffer is not acceptable, then >> `Camera::Private::rejectBuffer(buffer)` can be used to reject it. Otherwise >> `completeBuffer(request, buffer)` is used normally to attach it to a request. >> >> This new behaviour is opt-in for pipeline handlers for now, with no opt-in, >> the `PipelineHandler` base class fills each request with buffers before >> calling `queueRequestDevice()`. > > One thing I raised a while back was whether there are applications > that want to target specific buffers at particular requests. I'm > thinking of Android here, though Android remains a giant mystery to > me. But maybe another application might have a reason to do that? Android has caused quite a bit of headache, so in the end a switch to their "new" buffer model was made: https://source.android.com/docs/core/camera/buffer-management-api This uses a similar pool based approach where requests no longer contain any buffers. > > The thought was to leave *optional* buffers in request. If it's there, > you use it and otherwise the camera system goes to the buffer pool to > get one. It sort of feels to me like it should be doable, but is it > useful? > > I used to wonder whether an application might want to hold back a > number of buffers so that it could do a burst capture, guaranteeing to > have a buffer for every request. But I'm not so convinced by that, > presumably you can check if you have enough buffers in the pool. > > Though having optional buffers would certainly ease migration to the new scheme! I don't have a satisfactory answer unfortunately. I think it's a matter of making a decision, it seems technically feasible. Of course it would complicate the core and pipeline handler parts. There is also a question of how to handle requests that mix both: having some buffers and also some "just" enabled streams. Then fences must also not be forgotten about. And then it has to be evaluated how many technical difficulties supporting this mixed model would cause for the existing pipeline handler implementations. An earlier prototype explored the idea of automatically adding buffers from requests to the pool when they are queued. This of course would break the association between buffers and requests, so it wouldn't address your point. And it was decided not to go forward with this compatibility behaviour in any case. > >> >> >> # discussion points >> >> ## adding buffers before `Camera::start()` >> >> Currently buffers can only be added in the "Running" state. They could >> be added in "Configured" as well, but there is no "unconfigure" function, >> and putting it into `stop()` would be a bit asymmetric. > > Indeed, I've always wanted to queue requests before starting the camera! > >> >> ## empty pool notification >> >> Currently there is no mechanism for the application to know how many >> buffers it should keep in the pool. I think a signal to notify the application >> when it should add more buffers for a given stream could be added. But the >> main idea was to expose some kind of minimum buffer count quantity. >> >> ## requirements towards application wrt. buffers >> >> With this prototype, an application is essentially required to always process *all* >> buffers in *all* completed requests, otherwise it might lose buffers. (This >> applies to cancelled requests as well.) Is that reasonable? > > I think it sounds OK to expect applications to manage buffers, at > least to an extent. As I remarked above, I think maybe applications > will want to manage buffers for their own convenience. > >> >> ## returning unused buffers >> >> Currently this is done from `Camera::stop()` with the `bufferCompleted` signal >> where `request == nullptr`. This works, but may not be the best interface. >> >> ## returning rejected buffers >> >> Pipeline handlers can reject buffers. But how should that be signalled to the >> application. Currently this is also done via `bufferCompleted` with `request == nullptr`. >> But this is not ideal because there isn't really a way to distinguish it from >> an unused buffer since both are returned with `FrameMetadata::Status::FrameCancelled` >> because `FrameError` requires some other fields to be valid. >> >> ## extending `completeBuffer()` with stream parameter >> >> Initially I planned to add a stream parameter to `completeBuffer()`, but most pipeline handlers >> are not trivially convertible, especially something like `imx8-isi`. So I simply went ahead >> with storing the associated stream in the `FrameBuffer` itself for now. I still think having >> `completeBuffer(req, stream, buf)` is worthwhile, but it is not a "natural" change at the moment. > > It does feel a bit strange to me that buffers should know about > streams, and so on. I think I was expecting buffers/pools to be > largely independent of such things. But I accept there might be a good > reason. Indeed they shouldn't. But migrating straight to `completeBuffer(req, stream, buf)` world where buffers have no idea about requests and streams is not as simple as I would have liked. Especially since most pipeline handlers are written with the current model in mind where a buffer is already in a request associated with a stream. And so the actual stream is not even easily available at buffer completion time in many cases. So this is a compromise. > >> >> ## fence timeout >> >> Previously a hard-coded timeout was used for fences. This is now removed. >> Should there be a hard-coded fixed timeout for fences? >> >> >> # TODO >> >> # converting more pipeline handlers >> >> Only the uvcvideo pipeline handler is converted to use the buffer pool >> directly as an experiment, but more should be converted to really see >> if this public api is a good fit for pipeline handlers. Especially something >> like `imx8-isi` since that has a bit different architecture from the others. >> >> # minimum buffer count >> >> The applications need to know how many buffers should be kept in the pool >> for a given stream for continuous streaming. This should be a property or >> similar. > > Yes, I guess so. In practice we always allocate way more than the > minimum, simply because that avoids frame drops. Yes, but this information can be useful if the buffer goes into a processing pipeline that holds on to it, or if the applications creates its own `FrameBuffer`s without the `FrameBufferAllocator`. > >> >> # `FrameBuffer::Private::request()` >> >> After all pipeline handlers are migrated, the request member of `FrameBuffer` >> should probably be removed to fully decouple the two. > > One other slightly unusual use case I'd like to add into the mix is > one I remember hearing on a Kamaros call. There's a stream of frames > coming back from a camera, but then there's a second stream, made from > the same sensor images, but which is generated only occasionally (so > runs at a lower framerate). > > Does the buffer pool mechanism give us a way to handle this? When a > request completes there could be 3 outcomes - 1. here's your buffer, > 2. sorry, there wasn't a buffer in the pool, and now 3. there wasn't > actually a frame generated for this stream on this occasion. You mean like a stream that might not supply a frame even if the request enables it, there are available buffers, and no errors are encountered? Or put differently, a stream that "itself" decides when to supply frames regardless of the streams in the request? In that case currently (2) and (3) cannot be differentiated. But it's a matter of extending the buffer map in the request. > > The final thing I'd like to discuss is how this relates to control > queues. Of course, strictly speaking the two subjects are independent, > but nonetheless I have a couple of questions. > > * The buffer/request split will obviously break the existing API and > cause a certain amount of work. If we're going to have control queues, > are we going to break everything again later, or do it all at once? > Just wondering... I suppose it would be better to break the world once, but I'm no authority on the question of libcamera stability and releases. > > * If you took control lists out of requests you might find there's > almost nothing left in them. Wanted to check we're OK with that just > in case we were to end up there! I think it's fine. A request would still have a "cookie" and the set of enabled streams. And it has a sequence number, status, a set of buffers, and a set of metadata items when it completes. Nonetheless we could explore getting rid of the application facing `Request` type and using something different, maybe something more centered around using the numeric sequence number as the identity. > > Anyway, apologies for the big dump of questions and then running away. > Look forward to hearing more on the subject! > > Thanks > David > >> >> --- >> >> Barnabás Pőcze (54): >> apps: cam: Simplify buffer reuse >> libcamera: request: Disassociate buffer when cancelling >> libcamera: pipeline: Replace open-coded request cancellation >> libcamera: pipeline: mali-c55: Remove `setRequest()` calls >> libcamera: pipeline: virtual: Make copy of request's buffer map >> libcamera: request: completeBuffer(): Emit `bufferCompleted` here >> libcamera: pipeline_handler: completeBuffer(): Inline and `static` >> v4l2: v4l2_camera: Avoid a level of indirection >> v4l2: v4l2_camera: Remove repated index checks >> v4l2: v4l2_camera_proxy: Remove `bufferCount_` >> v4l2: v4l2_camera: Use actually allocated buffer count >> v4l2: v4l2_camera: Use buffer cookie for indexing >> v4l2: v4l2_camera: Rename `Buffer` to `CompletedBuffer` >> v4l2: v4l2_camera: Always clear pending requests >> v4l2: v4l2_camera: Clear completed requests when stopping >> v4l2: v4l2_camera: Provide buffers one by one >> test: fence: Disable temporarily >> libcamera: request: Remove `ReuseBuffers` >> libcamera: framebuffer: request(): Move to private type >> libcamera: framebuffer: Store associated Stream >> libcamera: camera: Add `StreamData` >> libcamera: camera: Add buffer pool >> libcamera: request: addBuffer(): Remove impl >> libcamera: request: Remove fence support >> libcamera: request: Store count of pending buffers >> libcamera: request: doCancelRequest(): Remove >> libcamera: pipeline_handler: Move constructor options to a separate >> type >> libcamera: pipeline_handler: Acquire buffers if not using pool >> libcamera: request: enableStream(): Add >> libcamera: camera: acquireBuffer(): Add >> libcamera: camera: rejectBuffer(): Add >> libcamera: pipeline_handler: buffersAddedDevice(): New virtual >> function >> libcamera: pipeline_handler: Use `std::deque` >> libcamera: pipeline_handler: completeRequest(): Return request count >> libcamera: pipeline: uvcvideo: Use buffer pool prototype >> libcamera: request: completeBuffer(): Emit `bufferCompleted` last >> libcamera: camera: bufferCompleted: Pass `Stream` as well >> libcamera: camera: queueRequest(): Adjust buffer map empty error >> message >> libcamera: camera: queueBuffer(): Reject if it has buffers >> apps: cam: Use camera buffer pool >> app: lc-compliance: Use camera buffer pool >> apps: lc-compliance: Add buffer pool tests >> test: Use camera buffer pool >> apps: qcam: Use camera buffer pool >> v4l2: Use camera buffer pool >> py: Use camera buffer pool >> gstreamer: Use camera buffer pool >> libcamera: request: Remove `ReuseFlag` >> android: Update imported files >> android: camera_request: Add helper for buffer conversion >> android: camera_device: Move fence restoration into >> `prepareToReturn()` >> android: Use camera buffer pool >> libcamera: request: Remove `addBuffer()` >> test: fence: Enable >> >> .../guides/application-developer.rst | 9 - >> Documentation/guides/pipeline-handler.rst | 10 +- >> .../libhardware/include/hardware/camera3.h | 360 +- >> .../include/hardware/camera_common.h | 304 +- >> .../libhardware/include/hardware/gralloc.h | 99 +- >> .../android/metadata/camera_metadata_hidden.h | 11 +- >> .../android/metadata/system/camera_metadata.h | 41 +- >> .../metadata/system/camera_metadata_tags.h | 633 +- >> .../metadata/system/camera_vendor_tags.h | 10 +- >> .../android/system/core/include/android/log.h | 450 +- >> .../core/include/cutils/native_handle.h | 62 +- >> .../system/core/include/system/camera.h | 4 +- >> .../core/include/system/graphics-base-v1.2.h | 36 + >> .../core/include/system/graphics-base.h | 1 + >> .../system/core/include/system/graphics.h | 10 +- >> include/libcamera/camera.h | 5 +- >> include/libcamera/framebuffer.h | 1 - >> include/libcamera/internal/camera.h | 46 +- >> include/libcamera/internal/framebuffer.h | 6 + >> include/libcamera/internal/pipeline_handler.h | 31 +- >> include/libcamera/internal/request.h | 24 +- >> include/libcamera/request.h | 10 +- >> src/android/camera3_hal.cpp | 3 + >> src/android/camera_capabilities.cpp | 3 + >> src/android/camera_device.cpp | 287 +- >> src/android/camera_device.h | 4 + >> src/android/camera_hal_manager.cpp | 2 +- >> src/android/camera_ops.cpp | 16 +- >> src/android/camera_request.cpp | 53 +- >> src/android/camera_request.h | 11 +- >> src/android/metadata/camera_metadata.c | 183 +- >> .../metadata/camera_metadata_tag_info.c | 5478 ++++++++++++++++- >> src/apps/cam/camera_session.cpp | 36 +- >> src/apps/cam/camera_session.h | 2 +- >> src/apps/lc-compliance/helpers/capture.cpp | 23 +- >> src/apps/lc-compliance/tests/capture_test.cpp | 261 + >> src/apps/qcam/main_window.cpp | 82 +- >> src/apps/qcam/main_window.h | 1 - >> src/gstreamer/gstlibcamerasrc.cpp | 136 +- >> src/libcamera/camera.cpp | 164 +- >> src/libcamera/fence.cpp | 15 +- >> src/libcamera/framebuffer.cpp | 57 +- >> src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 5 +- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +- >> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 7 +- >> src/libcamera/pipeline/mali-c55/rzg2l-cru.cpp | 3 +- >> src/libcamera/pipeline/mali-c55/rzg2l-cru.h | 3 +- >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +- >> .../pipeline/rpi/common/pipeline_base.cpp | 16 +- >> .../pipeline/rpi/common/pipeline_base.h | 2 +- >> src/libcamera/pipeline/simple/simple.cpp | 17 +- >> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 72 +- >> src/libcamera/pipeline/vimc/vimc.cpp | 11 +- >> src/libcamera/pipeline/virtual/virtual.cpp | 44 +- >> src/libcamera/pipeline_handler.cpp | 260 +- >> src/libcamera/request.cpp | 290 +- >> src/py/cam/cam.py | 20 +- >> src/py/examples/simple-cam.py | 13 +- >> src/py/examples/simple-capture.py | 20 +- >> src/py/examples/simple-continuous-capture.py | 32 +- >> src/py/libcamera/py_main.cpp | 65 +- >> src/v4l2/v4l2_camera.cpp | 150 +- >> src/v4l2/v4l2_camera.h | 25 +- >> src/v4l2/v4l2_camera_proxy.cpp | 118 +- >> src/v4l2/v4l2_camera_proxy.h | 3 - >> test/camera/buffer_import.cpp | 22 +- >> test/camera/camera_reconfigure.cpp | 22 +- >> test/camera/capture.cpp | 24 +- >> test/camera/statemachine.cpp | 4 +- >> test/fence.cpp | 214 +- >> 70 files changed, 9182 insertions(+), 1283 deletions(-) >> create mode 100644 include/android/system/core/include/system/graphics-base-v1.2.h >> >> -- >> 2.54.0
This is the first RFC changeset for splitting requests and buffers. It largely includes https://patchwork.libcamera.org/cover/26964/, but many of those are already reviewed. The "interesting" changes start with "test: fence: Disable temporarily". --- # introduction The basic idea is that requests no longer have buffers when they are submitted, and the camera keeps track of a set of buffers for each stream, and buffers will be attached to requests from this pool in an unspecified order A new function, `Camera::addBuffer(stream, buffer, fence)` is added to fill the pool; this can be called in the "Running" state. `Camera::stop()` empties the pool via the `bufferCompleted` event. Another new function, `Request::enableStream(stream, bool)` is used to set which streams a request wants frames from. Pipeline handlers can use `Camera::Private::acquireBuffer(stream)` to retrieve a buffer for the given stream. If this buffer is not acceptable, then `Camera::Private::rejectBuffer(buffer)` can be used to reject it. Otherwise `completeBuffer(request, buffer)` is used normally to attach it to a request. This new behaviour is opt-in for pipeline handlers for now, with no opt-in, the `PipelineHandler` base class fills each request with buffers before calling `queueRequestDevice()`. # discussion points ## adding buffers before `Camera::start()` Currently buffers can only be added in the "Running" state. They could be added in "Configured" as well, but there is no "unconfigure" function, and putting it into `stop()` would be a bit asymmetric. ## empty pool notification Currently there is no mechanism for the application to know how many buffers it should keep in the pool. I think a signal to notify the application when it should add more buffers for a given stream could be added. But the main idea was to expose some kind of minimum buffer count quantity. ## requirements towards application wrt. buffers With this prototype, an application is essentially required to always process *all* buffers in *all* completed requests, otherwise it might lose buffers. (This applies to cancelled requests as well.) Is that reasonable? ## returning unused buffers Currently this is done from `Camera::stop()` with the `bufferCompleted` signal where `request == nullptr`. This works, but may not be the best interface. ## returning rejected buffers Pipeline handlers can reject buffers. But how should that be signalled to the application. Currently this is also done via `bufferCompleted` with `request == nullptr`. But this is not ideal because there isn't really a way to distinguish it from an unused buffer since both are returned with `FrameMetadata::Status::FrameCancelled` because `FrameError` requires some other fields to be valid. ## extending `completeBuffer()` with stream parameter Initially I planned to add a stream parameter to `completeBuffer()`, but most pipeline handlers are not trivially convertible, especially something like `imx8-isi`. So I simply went ahead with storing the associated stream in the `FrameBuffer` itself for now. I still think having `completeBuffer(req, stream, buf)` is worthwhile, but it is not a "natural" change at the moment. ## fence timeout Previously a hard-coded timeout was used for fences. This is now removed. Should there be a hard-coded fixed timeout for fences? # TODO # converting more pipeline handlers Only the uvcvideo pipeline handler is converted to use the buffer pool directly as an experiment, but more should be converted to really see if this public api is a good fit for pipeline handlers. Especially something like `imx8-isi` since that has a bit different architecture from the others. # minimum buffer count The applications need to know how many buffers should be kept in the pool for a given stream for continuous streaming. This should be a property or similar. # `FrameBuffer::Private::request()` After all pipeline handlers are migrated, the request member of `FrameBuffer` should probably be removed to fully decouple the two. --- Barnabás Pőcze (54): apps: cam: Simplify buffer reuse libcamera: request: Disassociate buffer when cancelling libcamera: pipeline: Replace open-coded request cancellation libcamera: pipeline: mali-c55: Remove `setRequest()` calls libcamera: pipeline: virtual: Make copy of request's buffer map libcamera: request: completeBuffer(): Emit `bufferCompleted` here libcamera: pipeline_handler: completeBuffer(): Inline and `static` v4l2: v4l2_camera: Avoid a level of indirection v4l2: v4l2_camera: Remove repated index checks v4l2: v4l2_camera_proxy: Remove `bufferCount_` v4l2: v4l2_camera: Use actually allocated buffer count v4l2: v4l2_camera: Use buffer cookie for indexing v4l2: v4l2_camera: Rename `Buffer` to `CompletedBuffer` v4l2: v4l2_camera: Always clear pending requests v4l2: v4l2_camera: Clear completed requests when stopping v4l2: v4l2_camera: Provide buffers one by one test: fence: Disable temporarily libcamera: request: Remove `ReuseBuffers` libcamera: framebuffer: request(): Move to private type libcamera: framebuffer: Store associated Stream libcamera: camera: Add `StreamData` libcamera: camera: Add buffer pool libcamera: request: addBuffer(): Remove impl libcamera: request: Remove fence support libcamera: request: Store count of pending buffers libcamera: request: doCancelRequest(): Remove libcamera: pipeline_handler: Move constructor options to a separate type libcamera: pipeline_handler: Acquire buffers if not using pool libcamera: request: enableStream(): Add libcamera: camera: acquireBuffer(): Add libcamera: camera: rejectBuffer(): Add libcamera: pipeline_handler: buffersAddedDevice(): New virtual function libcamera: pipeline_handler: Use `std::deque` libcamera: pipeline_handler: completeRequest(): Return request count libcamera: pipeline: uvcvideo: Use buffer pool prototype libcamera: request: completeBuffer(): Emit `bufferCompleted` last libcamera: camera: bufferCompleted: Pass `Stream` as well libcamera: camera: queueRequest(): Adjust buffer map empty error message libcamera: camera: queueBuffer(): Reject if it has buffers apps: cam: Use camera buffer pool app: lc-compliance: Use camera buffer pool apps: lc-compliance: Add buffer pool tests test: Use camera buffer pool apps: qcam: Use camera buffer pool v4l2: Use camera buffer pool py: Use camera buffer pool gstreamer: Use camera buffer pool libcamera: request: Remove `ReuseFlag` android: Update imported files android: camera_request: Add helper for buffer conversion android: camera_device: Move fence restoration into `prepareToReturn()` android: Use camera buffer pool libcamera: request: Remove `addBuffer()` test: fence: Enable .../guides/application-developer.rst | 9 - Documentation/guides/pipeline-handler.rst | 10 +- .../libhardware/include/hardware/camera3.h | 360 +- .../include/hardware/camera_common.h | 304 +- .../libhardware/include/hardware/gralloc.h | 99 +- .../android/metadata/camera_metadata_hidden.h | 11 +- .../android/metadata/system/camera_metadata.h | 41 +- .../metadata/system/camera_metadata_tags.h | 633 +- .../metadata/system/camera_vendor_tags.h | 10 +- .../android/system/core/include/android/log.h | 450 +- .../core/include/cutils/native_handle.h | 62 +- .../system/core/include/system/camera.h | 4 +- .../core/include/system/graphics-base-v1.2.h | 36 + .../core/include/system/graphics-base.h | 1 + .../system/core/include/system/graphics.h | 10 +- include/libcamera/camera.h | 5 +- include/libcamera/framebuffer.h | 1 - include/libcamera/internal/camera.h | 46 +- include/libcamera/internal/framebuffer.h | 6 + include/libcamera/internal/pipeline_handler.h | 31 +- include/libcamera/internal/request.h | 24 +- include/libcamera/request.h | 10 +- src/android/camera3_hal.cpp | 3 + src/android/camera_capabilities.cpp | 3 + src/android/camera_device.cpp | 287 +- src/android/camera_device.h | 4 + src/android/camera_hal_manager.cpp | 2 +- src/android/camera_ops.cpp | 16 +- src/android/camera_request.cpp | 53 +- src/android/camera_request.h | 11 +- src/android/metadata/camera_metadata.c | 183 +- .../metadata/camera_metadata_tag_info.c | 5478 ++++++++++++++++- src/apps/cam/camera_session.cpp | 36 +- src/apps/cam/camera_session.h | 2 +- src/apps/lc-compliance/helpers/capture.cpp | 23 +- src/apps/lc-compliance/tests/capture_test.cpp | 261 + src/apps/qcam/main_window.cpp | 82 +- src/apps/qcam/main_window.h | 1 - src/gstreamer/gstlibcamerasrc.cpp | 136 +- src/libcamera/camera.cpp | 164 +- src/libcamera/fence.cpp | 15 +- src/libcamera/framebuffer.cpp | 57 +- src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 5 +- src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +- src/libcamera/pipeline/mali-c55/mali-c55.cpp | 7 +- src/libcamera/pipeline/mali-c55/rzg2l-cru.cpp | 3 +- src/libcamera/pipeline/mali-c55/rzg2l-cru.h | 3 +- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +- .../pipeline/rpi/common/pipeline_base.cpp | 16 +- .../pipeline/rpi/common/pipeline_base.h | 2 +- src/libcamera/pipeline/simple/simple.cpp | 17 +- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 72 +- src/libcamera/pipeline/vimc/vimc.cpp | 11 +- src/libcamera/pipeline/virtual/virtual.cpp | 44 +- src/libcamera/pipeline_handler.cpp | 260 +- src/libcamera/request.cpp | 290 +- src/py/cam/cam.py | 20 +- src/py/examples/simple-cam.py | 13 +- src/py/examples/simple-capture.py | 20 +- src/py/examples/simple-continuous-capture.py | 32 +- src/py/libcamera/py_main.cpp | 65 +- src/v4l2/v4l2_camera.cpp | 150 +- src/v4l2/v4l2_camera.h | 25 +- src/v4l2/v4l2_camera_proxy.cpp | 118 +- src/v4l2/v4l2_camera_proxy.h | 3 - test/camera/buffer_import.cpp | 22 +- test/camera/camera_reconfigure.cpp | 22 +- test/camera/capture.cpp | 24 +- test/camera/statemachine.cpp | 4 +- test/fence.cpp | 214 +- 70 files changed, 9182 insertions(+), 1283 deletions(-) create mode 100644 include/android/system/core/include/system/graphics-base-v1.2.h -- 2.54.0