[libcamera-devel] [PATCH/RFC 06/12] libcamera: pipeline: Move camera data classes to the top level scope
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat May 18 19:27:45 CEST 2019
Hi Niklas,
On Sat, May 18, 2019 at 06:46:42PM +0200, Niklas Söderlund wrote:
> On 2019-05-18 02:06:15 +0300, Laurent Pinchart wrote:
> > Move the pipeline handler camera data classes, defined in the scope of
> > the respective pipeline handler class, to the top level of the libcamera
> > namespace. This prepares for the introduction of other classes that will
> > make use of them in the IPU3 and RkISP1 pipeline handlers. The UVC and
> > VIMC pipeline handlers are updated as well for consistency.
>
> Nit-pick/future work:
> What do we think about not splitting class definition and function
> implementations for the CameraData implementations? The classes are
> defined in a cpp file so are there any compelling reason to split the
> two for small to mid size classes?
When you inline the implementation inside the class definition, the
method becomes an inline from a C point of view, which means it could
get duplicated in every call site. Furthermore, especially for the IPU3
and RkISP1 pipeline handlers, I foresee a need to split the code in
multiple files at some point, so we would need to move the class
definition to a header.
> If we think a pipeline will grow to span multiple files I can see the
> point of keeping them split, but for uvc and vimc that will be a single
> file pipeline I think we could do without the split.
It's mostly the inline part that makes me prefer not inlining the
definition of methods, even for UVC and VIMC.
> With or without this addressed,
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 44 ++++++++++++------------
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 34 +++++++++---------
> > src/libcamera/pipeline/uvcvideo.cpp | 40 ++++++++++-----------
> > src/libcamera/pipeline/vimc.cpp | 40 ++++++++++-----------
> > 4 files changed, 79 insertions(+), 79 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 7c6f2d4a23be..8430e0591a41 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -145,6 +145,25 @@ public:
> > ImgUDevice::ImgUOutput *device_;
> > };
> >
> > +class IPU3CameraData : public CameraData
> > +{
> > +public:
> > + IPU3CameraData(PipelineHandler *pipe)
> > + : CameraData(pipe)
> > + {
> > + }
> > +
> > + void imguOutputBufferReady(Buffer *buffer);
> > + void imguInputBufferReady(Buffer *buffer);
> > + void cio2BufferReady(Buffer *buffer);
> > +
> > + CIO2Device cio2_;
> > + ImgUDevice *imgu_;
> > +
> > + IPU3Stream outStream_;
> > + IPU3Stream vfStream_;
> > +};
> > +
> > class PipelineHandlerIPU3 : public PipelineHandler
> > {
> > public:
> > @@ -167,25 +186,6 @@ public:
> > bool match(DeviceEnumerator *enumerator) override;
> >
> > private:
> > - class IPU3CameraData : public CameraData
> > - {
> > - public:
> > - IPU3CameraData(PipelineHandler *pipe)
> > - : CameraData(pipe)
> > - {
> > - }
> > -
> > - void imguOutputBufferReady(Buffer *buffer);
> > - void imguInputBufferReady(Buffer *buffer);
> > - void cio2BufferReady(Buffer *buffer);
> > -
> > - CIO2Device cio2_;
> > - ImgUDevice *imgu_;
> > -
> > - IPU3Stream outStream_;
> > - IPU3Stream vfStream_;
> > - };
> > -
> > static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> >
> > IPU3CameraData *cameraData(const Camera *camera)
> > @@ -749,7 +749,7 @@ int PipelineHandlerIPU3::registerCameras()
> > * Buffers completed from the ImgU input are immediately queued back to the
> > * CIO2 unit to continue frame capture.
> > */
> > -void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)
> > +void IPU3CameraData::imguInputBufferReady(Buffer *buffer)
> > {
> > cio2_.output_->queueBuffer(buffer);
> > }
> > @@ -760,7 +760,7 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)
> > *
> > * Buffers completed from the ImgU output are directed to the application.
> > */
> > -void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
> > +void IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
> > {
> > Request *request = buffer->request();
> >
> > @@ -785,7 +785,7 @@ void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
> > * Buffers completed from the CIO2 are immediately queued to the ImgU unit
> > * for further processing.
> > */
> > -void PipelineHandlerIPU3::IPU3CameraData::cio2BufferReady(Buffer *buffer)
> > +void IPU3CameraData::cio2BufferReady(Buffer *buffer)
> > {
> > imgu_->input_->queueBuffer(buffer);
> > }
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index c3b3912c96f3..ec590a382751 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -28,6 +28,23 @@ namespace libcamera {
> >
> > LOG_DEFINE_CATEGORY(RkISP1)
> >
> > +class RkISP1CameraData : public CameraData
> > +{
> > +public:
> > + RkISP1CameraData(PipelineHandler *pipe)
> > + : CameraData(pipe), sensor_(nullptr)
> > + {
> > + }
> > +
> > + ~RkISP1CameraData()
> > + {
> > + delete sensor_;
> > + }
> > +
> > + Stream stream_;
> > + CameraSensor *sensor_;
> > +};
> > +
> > class PipelineHandlerRkISP1 : public PipelineHandler
> > {
> > public:
> > @@ -51,23 +68,6 @@ public:
> > bool match(DeviceEnumerator *enumerator) override;
> >
> > private:
> > - class RkISP1CameraData : public CameraData
> > - {
> > - public:
> > - RkISP1CameraData(PipelineHandler *pipe)
> > - : CameraData(pipe), sensor_(nullptr)
> > - {
> > - }
> > -
> > - ~RkISP1CameraData()
> > - {
> > - delete sensor_;
> > - }
> > -
> > - Stream stream_;
> > - CameraSensor *sensor_;
> > - };
> > -
> > static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> >
> > RkISP1CameraData *cameraData(const Camera *camera)
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 5c061ca61016..c20467766ed0 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -20,6 +20,25 @@ namespace libcamera {
> >
> > LOG_DEFINE_CATEGORY(UVC)
> >
> > +class UVCCameraData : public CameraData
> > +{
> > +public:
> > + UVCCameraData(PipelineHandler *pipe)
> > + : CameraData(pipe), video_(nullptr)
> > + {
> > + }
> > +
> > + ~UVCCameraData()
> > + {
> > + delete video_;
> > + }
> > +
> > + void bufferReady(Buffer *buffer);
> > +
> > + V4L2Device *video_;
> > + Stream stream_;
> > +};
> > +
> > class PipelineHandlerUVC : public PipelineHandler
> > {
> > public:
> > @@ -42,25 +61,6 @@ public:
> > bool match(DeviceEnumerator *enumerator) override;
> >
> > private:
> > - class UVCCameraData : public CameraData
> > - {
> > - public:
> > - UVCCameraData(PipelineHandler *pipe)
> > - : CameraData(pipe), video_(nullptr)
> > - {
> > - }
> > -
> > - ~UVCCameraData()
> > - {
> > - delete video_;
> > - }
> > -
> > - void bufferReady(Buffer *buffer);
> > -
> > - V4L2Device *video_;
> > - Stream stream_;
> > - };
> > -
> > UVCCameraData *cameraData(const Camera *camera)
> > {
> > return static_cast<UVCCameraData *>(
> > @@ -206,7 +206,7 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> > return true;
> > }
> >
> > -void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer)
> > +void UVCCameraData::bufferReady(Buffer *buffer)
> > {
> > Request *request = queuedRequests_.front();
> >
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 0ece97f09e7e..5575880cdbdf 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -20,6 +20,25 @@ namespace libcamera {
> >
> > LOG_DEFINE_CATEGORY(VIMC)
> >
> > +class VimcCameraData : public CameraData
> > +{
> > +public:
> > + VimcCameraData(PipelineHandler *pipe)
> > + : CameraData(pipe)
> > + {
> > + }
> > +
> > + ~VimcCameraData()
> > + {
> > + delete video_;
> > + }
> > +
> > + void bufferReady(Buffer *buffer);
> > +
> > + V4L2Device *video_;
> > + Stream stream_;
> > +};
> > +
> > class PipelineHandlerVimc : public PipelineHandler
> > {
> > public:
> > @@ -42,25 +61,6 @@ public:
> > bool match(DeviceEnumerator *enumerator) override;
> >
> > private:
> > - class VimcCameraData : public CameraData
> > - {
> > - public:
> > - VimcCameraData(PipelineHandler *pipe)
> > - : CameraData(pipe)
> > - {
> > - }
> > -
> > - ~VimcCameraData()
> > - {
> > - delete video_;
> > - }
> > -
> > - void bufferReady(Buffer *buffer);
> > -
> > - V4L2Device *video_;
> > - Stream stream_;
> > - };
> > -
> > VimcCameraData *cameraData(const Camera *camera)
> > {
> > return static_cast<VimcCameraData *>(
> > @@ -202,7 +202,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> > return true;
> > }
> >
> > -void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer)
> > +void VimcCameraData::bufferReady(Buffer *buffer)
> > {
> > Request *request = queuedRequests_.front();
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list