[libcamera-devel] [RFC PATCH 4/5] libcamera: pipeline: uvcvideo: Allocate metadata buffers

[Kieran Bingham]

Quoting Gabby George (2023-08-14 12:28:48)
> Perform the allocation and mapping of metadata buffers into
> libcamera's virtual address space.  UVC metadata buffers cannot be
> exported as DMA buffer file descriptors, so use the MappedFrameBuffer
> class to map them into memory directly.  This will give the UVC
> pipeline access to buffer data to extract timestamp information.
> 
> Metadata buffers are internal to the UVC pipeline, so buffer memory
> should not be exposed to the user.

Sounds reasonable.


> 
> Signed-off-by: Gabby George <gabbymg94 at gmail.com>
> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 97 +++++++++++++++++++-
>  1 file changed, 95 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 4470d8a2..51f30187 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -24,6 +24,7 @@
>  
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/device_enumerator.h"
> +#include "libcamera/internal/mapped_framebuffer.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/sysfs.h"
> @@ -51,11 +52,17 @@ public:
>         std::unique_ptr<V4L2VideoDevice> video_;
>         std::unique_ptr<V4L2VideoDevice> metadata_;
>         Stream stream_;
> +       std::vector<std::unique_ptr<FrameBuffer>> metadataBuffers_;
> +       std::map<unsigned int, MappedFrameBuffer> mappedMetadataBuffers_;
> +       bool useMetadataStream_;

I wonder if we'll need this bool. Just from reading these 6 lines, I
could probably assume we could replace anything that has 
	if (useMoetadataStream_)
with
	if (metadata_)



> +
>         std::map<PixelFormat, std::vector<SizeRange>> formats_;
>  
>  private:
>         int initMetadata(MediaDevice *media);
>  
> +       const unsigned int minLengthHeaderBuf_ = 10;
> +
>         bool generateId();
>  
>         std::string id_;
> @@ -96,6 +103,10 @@ private:
>                            const ControlValue &value);
>         int processControls(UVCCameraData *data, Request *request);
>  
> +       int createMetadataBuffers(Camera *camera, unsigned int count);
> +       int cleanupMetadataBuffers(Camera *camera);
> +       int cleanup(Camera *camera);
> +
>         UVCCameraData *cameraData(Camera *camera)
>         {
>                 return static_cast<UVCCameraData *>(camera->_d());
> @@ -225,10 +236,66 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
>                 return -EINVAL;
>  
>         cfg.setStream(&data->stream_);
> +       return 0;
> +}
>  
> +int PipelineHandlerUVC::cleanupMetadataBuffers(Camera *camera)
> +{
> +       int ret = 0;
> +       UVCCameraData *data = cameraData(camera);
> +
> +       ret = data->metadata_->releaseBuffers();
> +       data->metadataBuffers_.clear(); //call the destructor for the frame buffers

I think we can remove the comment on that line.

> +       data->mappedMetadataBuffers_.clear();
> +       data->useMetadataStream_ = false;
> +
> +       return ret;
> +}
> +
> +int PipelineHandlerUVC::cleanup(Camera *camera)
> +{
> +       UVCCameraData *data = cameraData(camera);
> +       cleanupMetadataBuffers(camera);
> +       data->video_->releaseBuffers();
>         return 0;
>  }
>  
> +/*

Open a block comment with
 	/**

if it's a documentation comment for doxygen.


> + * UVC Metadata stream does not support exporting buffers via EXPBUF,
> + * so it is necessary to create and store mmap-ed addresses.
> + * Metadata buffers are internal to libcamera. They are not, and
> + * cannot be, exposed to the user.
> + *
> + * Returns the number of buffers allocated and mapped.
> + *
> + * \return The number of buffers allocated, or a negative error code if
> + * the number of buffers allocated was not equal to "count"
> + * \retval -EINVAL if "count" buffers were not successfully allocated.
> + * \retval -ENOMEM if mmap failed.

Hrm ... Have you checked if this is what MappedFrameBuffer.error() will
return? Maybe this should refer to saying 'MappedFrameBuffer::error()'
if the Frame could not be mapped successfully ...

> + */
> +int PipelineHandlerUVC::createMetadataBuffers(Camera *camera, unsigned int count)
> +{
> +       UVCCameraData *data = cameraData(camera);
> +       int ret = data->metadata_->allocateBuffers(count, &data->metadataBuffers_);
> +       if (ret < 0)
> +               return -EINVAL;
> +
> +       for (unsigned int i = 0; i < count; i++) {
> +               MappedFrameBuffer mappedBuffer(data->metadataBuffers_[i].get(),
> +                                              MappedFrameBuffer::MapFlag::Read, true);
> +               if (!mappedBuffer.isValid()) {
> +                       LOG(UVC, Error)
> +                               << "Failed to mmap metadata buffer: "
> +                               << strerror(mappedBuffer.error());
> +                       return mappedBuffer.error();
> +               }
> +
> +               data->mappedMetadataBuffers_.emplace(i, std::move(mappedBuffer));
> +               data->metadataBuffers_[i]->setCookie(i);
> +       }
> +       return ret;
> +}
> +
>  int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,
>                                            std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
> @@ -247,20 +314,46 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
>         if (ret < 0)
>                 return ret;
>  
> +       if (data->useMetadataStream_) {
> +               if (createMetadataBuffers(camera, count) < 0) {
> +                       LOG(UVC, Error) << "Unable to allocate buffers for UVC metadata stream.";
> +                       data->useMetadataStream_ = false;
> +               }
> +       }
> +
>         ret = data->video_->streamOn();
>         if (ret < 0) {
> -               data->video_->releaseBuffers();
> +               cleanup(camera);
>                 return ret;
>         }
>  
> +       if (data->useMetadataStream_) {
> +               ret = data->metadata_->streamOn();
> +               if (ret) {
> +                       LOG(UVC, Error) << "Failed to start metadata stream";
> +                       return ret;
> +               }
> +
> +               for (std::unique_ptr<FrameBuffer> &buf : data->metadataBuffers_) {
> +                       ret = data->metadata_->queueBuffer(buf.get());
> +                       if (ret < 0) {
> +                               cleanupMetadataBuffers(camera);
> +                               return ret;
> +                       }
> +               }

Can all of this be done before video_->streamOn() to avoid having two
conditionals that check if we useMetadataStream_ ?


> +       }
>         return 0;
>  }
>  
>  void PipelineHandlerUVC::stopDevice(Camera *camera)
>  {
>         UVCCameraData *data = cameraData(camera);
> +
>         data->video_->streamOff();
> -       data->video_->releaseBuffers();
> +
> +       data->metadata_->streamOff();
> +
> +       cleanup(camera);
>  }
>  
>  int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> -- 
> 2.34.1
>