[libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2: Consolidate information about formats

[Laurent Pinchart]

Hi Jacopo and Niklas,

On Wed, Jun 24, 2020 at 12:01:14PM +0200, Jacopo Mondi wrote:
> On Tue, Jun 23, 2020 at 04:09:26AM +0200, Niklas Söderlund wrote:
> > Instead of spreading the mapping between media bus codes and V4L2 FourCC
> > all over the CIO2 code collect it in a single map and extract the data
> > from it. This is done in preparation of adding PixelFormat information
> > to the mix.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/pipeline/ipu3/cio2.cpp | 50 +++++++++++++---------------
> >  1 file changed, 24 insertions(+), 26 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > index dbbcf79c7b2ed5c1..f23128d412e6b1a5 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > @@ -18,6 +18,17 @@ namespace libcamera {
> >
> >  LOG_DECLARE_CATEGORY(IPU3)
> >
> > +namespace {
> > +
> > +static const std::map<uint32_t, V4L2PixelFormat> mbusCodesToInfo = {
> > +	{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },
> > +	{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },
> > +	{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },
> > +	{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },
> > +};
> > +
> > +} /* namespace */
> > +
> 
> Ok, I see where this is going with the next patch, but I'm not super
> excited by having to iterate the map just to extract keys (and anyway,
> why doesn't std::map have a keys() function ??).

Possibly because its implementation would be O(n), and STL generally
tries to avoid hiding expensive operations behind apparently simple
functions. Just a speculation.

> >  CIO2Device::CIO2Device()
> >  	: output_(nullptr), csi2_(nullptr), sensor_(nullptr)
> >  {
> > @@ -78,10 +89,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> >  	 * Make sure the sensor produces at least one format compatible with
> >  	 * the CIO2 requirements.
> >  	 */
> > -	const std::set<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,
> > -						MEDIA_BUS_FMT_SGRBG10_1X10,
> > -						MEDIA_BUS_FMT_SGBRG10_1X10,
> > -						MEDIA_BUS_FMT_SRGGB10_1X10 };
> > +	std::set<unsigned int> cio2Codes;
> 
> why a set ?

To keep this efficient, given that std::map is sorted by key, this could
be a vector, and we could call

	cio2Codes.reserve(mbusCodesToInfo.size());

before the std::transform(). I think a set is indeed overkill.

> > +	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> > +		std::inserter(cio2Codes, cio2Codes.begin()),
> > +		[](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; });
> 
> you could
> 		[](decltype(mbusCodeToInfo)::value_type &pair){ return pair.first; });
> or since C++14
> 		[](auto &pair){ return pair.first; });

Much nicer indeed.

> >  	const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes();
> >  	if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
> >  				cio2Codes.begin(), cio2Codes.end())) {
> > @@ -125,11 +136,12 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
> >  	 * Apply the selected format to the sensor, the CSI-2 receiver and
> >  	 * the CIO2 output device.
> >  	 */
> > -	sensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
> > -					    MEDIA_BUS_FMT_SGBRG10_1X10,
> > -					    MEDIA_BUS_FMT_SGRBG10_1X10,
> > -					    MEDIA_BUS_FMT_SRGGB10_1X10 },
> > -					  size);
> > +	std::vector<unsigned int> mbusCodes;
> 
> You could reserve space in the vector knowing the map size

Seems we agree :-)

> > +	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> > +		std::back_inserter(mbusCodes),
> > +		[](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; });
> > +
> > +	sensorFormat = sensor_->getFormat(mbusCodes, size);
> >  	ret = sensor_->setFormat(&sensorFormat);
> >  	if (ret)
> >  		return ret;
> > @@ -138,25 +150,11 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
> >  	if (ret)
> >  		return ret;
> >
> > -	V4L2PixelFormat v4l2Format;
> > -	switch (sensorFormat.mbus_code) {
> > -	case MEDIA_BUS_FMT_SBGGR10_1X10:
> > -		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);
> > -		break;
> > -	case MEDIA_BUS_FMT_SGBRG10_1X10:
> > -		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);
> > -		break;
> > -	case MEDIA_BUS_FMT_SGRBG10_1X10:
> > -		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);
> > -		break;
> > -	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > -		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);
> > -		break;
> > -	default:
> > +	const auto &itInfo = mbusCodesToInfo.find(sensorFormat.mbus_code);
> > +	if (itInfo == mbusCodesToInfo.end())
> >  		return -EINVAL;
> > -	}
> >
> > -	outputFormat->fourcc = v4l2Format;
> > +	outputFormat->fourcc = itInfo->second;
> 
> I would be tempted to suggest you to add helper functions around that
> map, not sure it's worth the effort, but those transform to just
> retreive keys are not nice imho.

I was about to mention that, thinking that the vector of keys could then
be cached, but given that the code is only used in init paths, it may
not be worth it.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> That apart
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> >  	outputFormat->size = sensorFormat.size;
> >  	outputFormat->planesCount = 1;
> >

-- 
Regards,

Laurent Pinchart