| Message ID | 20191012184407.31684-10-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Sat, Oct 12, 2019 at 09:44:02PM +0300, Laurent Pinchart wrote: > The V4L2Device::listControls() method creates an instance of > V4L2ControlInfo and then inserts it in the device's list of controls. > The insertion uses std::map::emplace(), but passes the V4L2ControlInfo, > resulting in a copy being performed (using the copy constructor of > V4L2ControlInfo). > > Optimise this by really constructing the V4L2ControlInfo in-place. The > use of std::piecewise_construct is required for gcc-5 that seems to have > trouble with std::map::emplace() on non-copyable values in this case. Thanks for fixing this, even if it will go away in next patches I think Acked-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/v4l2_device.cpp | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index fd4b9c6d5c62..3bd82ff23212 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -352,8 +352,7 @@ void V4L2Device::listControls() > ctrl.flags & V4L2_CTRL_FLAG_DISABLED) > continue; > > - V4L2ControlInfo info(ctrl); > - switch (info.type()) { > + switch (ctrl.type) { > case V4L2_CTRL_TYPE_INTEGER: > case V4L2_CTRL_TYPE_BOOLEAN: > case V4L2_CTRL_TYPE_MENU: > @@ -364,12 +363,14 @@ void V4L2Device::listControls() > break; > /* \todo Support compound controls. */ > default: > - LOG(V4L2, Debug) << "Control type '" << info.type() > + LOG(V4L2, Debug) << "Control type '" << ctrl.type > << "' not supported"; > continue; > } > > - controls_.emplace(ctrl.id, info); > + controls_.emplace(std::piecewise_construct, > + std::forward_as_tuple(ctrl.id), > + std::forward_as_tuple(ctrl)); > } > } > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, Thanks for your patch. On 2019-10-12 21:44:02 +0300, Laurent Pinchart wrote: > The V4L2Device::listControls() method creates an instance of > V4L2ControlInfo and then inserts it in the device's list of controls. > The insertion uses std::map::emplace(), but passes the V4L2ControlInfo, > resulting in a copy being performed (using the copy constructor of > V4L2ControlInfo). > > Optimise this by really constructing the V4L2ControlInfo in-place. The > use of std::piecewise_construct is required for gcc-5 that seems to have > trouble with std::map::emplace() on non-copyable values in this case. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/v4l2_device.cpp | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index fd4b9c6d5c62..3bd82ff23212 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -352,8 +352,7 @@ void V4L2Device::listControls() > ctrl.flags & V4L2_CTRL_FLAG_DISABLED) > continue; > > - V4L2ControlInfo info(ctrl); > - switch (info.type()) { > + switch (ctrl.type) { > case V4L2_CTRL_TYPE_INTEGER: > case V4L2_CTRL_TYPE_BOOLEAN: > case V4L2_CTRL_TYPE_MENU: > @@ -364,12 +363,14 @@ void V4L2Device::listControls() > break; > /* \todo Support compound controls. */ > default: > - LOG(V4L2, Debug) << "Control type '" << info.type() > + LOG(V4L2, Debug) << "Control type '" << ctrl.type > << "' not supported"; > continue; > } > > - controls_.emplace(ctrl.id, info); > + controls_.emplace(std::piecewise_construct, > + std::forward_as_tuple(ctrl.id), > + std::forward_as_tuple(ctrl)); > } > } > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index fd4b9c6d5c62..3bd82ff23212 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -352,8 +352,7 @@ void V4L2Device::listControls() ctrl.flags & V4L2_CTRL_FLAG_DISABLED) continue; - V4L2ControlInfo info(ctrl); - switch (info.type()) { + switch (ctrl.type) { case V4L2_CTRL_TYPE_INTEGER: case V4L2_CTRL_TYPE_BOOLEAN: case V4L2_CTRL_TYPE_MENU: @@ -364,12 +363,14 @@ void V4L2Device::listControls() break; /* \todo Support compound controls. */ default: - LOG(V4L2, Debug) << "Control type '" << info.type() + LOG(V4L2, Debug) << "Control type '" << ctrl.type << "' not supported"; continue; } - controls_.emplace(ctrl.id, info); + controls_.emplace(std::piecewise_construct, + std::forward_as_tuple(ctrl.id), + std::forward_as_tuple(ctrl)); } }
The V4L2Device::listControls() method creates an instance of V4L2ControlInfo and then inserts it in the device's list of controls. The insertion uses std::map::emplace(), but passes the V4L2ControlInfo, resulting in a copy being performed (using the copy constructor of V4L2ControlInfo). Optimise this by really constructing the V4L2ControlInfo in-place. The use of std::piecewise_construct is required for gcc-5 that seems to have trouble with std::map::emplace() on non-copyable values in this case. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/v4l2_device.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)