[PATCH v1] libcamera: pipeline: Put single file pipelines into anonymous namespace
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Mar 18 12:25:20 CET 2025
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 at 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
More information about the libcamera-devel
mailing list