Message ID | 20220829163742.1006102-4-umang.jain@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Umang, Thank you for the patch. On Mon, Aug 29, 2022 at 10:07:38PM +0530, Umang Jain via libcamera-devel wrote: > Abstract out usage of V4L2Device::toColorspace() to a separate > namespace. Since it is separated out in an unnamed namespace, > the V4L2Device::toColorSpace() function needs to made public. > This is not an issue since V4L2Device is an internal class. > > No functional changes intended. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > include/libcamera/internal/v4l2_device.h | 14 +++++++------- > src/libcamera/v4l2_videodevice.cpp | 23 +++++++++++++++-------- > 2 files changed, 22 insertions(+), 15 deletions(-) > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > index 75304be1..606332e1 100644 > --- a/include/libcamera/internal/v4l2_device.h > +++ b/include/libcamera/internal/v4l2_device.h > @@ -49,6 +49,13 @@ public: > > void updateControlInfo(); > > + template<typename T> > + static std::optional<ColorSpace> toColorSpace(const T &v4l2Format, > + PixelFormatInfo::ColourEncoding colourEncoding); > + > + template<typename T> > + static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format); > + > protected: > V4L2Device(const std::string &deviceNode); > ~V4L2Device(); > @@ -60,13 +67,6 @@ protected: > > int fd() const { return fd_.get(); } > > - template<typename T> > - static std::optional<ColorSpace> toColorSpace(const T &v4l2Format, > - PixelFormatInfo::ColourEncoding colourEncoding); > - > - template<typename T> > - static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format); > - > private: > static ControlType v4l2CtrlType(uint32_t ctrlType); > static std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl &ctrl); > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 0e3f5436..b443253d 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -914,6 +914,17 @@ int V4L2VideoDevice::trySetFormatMeta(V4L2DeviceFormat *format, bool set) > return 0; > } > > +namespace { > + > +template<typename T> > +std::optional<ColorSpace> getColorSpace(const T &v4l2Format) > +{ > + V4L2PixelFormat fourcc{ v4l2Format.pixelformat }; > + return V4L2Device::toColorSpace(v4l2Format, PixelFormatInfo::info(fourcc).colourEncoding); > +} Have you considered making this function a (static and private) member of the V4L2VideoDevice class ? That way you wouldn't have to make the V4L2Device toColorSpace() function public. I'd then also call it toColorSpace() instead of getColorSpace(). I think the resulting patch would be simpler. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > +} /* namespace */ > + > int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format) > { > struct v4l2_format v4l2Format = {}; > @@ -931,8 +942,7 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format) > format->size.height = pix->height; > format->fourcc = V4L2PixelFormat(pix->pixelformat); > format->planesCount = pix->num_planes; > - format->colorSpace = > - toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding); > + format->colorSpace = getColorSpace(*pix); > > for (unsigned int i = 0; i < format->planesCount; ++i) { > format->planes[i].bpl = pix->plane_fmt[i].bytesperline; > @@ -988,8 +998,7 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > format->planes[i].bpl = pix->plane_fmt[i].bytesperline; > format->planes[i].size = pix->plane_fmt[i].sizeimage; > } > - format->colorSpace = > - toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding); > + format->colorSpace = getColorSpace(*pix); > > return 0; > } > @@ -1013,8 +1022,7 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format) > format->planesCount = 1; > format->planes[0].bpl = pix->bytesperline; > format->planes[0].size = pix->sizeimage; > - format->colorSpace = > - toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding); > + format->colorSpace = getColorSpace(*pix); > > return 0; > } > @@ -1056,8 +1064,7 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > format->planesCount = 1; > format->planes[0].bpl = pix->bytesperline; > format->planes[0].size = pix->sizeimage; > - format->colorSpace = > - toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding); > + format->colorSpace = getColorSpace(*pix); > > return 0; > }
diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h index 75304be1..606332e1 100644 --- a/include/libcamera/internal/v4l2_device.h +++ b/include/libcamera/internal/v4l2_device.h @@ -49,6 +49,13 @@ public: void updateControlInfo(); + template<typename T> + static std::optional<ColorSpace> toColorSpace(const T &v4l2Format, + PixelFormatInfo::ColourEncoding colourEncoding); + + template<typename T> + static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format); + protected: V4L2Device(const std::string &deviceNode); ~V4L2Device(); @@ -60,13 +67,6 @@ protected: int fd() const { return fd_.get(); } - template<typename T> - static std::optional<ColorSpace> toColorSpace(const T &v4l2Format, - PixelFormatInfo::ColourEncoding colourEncoding); - - template<typename T> - static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format); - private: static ControlType v4l2CtrlType(uint32_t ctrlType); static std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl &ctrl); diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 0e3f5436..b443253d 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -914,6 +914,17 @@ int V4L2VideoDevice::trySetFormatMeta(V4L2DeviceFormat *format, bool set) return 0; } +namespace { + +template<typename T> +std::optional<ColorSpace> getColorSpace(const T &v4l2Format) +{ + V4L2PixelFormat fourcc{ v4l2Format.pixelformat }; + return V4L2Device::toColorSpace(v4l2Format, PixelFormatInfo::info(fourcc).colourEncoding); +} + +} /* namespace */ + int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format) { struct v4l2_format v4l2Format = {}; @@ -931,8 +942,7 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format) format->size.height = pix->height; format->fourcc = V4L2PixelFormat(pix->pixelformat); format->planesCount = pix->num_planes; - format->colorSpace = - toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding); + format->colorSpace = getColorSpace(*pix); for (unsigned int i = 0; i < format->planesCount; ++i) { format->planes[i].bpl = pix->plane_fmt[i].bytesperline; @@ -988,8 +998,7 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) format->planes[i].bpl = pix->plane_fmt[i].bytesperline; format->planes[i].size = pix->plane_fmt[i].sizeimage; } - format->colorSpace = - toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding); + format->colorSpace = getColorSpace(*pix); return 0; } @@ -1013,8 +1022,7 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format) format->planesCount = 1; format->planes[0].bpl = pix->bytesperline; format->planes[0].size = pix->sizeimage; - format->colorSpace = - toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding); + format->colorSpace = getColorSpace(*pix); return 0; } @@ -1056,8 +1064,7 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) format->planesCount = 1; format->planes[0].bpl = pix->bytesperline; format->planes[0].size = pix->sizeimage; - format->colorSpace = - toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding); + format->colorSpace = getColorSpace(*pix); return 0; }
Abstract out usage of V4L2Device::toColorspace() to a separate namespace. Since it is separated out in an unnamed namespace, the V4L2Device::toColorSpace() function needs to made public. This is not an issue since V4L2Device is an internal class. No functional changes intended. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- include/libcamera/internal/v4l2_device.h | 14 +++++++------- src/libcamera/v4l2_videodevice.cpp | 23 +++++++++++++++-------- 2 files changed, 22 insertions(+), 15 deletions(-)