[v1] libcamera: pipeline: Put single file pipelines into anonymous namespace
diff mbox series

Message ID 20250314174200.1015493-1-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • [v1] libcamera: pipeline: Put single file pipelines into anonymous namespace
Related show

Commit Message

Barnabás Pőcze March 14, 2025, 5:42 p.m. UTC
Putting a symbol into an anonymous namespace gives its internal linkage.
This is useful for a couple reasons:
  * the compiler can do more optimizations (e.g. inlining);
  * fewer symbols are exported from the DSO;
  * unused symbols can be better diagnosed.

For example, as it turns out, `PipelineHandlerMaliC55::applyScalerCrop()`
is not actually used, which was not diagnosed previously.

Similarly, `ISICameraData::pipe()` is also not used, it is removed.

This could be done with the other pipeline handlers, but is not done,
due to the issues caused by the log category symbols and to avoid
inconsistencies where half of a pipeline handler is in one namespace
while the other half is somewhere else.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/libcamera/pipeline/imx8-isi/imx8-isi.cpp |  13 +--
 src/libcamera/pipeline/mali-c55/mali-c55.cpp | 104 +------------------
 src/libcamera/pipeline/simple/simple.cpp     |   6 +-
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp |   6 +-
 src/libcamera/pipeline/vimc/vimc.cpp         |   6 +-
 5 files changed, 20 insertions(+), 115 deletions(-)

--
2.48.1

Comments

Jacopo Mondi March 17, 2025, 7:12 p.m. UTC | #1
On Fri, Mar 14, 2025 at 06:42:00PM +0100, Barnabás Pőcze wrote:
> Putting a symbol into an anonymous namespace gives its internal linkage.
> This is useful for a couple reasons:
>   * the compiler can do more optimizations (e.g. inlining);
>   * fewer symbols are exported from the DSO;
>   * unused symbols can be better diagnosed.
>
> For example, as it turns out, `PipelineHandlerMaliC55::applyScalerCrop()`
> is not actually used, which was not diagnosed previously.
>
> Similarly, `ISICameraData::pipe()` is also not used, it is removed.
>
> This could be done with the other pipeline handlers, but is not done,
> due to the issues caused by the log category symbols and to avoid
> inconsistencies where half of a pipeline handler is in one namespace
> while the other half is somewhere else.

I'm just afraid once these pipelines grow to be larger than a single
file, whoever will modify this in future might be a bit confused ?

Can we comment that we can only do this for single file pipelines ?
But then, you know, some pipeline handlers are anonymized some are
not, not sure, I'm a bit in two minds here.

Let's see what others think..

>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp |  13 +--
>  src/libcamera/pipeline/mali-c55/mali-c55.cpp | 104 +------------------
>  src/libcamera/pipeline/simple/simple.cpp     |   6 +-
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp |   6 +-
>  src/libcamera/pipeline/vimc/vimc.cpp         |   6 +-
>  5 files changed, 20 insertions(+), 115 deletions(-)
>
> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> index 4e66b3368..421328536 100644
> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> @@ -31,7 +31,9 @@
>
>  #include "linux/media-bus-format.h"
>
> -namespace libcamera {
> +namespace {
> +
> +using namespace libcamera;
>
>  LOG_DEFINE_CATEGORY(ISI)
>
> @@ -50,8 +52,6 @@ public:
>  		streams_.resize(2);
>  	}
>
> -	PipelineHandlerISI *pipe();
> -
>  	int init();
>
>  	unsigned int pipeIndex(const Stream *stream)
> @@ -149,11 +149,6 @@ private:
>   * Camera Data
>   */
>
> -PipelineHandlerISI *ISICameraData::pipe()
> -{
> -	return static_cast<PipelineHandlerISI *>(Camera::Private::pipe());
> -}
> -
>  /* Open and initialize pipe components. */
>  int ISICameraData::init()
>  {
> @@ -1113,4 +1108,4 @@ void PipelineHandlerISI::bufferReady(FrameBuffer *buffer)
>
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerISI, "imx8-isi")
>
> -} /* namespace libcamera */
> +} /* namespace */
> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> index a05e11fcc..17cae9b96 100644
> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> @@ -52,7 +52,9 @@ bool isFormatRaw(const libcamera::PixelFormat &pixFmt)
>
>  } /* namespace */
>
> -namespace libcamera {
> +namespace {
> +
> +using namespace libcamera;
>
>  LOG_DEFINE_CATEGORY(MaliC55)
>
> @@ -677,8 +679,6 @@ private:
>  				     const StreamConfiguration &config,
>  				     V4L2SubdeviceFormat &subdevFormat);
>
> -	void applyScalerCrop(Camera *camera, const ControlList &controls);
> -
>  	bool registerMaliCamera(std::unique_ptr<MaliC55CameraData> data,
>  				const std::string &name);
>  	bool registerTPGCamera(MediaLink *link);
> @@ -1270,102 +1270,6 @@ void PipelineHandlerMaliC55::stopDevice(Camera *camera)
>  	freeBuffers(camera);
>  }
>
> -void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera,
> -					     const ControlList &controls)
> -{
> -	MaliC55CameraData *data = cameraData(camera);
> -
> -	const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop);
> -	if (!scalerCrop)
> -		return;
> -
> -	if (!data->sensor_) {
> -		LOG(MaliC55, Error) << "ScalerCrop not supported for TPG";
> -		return;
> -	}
> -
> -	Rectangle nativeCrop = *scalerCrop;
> -
> -	IPACameraSensorInfo sensorInfo;
> -	int ret = data->sensor_->sensorInfo(&sensorInfo);
> -	if (ret) {
> -		LOG(MaliC55, Error) << "Failed to retrieve sensor info";
> -		return;
> -	}
> -
> -	/*
> -	 * The ScalerCrop rectangle re-scaling in the ISP crop rectangle
> -	 * comes straight from the RPi pipeline handler.
> -	 *
> -	 * Create a version of the crop rectangle aligned to the analogue crop
> -	 * rectangle top-left coordinates and scaled in the [analogue crop to
> -	 * output frame] ratio to take into account binning/skipping on the
> -	 * sensor.
> -	 */
> -	Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop
> -							       .topLeft());
> -	ispCrop.scaleBy(sensorInfo.outputSize, sensorInfo.analogCrop.size());
> -
> -	/*
> -	 * The crop rectangle should be:
> -	 * 1. At least as big as ispMinCropSize_, once that's been
> -	 *    enlarged to the same aspect ratio.
> -	 * 2. With the same mid-point, if possible.
> -	 * 3. But it can't go outside the sensor area.
> -	 */
> -	Rectangle ispMinCrop{ 0, 0, 640, 480 };
> -	Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size());
> -	Size size = ispCrop.size().expandedTo(minSize);
> -	ispCrop = size.centeredTo(ispCrop.center())
> -		      .enclosedIn(Rectangle(sensorInfo.outputSize));
> -
> -	/*
> -	 * As the resizer can't upscale, the crop rectangle has to be larger
> -	 * than the larger stream output size.
> -	 */
> -	Size maxYuvSize;
> -	for (MaliC55Pipe &pipe : pipes_) {
> -		if (!pipe.stream)
> -			continue;
> -
> -		const StreamConfiguration &config = pipe.stream->configuration();
> -		if (isFormatRaw(config.pixelFormat)) {
> -			LOG(MaliC55, Debug) << "Cannot crop with a RAW stream";
> -			return;
> -		}
> -
> -		Size streamSize = config.size;
> -		if (streamSize.width > maxYuvSize.width)
> -			maxYuvSize.width = streamSize.width;
> -		if (streamSize.height > maxYuvSize.height)
> -			maxYuvSize.height = streamSize.height;
> -	}
> -
> -	ispCrop.size().expandTo(maxYuvSize);
> -
> -	/*
> -	 * Now apply the scaler crop to each enabled output. This overrides the
> -	 * crop configuration performed at configure() time and can cause
> -	 * square pixels if the crop rectangle and scaler output FOV ratio are
> -	 * different.
> -	 */
> -	for (MaliC55Pipe &pipe : pipes_) {
> -		if (!pipe.stream)
> -			continue;
> -
> -		/* Create a copy to avoid setSelection() to modify ispCrop. */
> -		Rectangle pipeCrop = ispCrop;
> -		ret = pipe.resizer->setSelection(0, V4L2_SEL_TGT_CROP, &pipeCrop);
> -		if (ret) {
> -			LOG(MaliC55, Error)
> -				<< "Failed to apply crop to "
> -				<< (pipe.stream == &data->frStream_ ?
> -				    "FR" : "DS") << " pipe";
> -			return;
> -		}
> -	}
> -}
> -
>  int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)
>  {
>  	MaliC55CameraData *data = cameraData(camera);
> @@ -1752,4 +1656,4 @@ bool PipelineHandlerMaliC55::match(DeviceEnumerator *enumerator)
>
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerMaliC55, "mali-c55")
>
> -} /* namespace libcamera */
> +} /* namespace */
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 6e039bf35..4d9943d61 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -41,7 +41,9 @@
>  #include "libcamera/internal/v4l2_subdevice.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
>
> -namespace libcamera {
> +namespace {
> +
> +using namespace libcamera;
>
>  LOG_DEFINE_CATEGORY(SimplePipeline)
>
> @@ -1751,4 +1753,4 @@ void SimplePipelineHandler::releasePipeline(SimpleCameraData *data)
>
>  REGISTER_PIPELINE_HANDLER(SimplePipelineHandler, "simple")
>
> -} /* namespace libcamera */
> +} /* namespace */
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 7470b5627..dedcac89b 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -32,7 +32,9 @@
>  #include "libcamera/internal/sysfs.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
>
> -namespace libcamera {
> +namespace {
> +
> +using namespace libcamera;
>
>  LOG_DEFINE_CATEGORY(UVC)
>
> @@ -804,4 +806,4 @@ void UVCCameraData::imageBufferReady(FrameBuffer *buffer)
>
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC, "uvcvideo")
>
> -} /* namespace libcamera */
> +} /* namespace */
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 07273bd2b..f86ee5206 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -42,7 +42,9 @@
>  #include "libcamera/internal/v4l2_subdevice.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
>
> -namespace libcamera {
> +namespace {
> +
> +using namespace libcamera;
>
>  LOG_DEFINE_CATEGORY(VIMC)
>
> @@ -646,4 +648,4 @@ void VimcCameraData::paramsComputed([[maybe_unused]] unsigned int id,
>
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc, "vimc")
>
> -} /* namespace libcamera */
> +} /* namespace */
> --
> 2.48.1
Kieran Bingham March 18, 2025, 11:25 a.m. UTC | #2
Quoting Jacopo Mondi (2025-03-17 19:12:15)
> On Fri, Mar 14, 2025 at 06:42:00PM +0100, Barnabás Pőcze wrote:
> > Putting a symbol into an anonymous namespace gives its internal linkage.
> > This is useful for a couple reasons:
> >   * the compiler can do more optimizations (e.g. inlining);
> >   * fewer symbols are exported from the DSO;
> >   * unused symbols can be better diagnosed.
> >
> > For example, as it turns out, `PipelineHandlerMaliC55::applyScalerCrop()`
> > is not actually used, which was not diagnosed previously.
> >
> > Similarly, `ISICameraData::pipe()` is also not used, it is removed.

If we've got unused/dead code I think that should be removed in a
dedicated patch of it's own.

> > This could be done with the other pipeline handlers, but is not done,
> > due to the issues caused by the log category symbols and to avoid
> > inconsistencies where half of a pipeline handler is in one namespace
> > while the other half is somewhere else.
> 
> I'm just afraid once these pipelines grow to be larger than a single
> file, whoever will modify this in future might be a bit confused ?
> 
> Can we comment that we can only do this for single file pipelines ?
> But then, you know, some pipeline handlers are anonymized some are
> not, not sure, I'm a bit in two minds here.
> 
> Let's see what others think..

I don't object to simplifying the namespace of the pipeline handler
internals. The rest of libcamera shouldn't be accessing internals of a
pipeline ...

Perhaps even pipeline handlers might get smaller if we can progress
towards 'modular pipeline handlers' with more common code abstracted
anyway ...



> 
> >
> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp |  13 +--
> >  src/libcamera/pipeline/mali-c55/mali-c55.cpp | 104 +------------------
> >  src/libcamera/pipeline/simple/simple.cpp     |   6 +-
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp |   6 +-
> >  src/libcamera/pipeline/vimc/vimc.cpp         |   6 +-
> >  5 files changed, 20 insertions(+), 115 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > index 4e66b3368..421328536 100644
> > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > @@ -31,7 +31,9 @@
> >
> >  #include "linux/media-bus-format.h"
> >
> > -namespace libcamera {
> > +namespace {
> > +
> > +using namespace libcamera;
> >
> >  LOG_DEFINE_CATEGORY(ISI)
> >
> > @@ -50,8 +52,6 @@ public:
> >               streams_.resize(2);
> >       }
> >
> > -     PipelineHandlerISI *pipe();
> > -
> >       int init();
> >
> >       unsigned int pipeIndex(const Stream *stream)
> > @@ -149,11 +149,6 @@ private:
> >   * Camera Data
> >   */
> >
> > -PipelineHandlerISI *ISICameraData::pipe()
> > -{
> > -     return static_cast<PipelineHandlerISI *>(Camera::Private::pipe());
> > -}
> > -
> >  /* Open and initialize pipe components. */
> >  int ISICameraData::init()
> >  {
> > @@ -1113,4 +1108,4 @@ void PipelineHandlerISI::bufferReady(FrameBuffer *buffer)
> >
> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerISI, "imx8-isi")
> >
> > -} /* namespace libcamera */
> > +} /* namespace */
> > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > index a05e11fcc..17cae9b96 100644
> > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > @@ -52,7 +52,9 @@ bool isFormatRaw(const libcamera::PixelFormat &pixFmt)
> >
> >  } /* namespace */
> >
> > -namespace libcamera {
> > +namespace {
> > +
> > +using namespace libcamera;
> >
> >  LOG_DEFINE_CATEGORY(MaliC55)
> >
> > @@ -677,8 +679,6 @@ private:
> >                                    const StreamConfiguration &config,
> >                                    V4L2SubdeviceFormat &subdevFormat);
> >
> > -     void applyScalerCrop(Camera *camera, const ControlList &controls);
> > -
> >       bool registerMaliCamera(std::unique_ptr<MaliC55CameraData> data,
> >                               const std::string &name);
> >       bool registerTPGCamera(MediaLink *link);
> > @@ -1270,102 +1270,6 @@ void PipelineHandlerMaliC55::stopDevice(Camera *camera)
> >       freeBuffers(camera);
> >  }
> >
> > -void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera,
> > -                                          const ControlList &controls)
> > -{
> > -     MaliC55CameraData *data = cameraData(camera);
> > -
> > -     const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop);
> > -     if (!scalerCrop)
> > -             return;
> > -
> > -     if (!data->sensor_) {
> > -             LOG(MaliC55, Error) << "ScalerCrop not supported for TPG";
> > -             return;
> > -     }
> > -
> > -     Rectangle nativeCrop = *scalerCrop;
> > -
> > -     IPACameraSensorInfo sensorInfo;
> > -     int ret = data->sensor_->sensorInfo(&sensorInfo);
> > -     if (ret) {
> > -             LOG(MaliC55, Error) << "Failed to retrieve sensor info";
> > -             return;
> > -     }
> > -
> > -     /*
> > -      * The ScalerCrop rectangle re-scaling in the ISP crop rectangle
> > -      * comes straight from the RPi pipeline handler.
> > -      *
> > -      * Create a version of the crop rectangle aligned to the analogue crop
> > -      * rectangle top-left coordinates and scaled in the [analogue crop to
> > -      * output frame] ratio to take into account binning/skipping on the
> > -      * sensor.
> > -      */
> > -     Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop
> > -                                                            .topLeft());
> > -     ispCrop.scaleBy(sensorInfo.outputSize, sensorInfo.analogCrop.size());
> > -
> > -     /*
> > -      * The crop rectangle should be:
> > -      * 1. At least as big as ispMinCropSize_, once that's been
> > -      *    enlarged to the same aspect ratio.
> > -      * 2. With the same mid-point, if possible.
> > -      * 3. But it can't go outside the sensor area.
> > -      */
> > -     Rectangle ispMinCrop{ 0, 0, 640, 480 };
> > -     Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size());
> > -     Size size = ispCrop.size().expandedTo(minSize);
> > -     ispCrop = size.centeredTo(ispCrop.center())
> > -                   .enclosedIn(Rectangle(sensorInfo.outputSize));
> > -
> > -     /*
> > -      * As the resizer can't upscale, the crop rectangle has to be larger
> > -      * than the larger stream output size.
> > -      */
> > -     Size maxYuvSize;
> > -     for (MaliC55Pipe &pipe : pipes_) {
> > -             if (!pipe.stream)
> > -                     continue;
> > -
> > -             const StreamConfiguration &config = pipe.stream->configuration();
> > -             if (isFormatRaw(config.pixelFormat)) {
> > -                     LOG(MaliC55, Debug) << "Cannot crop with a RAW stream";
> > -                     return;
> > -             }
> > -
> > -             Size streamSize = config.size;
> > -             if (streamSize.width > maxYuvSize.width)
> > -                     maxYuvSize.width = streamSize.width;
> > -             if (streamSize.height > maxYuvSize.height)
> > -                     maxYuvSize.height = streamSize.height;
> > -     }
> > -
> > -     ispCrop.size().expandTo(maxYuvSize);
> > -
> > -     /*
> > -      * Now apply the scaler crop to each enabled output. This overrides the
> > -      * crop configuration performed at configure() time and can cause
> > -      * square pixels if the crop rectangle and scaler output FOV ratio are
> > -      * different.
> > -      */
> > -     for (MaliC55Pipe &pipe : pipes_) {
> > -             if (!pipe.stream)
> > -                     continue;
> > -
> > -             /* Create a copy to avoid setSelection() to modify ispCrop. */
> > -             Rectangle pipeCrop = ispCrop;
> > -             ret = pipe.resizer->setSelection(0, V4L2_SEL_TGT_CROP, &pipeCrop);
> > -             if (ret) {
> > -                     LOG(MaliC55, Error)
> > -                             << "Failed to apply crop to "
> > -                             << (pipe.stream == &data->frStream_ ?
> > -                                 "FR" : "DS") << " pipe";
> > -                     return;
> > -             }
> > -     }
> > -}
> > -
> >  int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)
> >  {
> >       MaliC55CameraData *data = cameraData(camera);
> > @@ -1752,4 +1656,4 @@ bool PipelineHandlerMaliC55::match(DeviceEnumerator *enumerator)
> >
> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerMaliC55, "mali-c55")
> >
> > -} /* namespace libcamera */
> > +} /* namespace */
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 6e039bf35..4d9943d61 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -41,7 +41,9 @@
> >  #include "libcamera/internal/v4l2_subdevice.h"
> >  #include "libcamera/internal/v4l2_videodevice.h"
> >
> > -namespace libcamera {
> > +namespace {
> > +
> > +using namespace libcamera;
> >
> >  LOG_DEFINE_CATEGORY(SimplePipeline)
> >
> > @@ -1751,4 +1753,4 @@ void SimplePipelineHandler::releasePipeline(SimpleCameraData *data)
> >
> >  REGISTER_PIPELINE_HANDLER(SimplePipelineHandler, "simple")
> >
> > -} /* namespace libcamera */
> > +} /* namespace */
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 7470b5627..dedcac89b 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -32,7 +32,9 @@
> >  #include "libcamera/internal/sysfs.h"
> >  #include "libcamera/internal/v4l2_videodevice.h"
> >
> > -namespace libcamera {
> > +namespace {
> > +
> > +using namespace libcamera;
> >
> >  LOG_DEFINE_CATEGORY(UVC)
> >
> > @@ -804,4 +806,4 @@ void UVCCameraData::imageBufferReady(FrameBuffer *buffer)
> >
> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC, "uvcvideo")
> >
> > -} /* namespace libcamera */
> > +} /* namespace */
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index 07273bd2b..f86ee5206 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -42,7 +42,9 @@
> >  #include "libcamera/internal/v4l2_subdevice.h"
> >  #include "libcamera/internal/v4l2_videodevice.h"
> >
> > -namespace libcamera {
> > +namespace {
> > +
> > +using namespace libcamera;
> >
> >  LOG_DEFINE_CATEGORY(VIMC)
> >
> > @@ -646,4 +648,4 @@ void VimcCameraData::paramsComputed([[maybe_unused]] unsigned int id,
> >
> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc, "vimc")
> >
> > -} /* namespace libcamera */
> > +} /* namespace */
> > --
> > 2.48.1
Laurent Pinchart March 18, 2025, 2:40 p.m. UTC | #3
On Tue, Mar 18, 2025 at 11:25:20AM +0000, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2025-03-17 19:12:15)
> > On Fri, Mar 14, 2025 at 06:42:00PM +0100, Barnabás Pőcze wrote:
> > > Putting a symbol into an anonymous namespace gives its internal linkage.
> > > This is useful for a couple reasons:
> > >   * the compiler can do more optimizations (e.g. inlining);
> > >   * fewer symbols are exported from the DSO;
> > >   * unused symbols can be better diagnosed.
> > >
> > > For example, as it turns out, `PipelineHandlerMaliC55::applyScalerCrop()`
> > > is not actually used, which was not diagnosed previously.

Dan, should this function be dropped, or should we implement scaler crop
in the pipeline handler ?

> > >
> > > Similarly, `ISICameraData::pipe()` is also not used, it is removed.
> 
> If we've got unused/dead code I think that should be removed in a
> dedicated patch of it's own.
> 
> > > This could be done with the other pipeline handlers, but is not done,
> > > due to the issues caused by the log category symbols and to avoid
> > > inconsistencies where half of a pipeline handler is in one namespace
> > > while the other half is somewhere else.
> > 
> > I'm just afraid once these pipelines grow to be larger than a single
> > file, whoever will modify this in future might be a bit confused ?
> > 
> > Can we comment that we can only do this for single file pipelines ?
> > But then, you know, some pipeline handlers are anonymized some are
> > not, not sure, I'm a bit in two minds here.
> > 
> > Let's see what others think..
> 
> I don't object to simplifying the namespace of the pipeline handler
> internals. The rest of libcamera shouldn't be accessing internals of a
> pipeline ...
> 
> Perhaps even pipeline handlers might get smaller if we can progress
> towards 'modular pipeline handlers' with more common code abstracted
> anyway ...
> 
> 
> 
> > 
> > >
> > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp |  13 +--
> > >  src/libcamera/pipeline/mali-c55/mali-c55.cpp | 104 +------------------
> > >  src/libcamera/pipeline/simple/simple.cpp     |   6 +-
> > >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp |   6 +-
> > >  src/libcamera/pipeline/vimc/vimc.cpp         |   6 +-
> > >  5 files changed, 20 insertions(+), 115 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > > index 4e66b3368..421328536 100644
> > > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > > @@ -31,7 +31,9 @@
> > >
> > >  #include "linux/media-bus-format.h"
> > >
> > > -namespace libcamera {
> > > +namespace {
> > > +
> > > +using namespace libcamera;
> > >
> > >  LOG_DEFINE_CATEGORY(ISI)
> > >
> > > @@ -50,8 +52,6 @@ public:
> > >               streams_.resize(2);
> > >       }
> > >
> > > -     PipelineHandlerISI *pipe();
> > > -
> > >       int init();
> > >
> > >       unsigned int pipeIndex(const Stream *stream)
> > > @@ -149,11 +149,6 @@ private:
> > >   * Camera Data
> > >   */
> > >
> > > -PipelineHandlerISI *ISICameraData::pipe()
> > > -{
> > > -     return static_cast<PipelineHandlerISI *>(Camera::Private::pipe());
> > > -}
> > > -
> > >  /* Open and initialize pipe components. */
> > >  int ISICameraData::init()
> > >  {
> > > @@ -1113,4 +1108,4 @@ void PipelineHandlerISI::bufferReady(FrameBuffer *buffer)
> > >
> > >  REGISTER_PIPELINE_HANDLER(PipelineHandlerISI, "imx8-isi")
> > >
> > > -} /* namespace libcamera */
> > > +} /* namespace */
> > > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > index a05e11fcc..17cae9b96 100644
> > > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > @@ -52,7 +52,9 @@ bool isFormatRaw(const libcamera::PixelFormat &pixFmt)
> > >
> > >  } /* namespace */
> > >
> > > -namespace libcamera {
> > > +namespace {
> > > +
> > > +using namespace libcamera;
> > >
> > >  LOG_DEFINE_CATEGORY(MaliC55)
> > >
> > > @@ -677,8 +679,6 @@ private:
> > >                                    const StreamConfiguration &config,
> > >                                    V4L2SubdeviceFormat &subdevFormat);
> > >
> > > -     void applyScalerCrop(Camera *camera, const ControlList &controls);
> > > -
> > >       bool registerMaliCamera(std::unique_ptr<MaliC55CameraData> data,
> > >                               const std::string &name);
> > >       bool registerTPGCamera(MediaLink *link);
> > > @@ -1270,102 +1270,6 @@ void PipelineHandlerMaliC55::stopDevice(Camera *camera)
> > >       freeBuffers(camera);
> > >  }
> > >
> > > -void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera,
> > > -                                          const ControlList &controls)
> > > -{
> > > -     MaliC55CameraData *data = cameraData(camera);
> > > -
> > > -     const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop);
> > > -     if (!scalerCrop)
> > > -             return;
> > > -
> > > -     if (!data->sensor_) {
> > > -             LOG(MaliC55, Error) << "ScalerCrop not supported for TPG";
> > > -             return;
> > > -     }
> > > -
> > > -     Rectangle nativeCrop = *scalerCrop;
> > > -
> > > -     IPACameraSensorInfo sensorInfo;
> > > -     int ret = data->sensor_->sensorInfo(&sensorInfo);
> > > -     if (ret) {
> > > -             LOG(MaliC55, Error) << "Failed to retrieve sensor info";
> > > -             return;
> > > -     }
> > > -
> > > -     /*
> > > -      * The ScalerCrop rectangle re-scaling in the ISP crop rectangle
> > > -      * comes straight from the RPi pipeline handler.
> > > -      *
> > > -      * Create a version of the crop rectangle aligned to the analogue crop
> > > -      * rectangle top-left coordinates and scaled in the [analogue crop to
> > > -      * output frame] ratio to take into account binning/skipping on the
> > > -      * sensor.
> > > -      */
> > > -     Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop
> > > -                                                            .topLeft());
> > > -     ispCrop.scaleBy(sensorInfo.outputSize, sensorInfo.analogCrop.size());
> > > -
> > > -     /*
> > > -      * The crop rectangle should be:
> > > -      * 1. At least as big as ispMinCropSize_, once that's been
> > > -      *    enlarged to the same aspect ratio.
> > > -      * 2. With the same mid-point, if possible.
> > > -      * 3. But it can't go outside the sensor area.
> > > -      */
> > > -     Rectangle ispMinCrop{ 0, 0, 640, 480 };
> > > -     Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size());
> > > -     Size size = ispCrop.size().expandedTo(minSize);
> > > -     ispCrop = size.centeredTo(ispCrop.center())
> > > -                   .enclosedIn(Rectangle(sensorInfo.outputSize));
> > > -
> > > -     /*
> > > -      * As the resizer can't upscale, the crop rectangle has to be larger
> > > -      * than the larger stream output size.
> > > -      */
> > > -     Size maxYuvSize;
> > > -     for (MaliC55Pipe &pipe : pipes_) {
> > > -             if (!pipe.stream)
> > > -                     continue;
> > > -
> > > -             const StreamConfiguration &config = pipe.stream->configuration();
> > > -             if (isFormatRaw(config.pixelFormat)) {
> > > -                     LOG(MaliC55, Debug) << "Cannot crop with a RAW stream";
> > > -                     return;
> > > -             }
> > > -
> > > -             Size streamSize = config.size;
> > > -             if (streamSize.width > maxYuvSize.width)
> > > -                     maxYuvSize.width = streamSize.width;
> > > -             if (streamSize.height > maxYuvSize.height)
> > > -                     maxYuvSize.height = streamSize.height;
> > > -     }
> > > -
> > > -     ispCrop.size().expandTo(maxYuvSize);
> > > -
> > > -     /*
> > > -      * Now apply the scaler crop to each enabled output. This overrides the
> > > -      * crop configuration performed at configure() time and can cause
> > > -      * square pixels if the crop rectangle and scaler output FOV ratio are
> > > -      * different.
> > > -      */
> > > -     for (MaliC55Pipe &pipe : pipes_) {
> > > -             if (!pipe.stream)
> > > -                     continue;
> > > -
> > > -             /* Create a copy to avoid setSelection() to modify ispCrop. */
> > > -             Rectangle pipeCrop = ispCrop;
> > > -             ret = pipe.resizer->setSelection(0, V4L2_SEL_TGT_CROP, &pipeCrop);
> > > -             if (ret) {
> > > -                     LOG(MaliC55, Error)
> > > -                             << "Failed to apply crop to "
> > > -                             << (pipe.stream == &data->frStream_ ?
> > > -                                 "FR" : "DS") << " pipe";
> > > -                     return;
> > > -             }
> > > -     }
> > > -}
> > > -
> > >  int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)
> > >  {
> > >       MaliC55CameraData *data = cameraData(camera);
> > > @@ -1752,4 +1656,4 @@ bool PipelineHandlerMaliC55::match(DeviceEnumerator *enumerator)
> > >
> > >  REGISTER_PIPELINE_HANDLER(PipelineHandlerMaliC55, "mali-c55")
> > >
> > > -} /* namespace libcamera */
> > > +} /* namespace */
> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > index 6e039bf35..4d9943d61 100644
> > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > @@ -41,7 +41,9 @@
> > >  #include "libcamera/internal/v4l2_subdevice.h"
> > >  #include "libcamera/internal/v4l2_videodevice.h"
> > >
> > > -namespace libcamera {
> > > +namespace {
> > > +
> > > +using namespace libcamera;
> > >
> > >  LOG_DEFINE_CATEGORY(SimplePipeline)
> > >
> > > @@ -1751,4 +1753,4 @@ void SimplePipelineHandler::releasePipeline(SimpleCameraData *data)
> > >
> > >  REGISTER_PIPELINE_HANDLER(SimplePipelineHandler, "simple")
> > >
> > > -} /* namespace libcamera */
> > > +} /* namespace */
> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > index 7470b5627..dedcac89b 100644
> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > @@ -32,7 +32,9 @@
> > >  #include "libcamera/internal/sysfs.h"
> > >  #include "libcamera/internal/v4l2_videodevice.h"
> > >
> > > -namespace libcamera {
> > > +namespace {
> > > +
> > > +using namespace libcamera;
> > >
> > >  LOG_DEFINE_CATEGORY(UVC)
> > >
> > > @@ -804,4 +806,4 @@ void UVCCameraData::imageBufferReady(FrameBuffer *buffer)
> > >
> > >  REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC, "uvcvideo")
> > >
> > > -} /* namespace libcamera */
> > > +} /* namespace */
> > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > > index 07273bd2b..f86ee5206 100644
> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > @@ -42,7 +42,9 @@
> > >  #include "libcamera/internal/v4l2_subdevice.h"
> > >  #include "libcamera/internal/v4l2_videodevice.h"
> > >
> > > -namespace libcamera {
> > > +namespace {
> > > +
> > > +using namespace libcamera;
> > >
> > >  LOG_DEFINE_CATEGORY(VIMC)
> > >
> > > @@ -646,4 +648,4 @@ void VimcCameraData::paramsComputed([[maybe_unused]] unsigned int id,
> > >
> > >  REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc, "vimc")
> > >
> > > -} /* namespace libcamera */
> > > +} /* namespace */

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
index 4e66b3368..421328536 100644
--- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
+++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
@@ -31,7 +31,9 @@ 

 #include "linux/media-bus-format.h"

-namespace libcamera {
+namespace {
+
+using namespace libcamera;

 LOG_DEFINE_CATEGORY(ISI)

@@ -50,8 +52,6 @@  public:
 		streams_.resize(2);
 	}

-	PipelineHandlerISI *pipe();
-
 	int init();

 	unsigned int pipeIndex(const Stream *stream)
@@ -149,11 +149,6 @@  private:
  * Camera Data
  */

-PipelineHandlerISI *ISICameraData::pipe()
-{
-	return static_cast<PipelineHandlerISI *>(Camera::Private::pipe());
-}
-
 /* Open and initialize pipe components. */
 int ISICameraData::init()
 {
@@ -1113,4 +1108,4 @@  void PipelineHandlerISI::bufferReady(FrameBuffer *buffer)

 REGISTER_PIPELINE_HANDLER(PipelineHandlerISI, "imx8-isi")

-} /* namespace libcamera */
+} /* namespace */
diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
index a05e11fcc..17cae9b96 100644
--- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
+++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
@@ -52,7 +52,9 @@  bool isFormatRaw(const libcamera::PixelFormat &pixFmt)

 } /* namespace */

-namespace libcamera {
+namespace {
+
+using namespace libcamera;

 LOG_DEFINE_CATEGORY(MaliC55)

@@ -677,8 +679,6 @@  private:
 				     const StreamConfiguration &config,
 				     V4L2SubdeviceFormat &subdevFormat);

-	void applyScalerCrop(Camera *camera, const ControlList &controls);
-
 	bool registerMaliCamera(std::unique_ptr<MaliC55CameraData> data,
 				const std::string &name);
 	bool registerTPGCamera(MediaLink *link);
@@ -1270,102 +1270,6 @@  void PipelineHandlerMaliC55::stopDevice(Camera *camera)
 	freeBuffers(camera);
 }

-void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera,
-					     const ControlList &controls)
-{
-	MaliC55CameraData *data = cameraData(camera);
-
-	const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop);
-	if (!scalerCrop)
-		return;
-
-	if (!data->sensor_) {
-		LOG(MaliC55, Error) << "ScalerCrop not supported for TPG";
-		return;
-	}
-
-	Rectangle nativeCrop = *scalerCrop;
-
-	IPACameraSensorInfo sensorInfo;
-	int ret = data->sensor_->sensorInfo(&sensorInfo);
-	if (ret) {
-		LOG(MaliC55, Error) << "Failed to retrieve sensor info";
-		return;
-	}
-
-	/*
-	 * The ScalerCrop rectangle re-scaling in the ISP crop rectangle
-	 * comes straight from the RPi pipeline handler.
-	 *
-	 * Create a version of the crop rectangle aligned to the analogue crop
-	 * rectangle top-left coordinates and scaled in the [analogue crop to
-	 * output frame] ratio to take into account binning/skipping on the
-	 * sensor.
-	 */
-	Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop
-							       .topLeft());
-	ispCrop.scaleBy(sensorInfo.outputSize, sensorInfo.analogCrop.size());
-
-	/*
-	 * The crop rectangle should be:
-	 * 1. At least as big as ispMinCropSize_, once that's been
-	 *    enlarged to the same aspect ratio.
-	 * 2. With the same mid-point, if possible.
-	 * 3. But it can't go outside the sensor area.
-	 */
-	Rectangle ispMinCrop{ 0, 0, 640, 480 };
-	Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size());
-	Size size = ispCrop.size().expandedTo(minSize);
-	ispCrop = size.centeredTo(ispCrop.center())
-		      .enclosedIn(Rectangle(sensorInfo.outputSize));
-
-	/*
-	 * As the resizer can't upscale, the crop rectangle has to be larger
-	 * than the larger stream output size.
-	 */
-	Size maxYuvSize;
-	for (MaliC55Pipe &pipe : pipes_) {
-		if (!pipe.stream)
-			continue;
-
-		const StreamConfiguration &config = pipe.stream->configuration();
-		if (isFormatRaw(config.pixelFormat)) {
-			LOG(MaliC55, Debug) << "Cannot crop with a RAW stream";
-			return;
-		}
-
-		Size streamSize = config.size;
-		if (streamSize.width > maxYuvSize.width)
-			maxYuvSize.width = streamSize.width;
-		if (streamSize.height > maxYuvSize.height)
-			maxYuvSize.height = streamSize.height;
-	}
-
-	ispCrop.size().expandTo(maxYuvSize);
-
-	/*
-	 * Now apply the scaler crop to each enabled output. This overrides the
-	 * crop configuration performed at configure() time and can cause
-	 * square pixels if the crop rectangle and scaler output FOV ratio are
-	 * different.
-	 */
-	for (MaliC55Pipe &pipe : pipes_) {
-		if (!pipe.stream)
-			continue;
-
-		/* Create a copy to avoid setSelection() to modify ispCrop. */
-		Rectangle pipeCrop = ispCrop;
-		ret = pipe.resizer->setSelection(0, V4L2_SEL_TGT_CROP, &pipeCrop);
-		if (ret) {
-			LOG(MaliC55, Error)
-				<< "Failed to apply crop to "
-				<< (pipe.stream == &data->frStream_ ?
-				    "FR" : "DS") << " pipe";
-			return;
-		}
-	}
-}
-
 int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)
 {
 	MaliC55CameraData *data = cameraData(camera);
@@ -1752,4 +1656,4 @@  bool PipelineHandlerMaliC55::match(DeviceEnumerator *enumerator)

 REGISTER_PIPELINE_HANDLER(PipelineHandlerMaliC55, "mali-c55")

-} /* namespace libcamera */
+} /* namespace */
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 6e039bf35..4d9943d61 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -41,7 +41,9 @@ 
 #include "libcamera/internal/v4l2_subdevice.h"
 #include "libcamera/internal/v4l2_videodevice.h"

-namespace libcamera {
+namespace {
+
+using namespace libcamera;

 LOG_DEFINE_CATEGORY(SimplePipeline)

@@ -1751,4 +1753,4 @@  void SimplePipelineHandler::releasePipeline(SimpleCameraData *data)

 REGISTER_PIPELINE_HANDLER(SimplePipelineHandler, "simple")

-} /* namespace libcamera */
+} /* namespace */
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
index 7470b5627..dedcac89b 100644
--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
@@ -32,7 +32,9 @@ 
 #include "libcamera/internal/sysfs.h"
 #include "libcamera/internal/v4l2_videodevice.h"

-namespace libcamera {
+namespace {
+
+using namespace libcamera;

 LOG_DEFINE_CATEGORY(UVC)

@@ -804,4 +806,4 @@  void UVCCameraData::imageBufferReady(FrameBuffer *buffer)

 REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC, "uvcvideo")

-} /* namespace libcamera */
+} /* namespace */
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
index 07273bd2b..f86ee5206 100644
--- a/src/libcamera/pipeline/vimc/vimc.cpp
+++ b/src/libcamera/pipeline/vimc/vimc.cpp
@@ -42,7 +42,9 @@ 
 #include "libcamera/internal/v4l2_subdevice.h"
 #include "libcamera/internal/v4l2_videodevice.h"

-namespace libcamera {
+namespace {
+
+using namespace libcamera;

 LOG_DEFINE_CATEGORY(VIMC)

@@ -646,4 +648,4 @@  void VimcCameraData::paramsComputed([[maybe_unused]] unsigned int id,

 REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc, "vimc")

-} /* namespace libcamera */
+} /* namespace */