| Message ID | 20190527001543.13593-8-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, 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@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. Thanks j > { > - 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, > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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,
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) { - 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,
Align the enumPadSizes() interface and implementation with that of enumPadCodes(). There is no functional change. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/include/v4l2_subdevice.h | 4 +-- src/libcamera/v4l2_subdevice.cpp | 36 ++++++++++++-------------- 2 files changed, 19 insertions(+), 21 deletions(-)