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

[Laurent Pinchart]

On Mon, Aug 21, 2023 at 10:24:20PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Gabby George (2023-08-21 14:10:38)
> > 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.
> > 
> > Signed-off-by: Gabby George <gabbymg94 at gmail.com>
> > ---
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 98 +++++++++++++++++++-
> >  1 file changed, 96 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index dbe0cc8c..c8d6633f 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,6 +52,9 @@ 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_;
> > +
> >         std::map<PixelFormat, std::vector<SizeRange>> formats_;
> >  
> >  private:
> > @@ -96,6 +100,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 +233,67 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> >                 return -EINVAL;
> >  
> >         cfg.setStream(&data->stream_);
> > +       return 0;
> > +}
> > +
> > +int PipelineHandlerUVC::cleanupMetadataBuffers(Camera *camera)
> > +{
> > +       int ret = 0;
> 
> You set this and never use it except to return zero. You can just make
> both this, and cleanup() 'void' return functions.
> 
> 
> > +       UVCCameraData *data = cameraData(camera);
> > +
> > +       if (data->metadata_)
> > +               data->metadata_->releaseBuffers();
> > +       data->metadataBuffers_.clear();
> > +       data->mappedMetadataBuffers_.clear();
> > +       data->metadata_ = nullptr;
> >  
> > +       return ret;
> > +}
> > +
> > +int PipelineHandlerUVC::cleanup(Camera *camera)

"cleanup" is an ambiguous name. It perform some cleanups associated with
stopping capture, but it could also be understood as cleaning up the
whole pipeline handler. releaseBuffers() could be a better name.
Similarly, you could name the previous function
releaseMetadataBuffers().

> > +{
> > +       UVCCameraData *data = cameraData(camera);
> > +       cleanupMetadataBuffers(camera);
> > +       data->video_->releaseBuffers();
> >         return 0;
> >  }
> >  
> > +/**
> > + * 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 MappedFrameBuffer::error()
> > + */
> > +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, Warning)
> > +                               << "Failed to mmap metadata buffer: "
> > +                               << strerror(mappedBuffer.error());
> > +                       return mappedBuffer.error();
> > +               }
> > +
> > +               data->mappedMetadataBuffers_.emplace(i, std::move(mappedBuffer));

This map key is the index in the metadataBuffers_ vector, is there a
reason to use a map instead of a vector ?

> > +               data->metadataBuffers_[i]->setCookie(i);
> > +       }
> > +       return ret;
> > +}
> > +
> >  int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,
> >                                            std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >  {
> > @@ -249,18 +314,47 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
> >  
> >         ret = data->video_->streamOn();
> >         if (ret < 0) {
> > -               data->video_->releaseBuffers();
> > +               cleanup(camera);
> >                 return ret;
> >         }
> >  
> > +       /*
> > +        * If metadata allocation fails, exit this function but
> > +        * do not return a failure as video started successfully.
> > +        * Fall back on using driver timestamps.
> > +        */
> > +       if (data->metadata_) {
> 
> Should metadata buffers be allocated and queued before calling streamOn
> on the main video device so that it is capable of handling metadata for
> the first frame? I think this section should all be above the
> data->video_->streamOn() above.

That sounds like a good idea.

> > +               if (createMetadataBuffers(camera, count) < 0 ||
> > +                   data->metadata_->streamOn()) {
> > +                       LOG(UVC, Warning)
> > +                               << "Metadata stream unavailable.  Using driver timestamps.";
> > +                       data->metadata_ = nullptr;
> > +                       return 0;
> > +               }
> > +
> > +               for (std::unique_ptr<FrameBuffer> &buf : data->metadataBuffers_) {
> > +                       ret = data->metadata_->queueBuffer(buf.get());
> > +                       if (ret < 0) {
> > +                               LOG(UVC, Warning)
> > +                                       << "Metadata stream unavailable.  Using driver timestamps.";
> > +                               cleanupMetadataBuffers(camera);
> > +                               return 0;
> > +                       }
> > +               }
> > +       }

Missing blank line.

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

-- 
Regards,

Laurent Pinchart