[libcamera-devel] [PATCH 07/17] libcamera: v4l2_subdevice: Rework enumPadSizes()

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, May 27, 2019 at 09:05:30PM +0200, Jacopo Mondi wrote:
> On Mon, May 27, 2019 at 02:15:33AM +0200, Niklas Söderlund wrote:
> > Align the enumPadSizes() interface and implementation with that of
> > enumPadCodes(). There is no functional change.
> 
> I'm afraid there are a few :)
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/include/v4l2_subdevice.h |  4 +--
> >  src/libcamera/v4l2_subdevice.cpp       | 36 ++++++++++++--------------
> >  2 files changed, 19 insertions(+), 21 deletions(-)
> >
> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > index e714e2575022c04d..c6fdf417b43c0423 100644
> > --- a/src/libcamera/include/v4l2_subdevice.h
> > +++ b/src/libcamera/include/v4l2_subdevice.h
> > @@ -58,8 +58,8 @@ protected:
> >
> >  private:
> >  	std::vector<unsigned int> enumPadCodes(unsigned int pad);
> > -	int enumPadSizes(unsigned int pad, unsigned int code,
> > -			 std::vector<SizeRange> *size);
> > +	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> > +					    unsigned int code);
> >
> >  	int setSelection(unsigned int pad, unsigned int target,
> >  			 Rectangle *rect);
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index 51b0ffaafb3e668e..8720013600ddb95f 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -210,8 +210,7 @@ FormatEnum V4L2Subdevice::formats(unsigned int pad)
> >  	}
> >
> >  	for (unsigned int code : enumPadCodes(pad))
> > -		if (enumPadSizes(pad, code, &formatMap[code]))
> > -			return {};
> > +		formatMap[code] = enumPadSizes(pad, code);
> >
> >  	return formatMap;
> >  }
> > @@ -336,38 +335,37 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> >  	return codes;
> >  }
> >
> > -int V4L2Subdevice::enumPadSizes(unsigned int pad,unsigned int code,
> > -				std::vector<SizeRange> *sizes)
> > +std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > +						   unsigned int code)
> 
> One of the reason I returned an int and accepted the vector as
> parameter, was to be able to propagate up any error returned by
> ENUM_FRAME_SIZES which is here ignored instead (not true, a message is
> printed out) leaving the format map with an entry with an empty size
> list.
> 
> If you're confortable with not propagating the error up, which I agree
> might not be that -strictly- vital, at least you should check if the
> returned vector is empty and not add the format code to the map. This
> means you should store the size list in a local variable first, then
> copy it in the map, giving up  any return value optimization speed
> up. This was the reason this code returned an int.

I agree, I think we should fail if an enumeration error occurs. If we
silently continue we'll end up with inconsistent behaviours and possibly
hard to reproduce and debug errors.

> >  {
> > -	struct v4l2_subdev_frame_size_enum sizeEnum = {};
> > +	std::vector<SizeRange> sizes;
> >  	int ret;
> >
> > -	sizeEnum.index = 0;
> > -	sizeEnum.pad = pad;
> > -	sizeEnum.code = code;
> > -	sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > -	while (true) {
> > +	for (unsigned int index = 0;; index++) {
> > +		struct v4l2_subdev_frame_size_enum sizeEnum = {
> > +			.index = index,
> > +			.pad = pad,
> > +			.code = code,
> > +			.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +		};
> > +
> >  		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> >  		if (ret)
> >  			break;
> >
> > -		sizes->emplace_back(sizeEnum.min_width, sizeEnum.min_height,
> > +		sizes.emplace_back(sizeEnum.min_width, sizeEnum.min_height,
> >  				    sizeEnum.max_width, sizeEnum.max_height);
> > -
> > -		sizeEnum.index++;
> >  	}
> >
> >  	if (ret && (errno != EINVAL && errno != ENOTTY)) {
> > -		ret = -errno;
> > +		ret = errno;
> >  		LOG(V4L2Subdev, Error)
> >  			<< "Unable to enumerate sizes on pad " << pad
> > -			<< ": " << strerror(-ret);
> > -		sizes->clear();
> > -
> > -		return ret;
> > +			<< ": " << strerror(ret);
> > +		return {};
> >  	}
> >
> > -	return 0;
> > +	return sizes;
> >  }
> >
> >  int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,

-- 
Regards,

Laurent Pinchart