[libcamera-devel] [PATCH] libcamera: camera: Constify controls argument to start()
Niklas Söderlund
niklas.soderlund at ragnatech.se
Fri Feb 19 15:23:05 CET 2021
Hi Laurent,
On 2021-02-19 12:13:18 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> On Thu, Feb 18, 2021 at 03:13:23PM +0100, Niklas Söderlund wrote:
> > On 2021-02-18 14:37:55 +0100, Jacopo Mondi wrote:
> > > On Thu, Feb 18, 2021 at 01:46:08PM +0100, Niklas Söderlund wrote:
> > > > On 2021-02-18 08:53:29 +0200, Laurent Pinchart wrote:
> > > > > The ControlList passed to the Camera::start() function isn't modified.
> > > > > Make it const.
> > > >
> > > > To align with convection elsewhere should we not make it a const
> > > > reference ?
> > >
> > > I think the idea is to be able to call Camera::start() without any
> > > argument. A cost & would force the caller to pass in an empty control
> > > list and would require all users to update as it's a mjor API change.
> >
> > That is a good point, but can't that be solved with a default argument?
>
> The ControlList is supposed to be constructed with a ControlInfoMap, so
> we can't default it I'm afraid.
>
> Given the use case of telling start() there's no control to be set, I
> think a pointer makes sense. If it was easier to create an empty control
> list maybe we could turn it into a reference with a default argument,
> but right now it would be a bit annoying I think. Maybe something to
> keep in mind when improving the control API ?
That is a good point, for this patch.
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> > > The patch looks good
> > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > > ---
> > > > > include/libcamera/camera.h | 2 +-
> > > > > include/libcamera/internal/pipeline_handler.h | 2 +-
> > > > > src/libcamera/camera.cpp | 2 +-
> > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++--
> > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 ++--
> > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 ++--
> > > > > src/libcamera/pipeline/simple/simple.cpp | 4 ++--
> > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 ++--
> > > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 ++--
> > > > > 9 files changed, 15 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > > > index bd81fb54502e..326b14d0ca01 100644
> > > > > --- a/include/libcamera/camera.h
> > > > > +++ b/include/libcamera/camera.h
> > > > > @@ -100,7 +100,7 @@ public:
> > > > > std::unique_ptr<Request> createRequest(uint64_t cookie = 0);
> > > > > int queueRequest(Request *request);
> > > > >
> > > > > - int start(ControlList *controls = nullptr);
> > > > > + int start(const ControlList *controls = nullptr);
> > > > > int stop();
> > > > >
> > > > > private:
> > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > > > > index d455d3c9dc4f..6aca0b46f43d 100644
> > > > > --- a/include/libcamera/internal/pipeline_handler.h
> > > > > +++ b/include/libcamera/internal/pipeline_handler.h
> > > > > @@ -76,7 +76,7 @@ public:
> > > > > virtual int exportFrameBuffers(Camera *camera, Stream *stream,
> > > > > std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
> > > > >
> > > > > - virtual int start(Camera *camera, ControlList *controls) = 0;
> > > > > + virtual int start(Camera *camera, const ControlList *controls) = 0;
> > > > > virtual void stop(Camera *camera) = 0;
> > > > >
> > > > > int queueRequest(Request *request);
> > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > > > index c9c79b91e3ae..84edbb8f494d 100644
> > > > > --- a/src/libcamera/camera.cpp
> > > > > +++ b/src/libcamera/camera.cpp
> > > > > @@ -1018,7 +1018,7 @@ int Camera::queueRequest(Request *request)
> > > > > * \retval -ENODEV The camera has been disconnected from the system
> > > > > * \retval -EACCES The camera is not in a state where it can be started
> > > > > */
> > > > > -int Camera::start(ControlList *controls)
> > > > > +int Camera::start(const ControlList *controls)
> > > > > {
> > > > > Private *const d = LIBCAMERA_D_PTR();
> > > > >
> > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > index 3e6b88af4188..76fc7efd408b 100644
> > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > @@ -126,7 +126,7 @@ public:
> > > > > int exportFrameBuffers(Camera *camera, Stream *stream,
> > > > > std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > > > >
> > > > > - int start(Camera *camera, ControlList *controls) override;
> > > > > + int start(Camera *camera, const ControlList *controls) override;
> > > > > void stop(Camera *camera) override;
> > > > >
> > > > > int queueRequestDevice(Camera *camera, Request *request) override;
> > > > > @@ -646,7 +646,7 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > -int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *controls)
> > > > > +int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
> > > > > {
> > > > > std::map<uint32_t, ControlInfoMap> entityControls;
> > > > > IPU3CameraData *data = cameraData(camera);
> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > index 15aa600ed581..46361bf3fa51 100644
> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > @@ -252,7 +252,7 @@ public:
> > > > > int exportFrameBuffers(Camera *camera, Stream *stream,
> > > > > std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > > > >
> > > > > - int start(Camera *camera, ControlList *controls) override;
> > > > > + int start(Camera *camera, const ControlList *controls) override;
> > > > > void stop(Camera *camera) override;
> > > > >
> > > > > int queueRequestDevice(Camera *camera, Request *request) override;
> > > > > @@ -760,7 +760,7 @@ int PipelineHandlerRPi::exportFrameBuffers([[maybe_unused]] Camera *camera, Stre
> > > > > return ret;
> > > > > }
> > > > >
> > > > > -int PipelineHandlerRPi::start(Camera *camera, ControlList *controls)
> > > > > +int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> > > > > {
> > > > > RPiCameraData *data = cameraData(camera);
> > > > > int ret;
> > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > index a794501a9c8d..538c01392293 100644
> > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > @@ -142,7 +142,7 @@ public:
> > > > > int exportFrameBuffers(Camera *camera, Stream *stream,
> > > > > std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > > > >
> > > > > - int start(Camera *camera, ControlList *controls) override;
> > > > > + int start(Camera *camera, const ControlList *controls) override;
> > > > > void stop(Camera *camera) override;
> > > > >
> > > > > int queueRequestDevice(Camera *camera, Request *request) override;
> > > > > @@ -745,7 +745,7 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > -int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *controls)
> > > > > +int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
> > > > > {
> > > > > RkISP1CameraData *data = cameraData(camera);
> > > > > int ret;
> > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > > > index 23320d2687e1..35cd34ddbf54 100644
> > > > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > > > @@ -125,7 +125,7 @@ public:
> > > > > int exportFrameBuffers(Camera *camera, Stream *stream,
> > > > > std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > > > >
> > > > > - int start(Camera *camera, ControlList *controls) override;
> > > > > + int start(Camera *camera, const ControlList *controls) override;
> > > > > void stop(Camera *camera) override;
> > > > >
> > > > > bool match(DeviceEnumerator *enumerator) override;
> > > > > @@ -641,7 +641,7 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,
> > > > > return data->video_->exportBuffers(count, buffers);
> > > > > }
> > > > >
> > > > > -int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] ControlList *controls)
> > > > > +int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
> > > > > {
> > > > > SimpleCameraData *data = cameraData(camera);
> > > > > V4L2VideoDevice *video = data->video_;
> > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > > index 08a594175091..031f96e28449 100644
> > > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > > @@ -72,7 +72,7 @@ public:
> > > > > int exportFrameBuffers(Camera *camera, Stream *stream,
> > > > > std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > > > >
> > > > > - int start(Camera *camera, ControlList *controls) override;
> > > > > + int start(Camera *camera, const ControlList *controls) override;
> > > > > void stop(Camera *camera) override;
> > > > >
> > > > > int queueRequestDevice(Camera *camera, Request *request) override;
> > > > > @@ -232,7 +232,7 @@ int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,
> > > > > return data->video_->exportBuffers(count, buffers);
> > > > > }
> > > > >
> > > > > -int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] ControlList *controls)
> > > > > +int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
> > > > > {
> > > > > UVCCameraData *data = cameraData(camera);
> > > > > unsigned int count = data->stream_.configuration().bufferCount;
> > > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > > > > index 78b47916b4db..57c65b021c46 100644
> > > > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > > > @@ -86,7 +86,7 @@ public:
> > > > > int exportFrameBuffers(Camera *camera, Stream *stream,
> > > > > std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > > > >
> > > > > - int start(Camera *camera, ControlList *controls) override;
> > > > > + int start(Camera *camera, const ControlList *controls) override;
> > > > > void stop(Camera *camera) override;
> > > > >
> > > > > int queueRequestDevice(Camera *camera, Request *request) override;
> > > > > @@ -307,7 +307,7 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream,
> > > > > return data->video_->exportBuffers(count, buffers);
> > > > > }
> > > > >
> > > > > -int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] ControlList *controls)
> > > > > +int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
> > > > > {
> > > > > VimcCameraData *data = cameraData(camera);
> > > > > unsigned int count = data->stream_.configuration().bufferCount;
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list