[PATCH v1] libcamera: pipeline: Put single file pipelines into anonymous namespace
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Mar 18 15:40:14 CET 2025
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 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 */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list