[libcamera-devel] [PATCH] libcamera: Introduce V4L2Device base class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jun 12 14:22:50 CEST 2019
Hi Jacopo,
On Wed, Jun 12, 2019 at 01:46:03PM +0200, Jacopo Mondi wrote:
> On Wed, Jun 12, 2019 at 12:21:21PM +0100, Kieran Bingham wrote:
> > On 12/06/2019 11:41, Jacopo Mondi wrote:
> > > The V4L2 devices and subdevices share a few common operations and fields,
> > > like opening and closing a device node, and perform IOCTLs on the
> > > device's file descriptor. With the forthcoming introduction of support
> > > for V4L2 controls, the quantity of shared code will drastically increase,
> > > as the control implementation is identical for the two.
> > >
> > > To maximize code re-use and avoid duplications, provide a V4L2Device
> > > base class which groups the common operations and members, and perform a
> > > tree-wide rename of V4L2Device and V4L2Subdevice in V4L2Videodev and
> > > V4L2Subdev respectively.
> >
> > Bikeshedding here, should it be V4L2VideoDev / V4L2SubDev ?
>
> I have no strong opinion. I would say I'm fine with what I have here,
> since renaming is painful, but if you feel strong on this, I can
> change it.
"subdevice" is not a compound noun, so I think we should write Subdev or
Subdevice, not SubDev or SubDevice. "video device" is made of two words,
so we should capitalise each of them. VideoDev or VideoDevice. We should
thus use V4L2VideoDev / V4L2Subdev or V4L2VideoDevice / V4L2Subdevice. I
have a preference for the latter.
> > I think we commonly use subdev as a single word, but I don't think
> > videodev gets the same treatment usually?
> >
> > > The newly introduced base class provides methods to open/close a device
> > > node, access the file descriptor, and perform IOCTLs on the device.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > This patch is big, and will get in the way of many of the currently in-review
> > > series. At the same time, having a common base where to implement support for
> > > V4L2 controls is a requirement, and so I bit the bullet and went on the huge
> > > renaming operation you have here implemented.
> > >
> > > I renamed V4L2Device->V4L2Videodev and V4L2Subdevice->V4L2Subdev everywhere.
> > > I intentionally left the V4L2DeviceFormat and V4L2SubdeviceFormat un-touched
> > > as Niklas' series is going to remove them, so there was not point of making
> > > things more complex than necessary here.
> > >
> > > I'm sending this one alone, separately from the V4L2 control suppor, to fast
> > > track it and have it merged sooner than later. I know this will cause a lot
> > > of work in-flight to be rebased, I'm sorry about it, but sooner or later this
> > > had to be done.
> > >
> > > Thanks
> > > j
> >
> > Checkstyle on this patch runs pretty clean, considering the amount of
> > code it touches and some of the checkstyle warnings should certainly be
> > ignored.
> >
> > These are the ones I'd fix up:
> >
> > --- src/libcamera/v4l2_device.cpp
> > +++ src/libcamera/v4l2_device.cpp
> > @@ -48,8 +48,8 @@
> > *
> >
> > * Initialize the file descriptor to -1.
> > */
> > -V4L2Device::V4L2Device() :
> > - fd_(-1)
> > +V4L2Device::V4L2Device()
> > + : fd_(-1)
> > {
> > }
> >
> > --- src/libcamera/v4l2_subdev.cpp
> > +++ src/libcamera/v4l2_subdev.cpp
> > @@ -302,7 +302,7 @@
> > return "'" + entity_->name() + "'";
> > }
> >
> > -int V4L2Subdev::enumPadSizes(unsigned int pad,unsigned int code,
> > +int V4L2Subdev::enumPadSizes(unsigned int pad, unsigned int code,
> > std::vector<SizeRange> *sizes)
> > {
> > struct v4l2_subdev_frame_size_enum sizeEnum = {};
> >
> > --- src/libcamera/v4l2_videodev.cpp
> > +++ src/libcamera/v4l2_videodev.cpp
> > @@ -712,7 +712,7 @@
> > }
> >
> > int V4L2Videodev::createPlane(Buffer *buffer, unsigned int planeIndex,
> > - unsigned int length)
> > + unsigned int length)
> > {
> > struct v4l2_exportbuffer expbuf = {};
> > int ret;
> >
> >
> > Because this is just a global rename, There will be lots of other
> > 'alignment issues' from this patch:
> >
> > Such as :
> >
> > > - viewfinder_.dev = V4L2Device::fromEntityName(media,
> > > + viewfinder_.dev = V4L2Videodev::fromEntityName(media,
> > > name_ + " viewfinder");
> >
> > How should we handle that?
> >
> > Would you like to ignore them and fix them up as we go along (perfectly
> > understandable)? Or should we go through them as part of this rename action?
> >
> > Aha - actually I see evidence that you have fixed some or most of them
> > up - so I'll add inline notes when I see any more... <edit, I don't
> > think there are any other than the one above perhaps>
>
> Yes, I tried to tackle them while renaming, I might have missed some.
> Thanks for checking which ones still need to be handled.
>
> > One final point, which I'll leave up to you - could we split the rename
> > from the V4L2Device addition? Or is that too painful now?
> >
> > It would have been nice to see a first patch that did the renames and
> > moves. And then a second patch which introduced and moved/removed code
> > to the new base class.
> >
> > Otherwise, it's very hard to see what the new class adds. (due to bulk
> > of the patch)
> >
> > I can 'gloss over' that last point though.
> >
> >
> > <edit: I appreciate how painful this patch is to work with, but if it's
> > easy to split - it would add a lot of value. This patch shows a diff
> > between V4L2Device and V4L2Videodev, and 'adds' V4L2Videodev instead of
> > diffing V4L2Videodev and adding the new V4L2Device>
>
> Splitting might indeed be desirable, and as you've offered to look
> into doing that, I surely won't oppose :)
It would indeed be easier to review.
> > > ---
> > >
> > > include/libcamera/buffer.h | 2 +-
> > > src/libcamera/camera_sensor.cpp | 4 +-
> > > src/libcamera/include/camera_sensor.h | 4 +-
> > > src/libcamera/include/v4l2_device.h | 170 +--
> > > .../{v4l2_subdevice.h => v4l2_subdev.h} | 25 +-
> > > src/libcamera/include/v4l2_videodev.h | 183 ++++
> > > src/libcamera/meson.build | 6 +-
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 32 +-
> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 +-
> > > src/libcamera/pipeline/uvcvideo.cpp | 6 +-
> > > src/libcamera/pipeline/vimc.cpp | 6 +-
> > > src/libcamera/v4l2_device.cpp | 983 ++---------------
> > > .../{v4l2_subdevice.cpp => v4l2_subdev.cpp} | 100 +-
> > > src/libcamera/v4l2_videodev.cpp | 992 ++++++++++++++++++
> > > test/meson.build | 4 +-
> > > .../list_formats.cpp | 6 +-
> > > .../meson.build | 8 +-
> > > .../test_formats.cpp | 6 +-
> > > .../v4l2_subdev_test.cpp} | 12 +-
> > > .../v4l2_subdev_test.h} | 10 +-
> > > .../buffer_sharing.cpp | 18 +-
> > > .../capture_async.cpp | 6 +-
> > > .../double_open.cpp | 8 +-
> > > .../formats.cpp | 8 +-
> > > .../meson.build | 8 +-
> > > .../request_buffers.cpp | 6 +-
> > > .../stream_on_off.cpp | 6 +-
> > > .../v4l2_videodev_test.cpp} | 8 +-
> > > .../v4l2_videodev_test.h} | 8 +-
> > > 29 files changed, 1397 insertions(+), 1254 deletions(-)
> > > rename src/libcamera/include/{v4l2_subdevice.h => v4l2_subdev.h} (67%)
> > > create mode 100644 src/libcamera/include/v4l2_videodev.h
> > > rename src/libcamera/{v4l2_subdevice.cpp => v4l2_subdev.cpp} (79%)
> > > create mode 100644 src/libcamera/v4l2_videodev.cpp
> > > rename test/{v4l2_subdevice => v4l2_subdev}/list_formats.cpp (95%)
> > > rename test/{v4l2_subdevice => v4l2_subdev}/meson.build (57%)
> > > rename test/{v4l2_subdevice => v4l2_subdev}/test_formats.cpp (93%)
> > > rename test/{v4l2_subdevice/v4l2_subdevice_test.cpp => v4l2_subdev/v4l2_subdev_test.cpp} (84%)
> > > rename test/{v4l2_subdevice/v4l2_subdevice_test.h => v4l2_subdev/v4l2_subdev_test.h} (76%)
> > > rename test/{v4l2_device => v4l2_videodev}/buffer_sharing.cpp (90%)
> > > rename test/{v4l2_device => v4l2_videodev}/capture_async.cpp (92%)
> > > rename test/{v4l2_device => v4l2_videodev}/double_open.cpp (76%)
> > > rename test/{v4l2_device => v4l2_videodev}/formats.cpp (86%)
> > > rename test/{v4l2_device => v4l2_videodev}/meson.build (75%)
> > > rename test/{v4l2_device => v4l2_videodev}/request_buffers.cpp (78%)
> > > rename test/{v4l2_device => v4l2_videodev}/stream_on_off.cpp (79%)
> > > rename test/{v4l2_device/v4l2_device_test.cpp => v4l2_videodev/v4l2_videodev_test.cpp} (91%)
> > > rename test/{v4l2_device/v4l2_device_test.h => v4l2_videodev/v4l2_videodev_test.h} (82%)
[snip]
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list