[libcamera-devel] pipeline: rpi: Don't call toV4L2DeviceFormat() from validate()

18893 diff mbox series

Don't make an unnecessary call to toV4L2DeviceFormat() from validate()
to get a V4L2DeviceFormat. Instead, the conversion can happen directly
from the RAW stream PixelFormat.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Hi Naush

On Thu, Jul 27, 2023 at 02:01:07PM +0100, Naushir Patuck wrote:
> Don't make an unnecessary call to toV4L2DeviceFormat() from validate()
> to get a V4L2DeviceFormat. Instead, the conversion can happen directly
> from the RAW stream PixelFormat.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index e0fbeec37fe9..97acafbbb728 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -226,10 +226,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  	/* Further fixups on the RAW streams. */
>  	for (auto &raw : rawStreams) {
>  		StreamConfiguration &cfg = config_.at(raw.index);
> -		V4L2DeviceFormat rawFormat;
>
> -		BayerFormat::Packing packing = BayerFormat::fromPixelFormat(cfg.pixelFormat).packing;
> -		rawFormat = PipelineHandlerBase::toV4L2DeviceFormat(raw.dev, sensorFormat_, packing);

Is the function still used ?

> +		V4L2DeviceFormat rawFormat;
> +		rawFormat.fourcc = raw.dev->toV4L2PixelFormat(cfg.pixelFormat);
> +		rawFormat.size = cfg.size;
> +		rawFormat.colorSpace = cfg.colorSpace;
>
>  		int ret = raw.dev->tryFormat(&rawFormat);
>  		if (ret)
> --
> 2.34.1
>

Hi Jacopo,

On Thu, 27 Jul 2023 at 15:21, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Naush
>
> On Thu, Jul 27, 2023 at 02:01:07PM +0100, Naushir Patuck wrote:
> > Don't make an unnecessary call to toV4L2DeviceFormat() from validate()
> > to get a V4L2DeviceFormat. Instead, the conversion can happen directly
> > from the RAW stream PixelFormat.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index e0fbeec37fe9..97acafbbb728 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -226,10 +226,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> >       /* Further fixups on the RAW streams. */
> >       for (auto &raw : rawStreams) {
> >               StreamConfiguration &cfg = config_.at(raw.index);
> > -             V4L2DeviceFormat rawFormat;
> >
> > -             BayerFormat::Packing packing = BayerFormat::fromPixelFormat(cfg.pixelFormat).packing;
> > -             rawFormat = PipelineHandlerBase::toV4L2DeviceFormat(raw.dev, sensorFormat_, packing);
>
> Is the function still used ?

Yes, it does get used in vc4.cpp.  Essentially it converts from a
V4L2SubdeviceFormat to V4L2DeviceFormat for any given packing mode.

Regards,
Naush

>
> > +             V4L2DeviceFormat rawFormat;
> > +             rawFormat.fourcc = raw.dev->toV4L2PixelFormat(cfg.pixelFormat);
> > +             rawFormat.size = cfg.size;
> > +             rawFormat.colorSpace = cfg.colorSpace;
> >
> >               int ret = raw.dev->tryFormat(&rawFormat);
> >               if (ret)
> > --
> > 2.34.1
> >

Hi Naush

On Thu, Jul 27, 2023 at 03:32:52PM +0100, Naushir Patuck wrote:
> Hi Jacopo,
>
> On Thu, 27 Jul 2023 at 15:21, Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Hi Naush
> >
> > On Thu, Jul 27, 2023 at 02:01:07PM +0100, Naushir Patuck wrote:
> > > Don't make an unnecessary call to toV4L2DeviceFormat() from validate()
> > > to get a V4L2DeviceFormat. Instead, the conversion can happen directly
> > > from the RAW stream PixelFormat.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > index e0fbeec37fe9..97acafbbb728 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > @@ -226,10 +226,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > >       /* Further fixups on the RAW streams. */
> > >       for (auto &raw : rawStreams) {
> > >               StreamConfiguration &cfg = config_.at(raw.index);
> > > -             V4L2DeviceFormat rawFormat;
> > >
> > > -             BayerFormat::Packing packing = BayerFormat::fromPixelFormat(cfg.pixelFormat).packing;
> > > -             rawFormat = PipelineHandlerBase::toV4L2DeviceFormat(raw.dev, sensorFormat_, packing);
> >
> > Is the function still used ?
>
> Yes, it does get used in vc4.cpp.  Essentially it converts from a
> V4L2SubdeviceFormat to V4L2DeviceFormat for any given packing mode.
>

Ah sorry, I did a wrong `git grep` indeed :/

I guess there is no way we can get rid of this during ::platfomConfigure() as
we cannot bet on having a RAW PixelFormat as we do in this loop.

But anyway, the simplification here is good

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> Regards,
> Naush
>
> >
> > > +             V4L2DeviceFormat rawFormat;
> > > +             rawFormat.fourcc = raw.dev->toV4L2PixelFormat(cfg.pixelFormat);
> > > +             rawFormat.size = cfg.size;
> > > +             rawFormat.colorSpace = cfg.colorSpace;
> > >
> > >               int ret = raw.dev->tryFormat(&rawFormat);
> > >               if (ret)
> > > --
> > > 2.34.1
> > >