[libcamera-devel] [PATCH] libcamera: v4l2_subdevice: Silence warning for unknown metadata formats

[Laurent Pinchart]

Hi Umang,

On Tue, Sep 06, 2022 at 01:12:39AM +0530, Umang Jain wrote:
> On 9/5/22 10:16 PM, Laurent Pinchart via libcamera-devel wrote:
> > Commit e297673e7686 ("libcamera: v4l2_device: Adjust colorspace based on
> > pixel format") has introduced a warning when trying to convert a color
> > space from V4L2 to libcamera if the media bus code is unknown. This was
> > meant to catch unknown image formats, but turned out to be also
> > triggered for metadata formats.
> 
> err...
> 
> > Color spaces are not applicable to metadata formats, there should thus
> > be no warning. Fix it by skipping the color space translation and
> > returning std::nullopt directly if the kernel reports
> > V4L2_COLORSPACE_DEFAULT. This doesn't introduce any change in behaviour
> > other than getting rid of the warning, as the V4L2Device::toColorSpace()
> > function returns std::nullopt alread in that case.
> 
> s/alread/already
> 
> > Fixes: e297673e7686 ("libcamera: v4l2_device: Adjust colorspace based on pixel format")
> > Reported-by: Naushir Patuck <naush at raspberrypi.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >   include/libcamera/internal/v4l2_subdevice.h |  3 ++
> >   src/libcamera/v4l2_subdevice.cpp            | 51 ++++++++++-----------
> >   2 files changed, 26 insertions(+), 28 deletions(-)
> >
> > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> > index 00be17bb1465..69862de0585a 100644
> > --- a/include/libcamera/internal/v4l2_subdevice.h
> > +++ b/include/libcamera/internal/v4l2_subdevice.h
> > @@ -101,6 +101,9 @@ protected:
> >   private:
> >   	LIBCAMERA_DISABLE_COPY(V4L2Subdevice)
> >   
> > +	std::optional<ColorSpace>
> > +	toColorSpace(const v4l2_mbus_framefmt &format) const;
> > +
> >   	std::vector<unsigned int> enumPadCodes(unsigned int pad);
> >   	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> >   					    unsigned int code);
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index 95bfde340d8c..f3a9a0096c6c 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -477,6 +477,27 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
> >   	return formats;
> >   }
> >   
> > +std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &format) const
> > +{
> 
> Comment about metadata formats <>  V4L2_COLORSPACE_DEFAULT please :-)

I'll add

	/*
	 * Only image formats have a color space, for other formats (such as
	 * metadata formats) the color space concept isn't applicable. V4L2
	 * subdev drivers return a colorspace set to V4L2_COLORSPACE_DEFAULT in
	 * that case (as well as for image formats when the driver hasn't
	 * bothered implementing color space support). Check the colorspace
	 * field here and return std::nullopt directly to avoid logging a
	 * warning.
	 */

> Other than that, looks good to me.
> 
> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> 
> > +	if (format.colorspace == V4L2_COLORSPACE_DEFAULT)
> > +		return std::nullopt;
> > +
> > +	PixelFormatInfo::ColourEncoding colourEncoding;
> > +	auto iter = formatInfoMap.find(format.code);
> > +	if (iter != formatInfoMap.end()) {
> > +		colourEncoding = iter->second.colourEncoding;
> > +	} else {
> > +		LOG(V4L2, Warning)
> > +			<< "Unknown subdev format "
> > +			<< utils::hex(format.code, 4)
> > +			<< ", defaulting to RGB encoding";
> > +
> > +		colourEncoding = PixelFormatInfo::ColourEncodingRGB;
> > +	}
> > +
> > +	return V4L2Device::toColorSpace(format, colourEncoding);
> > +}
> > +
> >   /**
> >    * \brief Retrieve the image format set on one of the V4L2 subdevice pads
> >    * \param[in] pad The 0-indexed pad number the format is to be retrieved from
> > @@ -503,20 +524,7 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> >   	format->size.width = subdevFmt.format.width;
> >   	format->size.height = subdevFmt.format.height;
> >   	format->mbus_code = subdevFmt.format.code;
> > -
> > -	PixelFormatInfo::ColourEncoding colourEncoding;
> > -	auto iter = formatInfoMap.find(subdevFmt.format.code);
> > -	if (iter != formatInfoMap.end()) {
> > -		colourEncoding = iter->second.colourEncoding;
> > -	} else {
> > -		LOG(V4L2, Warning)
> > -			<< "Unknown subdev format "
> > -			<< utils::hex(subdevFmt.format.code, 4)
> > -			<< ", defaulting to RGB encoding";
> > -
> > -		colourEncoding = PixelFormatInfo::ColourEncodingRGB;
> > -	}
> > -	format->colorSpace = toColorSpace(subdevFmt.format, colourEncoding);
> > +	format->colorSpace = toColorSpace(subdevFmt.format);
> >   
> >   	return 0;
> >   }
> > @@ -562,20 +570,7 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> >   	format->size.width = subdevFmt.format.width;
> >   	format->size.height = subdevFmt.format.height;
> >   	format->mbus_code = subdevFmt.format.code;
> > -
> > -	PixelFormatInfo::ColourEncoding colourEncoding;
> > -	auto iter = formatInfoMap.find(subdevFmt.format.code);
> > -	if (iter != formatInfoMap.end()) {
> > -		colourEncoding = iter->second.colourEncoding;
> > -	} else {
> > -		LOG(V4L2, Warning)
> > -			<< "Unknown subdev format "
> > -			<< utils::hex(subdevFmt.format.code, 4)
> > -			<< ", defaulting to RGB encoding";
> > -
> > -		colourEncoding = PixelFormatInfo::ColourEncodingRGB;
> > -	}
> > -	format->colorSpace = toColorSpace(subdevFmt.format, colourEncoding);
> > +	format->colorSpace = toColorSpace(subdevFmt.format);
> >   
> >   	return 0;
> >   }
> >
> > base-commit: da9bb8dea6821edcfd3464e8cba37ad4a91087d6

-- 
Regards,

Laurent Pinchart