[libcamera-devel,RFC,1/2] libcamera: v4l2_videodevice: Get formats even if no framesizes
diff mbox series

Message ID 20220719121003.1829916-2-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • rkisp1: Get supported formats from driver
Related show

Commit Message

Paul Elder July 19, 2022, 12:10 p.m. UTC 
V4L2VideoDevice::formats() returns an empty list of formats if enumSizes
fails, including if the driver doesn't implement ENUM_FRAMESIZES.

Add an optional ignoreSizes parameter to formats() so that it can still
be used when the pipeline handler knows that its driver doesn't
implement ENUM_FRAMESIZES.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 include/libcamera/internal/v4l2_videodevice.h |  2 +-
 src/libcamera/v4l2_videodevice.cpp            | 12 ++++++++----
 2 files changed, 9 insertions(+), 5 deletions(-)

Comments

Jacopo Mondi July 19, 2022, 3:54 p.m. UTC | #1 
Hi Paul,

On Tue, Jul 19, 2022 at 09:10:02PM +0900, Paul Elder via libcamera-devel wrote:
> V4L2VideoDevice::formats() returns an empty list of formats if enumSizes
> fails, including if the driver doesn't implement ENUM_FRAMESIZES.
>
> Add an optional ignoreSizes parameter to formats() so that it can still
> be used when the pipeline handler knows that its driver doesn't
> implement ENUM_FRAMESIZES.

I think we shall require drivers to implement ENUM_FRAMESIZES, instead
of allowing them to ignore it.

I'm surprised the rkisp1 capture device does not support
.vidioc_enum_framesizes(). Should that be fixed in the driver ?

>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  2 +-
>  src/libcamera/v4l2_videodevice.cpp            | 12 ++++++++----
>  2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 8525acbc..6cc9be85 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -205,7 +205,7 @@ public:
>  	int getFormat(V4L2DeviceFormat *format);
>  	int tryFormat(V4L2DeviceFormat *format);
>  	int setFormat(V4L2DeviceFormat *format);
> -	Formats formats(uint32_t code = 0);
> +	Formats formats(uint32_t code = 0, bool ignoreSizes = false);
>
>  	int setSelection(unsigned int target, Rectangle *rect);
>
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 63911339..b8bd4bc4 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1045,14 +1045,18 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)
>   *
>   * \return A list of the supported video device formats
>   */
> -V4L2VideoDevice::Formats V4L2VideoDevice::formats(uint32_t code)
> +V4L2VideoDevice::Formats V4L2VideoDevice::formats(uint32_t code, bool ignoreSizes)
>  {
>  	Formats formats;
>
>  	for (V4L2PixelFormat pixelFormat : enumPixelformats(code)) {
> -		std::vector<SizeRange> sizes = enumSizes(pixelFormat);
> -		if (sizes.empty())
> -			return {};
> +		std::vector<SizeRange> sizes;
> +
> +		if (!ignoreSizes) {
> +			sizes = enumSizes(pixelFormat);
> +			if (sizes.empty())
> +				return {};
> +		}
>
>  		if (formats.find(pixelFormat) != formats.end()) {
>  			LOG(V4L2, Error)
> --
> 2.30.2
>
Laurent Pinchart July 19, 2022, 9:36 p.m. UTC | #2 
Hello,

On Tue, Jul 19, 2022 at 05:54:32PM +0200, Jacopo Mondi via libcamera-devel wrote:
> On Tue, Jul 19, 2022 at 09:10:02PM +0900, Paul Elder via libcamera-devel wrote:
> > V4L2VideoDevice::formats() returns an empty list of formats if enumSizes
> > fails, including if the driver doesn't implement ENUM_FRAMESIZES.
> >
> > Add an optional ignoreSizes parameter to formats() so that it can still
> > be used when the pipeline handler knows that its driver doesn't
> > implement ENUM_FRAMESIZES.
> 
> I think we shall require drivers to implement ENUM_FRAMESIZES, instead
> of allowing them to ignore it.
> 
> I'm surprised the rkisp1 capture device does not support
> .vidioc_enum_framesizes(). Should that be fixed in the driver ?

Good question. It should be fairly easy, as it should report a single
range with the min/max values corresponding to the absolute limits of
the hardware, and it would allow deferring the question of whether or
not we require all drivers to report sizes (as well as the bikeshedding
on the bool argument to formats() :-)).

> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  include/libcamera/internal/v4l2_videodevice.h |  2 +-
> >  src/libcamera/v4l2_videodevice.cpp            | 12 ++++++++----
> >  2 files changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > index 8525acbc..6cc9be85 100644
> > --- a/include/libcamera/internal/v4l2_videodevice.h
> > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > @@ -205,7 +205,7 @@ public:
> >  	int getFormat(V4L2DeviceFormat *format);
> >  	int tryFormat(V4L2DeviceFormat *format);
> >  	int setFormat(V4L2DeviceFormat *format);
> > -	Formats formats(uint32_t code = 0);
> > +	Formats formats(uint32_t code = 0, bool ignoreSizes = false);
> >
> >  	int setSelection(unsigned int target, Rectangle *rect);
> >
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 63911339..b8bd4bc4 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1045,14 +1045,18 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)
> >   *
> >   * \return A list of the supported video device formats
> >   */
> > -V4L2VideoDevice::Formats V4L2VideoDevice::formats(uint32_t code)
> > +V4L2VideoDevice::Formats V4L2VideoDevice::formats(uint32_t code, bool ignoreSizes)
> >  {
> >  	Formats formats;
> >
> >  	for (V4L2PixelFormat pixelFormat : enumPixelformats(code)) {
> > -		std::vector<SizeRange> sizes = enumSizes(pixelFormat);
> > -		if (sizes.empty())
> > -			return {};
> > +		std::vector<SizeRange> sizes;
> > +
> > +		if (!ignoreSizes) {
> > +			sizes = enumSizes(pixelFormat);
> > +			if (sizes.empty())
> > +				return {};
> > +		}
> >
> >  		if (formats.find(pixelFormat) != formats.end()) {
> >  			LOG(V4L2, Error)

Patch
diff mbox series 

diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
index 8525acbc..6cc9be85 100644
--- a/include/libcamera/internal/v4l2_videodevice.h
+++ b/include/libcamera/internal/v4l2_videodevice.h
@@ -205,7 +205,7 @@  public:
 	int getFormat(V4L2DeviceFormat *format);
 	int tryFormat(V4L2DeviceFormat *format);
 	int setFormat(V4L2DeviceFormat *format);
-	Formats formats(uint32_t code = 0);
+	Formats formats(uint32_t code = 0, bool ignoreSizes = false);
 
 	int setSelection(unsigned int target, Rectangle *rect);
 
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index 63911339..b8bd4bc4 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -1045,14 +1045,18 @@  int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)
  *
  * \return A list of the supported video device formats
  */
-V4L2VideoDevice::Formats V4L2VideoDevice::formats(uint32_t code)
+V4L2VideoDevice::Formats V4L2VideoDevice::formats(uint32_t code, bool ignoreSizes)
 {
 	Formats formats;
 
 	for (V4L2PixelFormat pixelFormat : enumPixelformats(code)) {
-		std::vector<SizeRange> sizes = enumSizes(pixelFormat);
-		if (sizes.empty())
-			return {};
+		std::vector<SizeRange> sizes;
+
+		if (!ignoreSizes) {
+			sizes = enumSizes(pixelFormat);
+			if (sizes.empty())
+				return {};
+		}
 
 		if (formats.find(pixelFormat) != formats.end()) {
 			LOG(V4L2, Error)