[libcamera-devel] [PATCH 08/11] libcamera: v4l2-subdevice: Add method to retrieve the media entity
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 16 22:14:04 CEST 2019
Hi Jacopo,
On Tue, Apr 16, 2019 at 05:20:58PM +0200, Jacopo Mondi wrote:
> On Mon, Apr 15, 2019 at 07:56:57PM +0300, Laurent Pinchart wrote:
> > Add a method to retrieve the media entity associated with a subdevice.
> > The entityName() and deviceNode() methods are not needed anymore as they
> > can be accessed through the media entity, remove them.
>
> I might have missed what's the benefit of this patch. I mean, maybe in
> next patches you need entity() and this is fine, but what's the
> benefit of removing deviceNode() and entityName(), if not writing
> longer lines when accessing them?
As you've correctly noted, adding the entity() function is needed. I
then decided to remove the entityName() and deviceNode() functions are
they can be accessed through entity(), without much extra hassle (apart
from slightly longer lines, especially for deviceNode()). I believe that
in general it is best not to offer different ways to access the same
information when those multiple ways have similar complexities,
otherwise it creates more complex APIs for little benefit.
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/libcamera/include/v4l2_subdevice.h | 3 +--
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++--
> > src/libcamera/v4l2_subdevice.cpp | 22 +++++++--------------
> > test/v4l2_subdevice/list_formats.cpp | 6 +++---
> > test/v4l2_subdevice/v4l2_subdevice_test.cpp | 2 +-
> > 5 files changed, 14 insertions(+), 23 deletions(-)
> >
> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > index c71dce7d8644..31bc9d298e5b 100644
> > --- a/src/libcamera/include/v4l2_subdevice.h
> > +++ b/src/libcamera/include/v4l2_subdevice.h
> > @@ -40,8 +40,7 @@ public:
> > bool isOpen() const;
> > void close();
> >
> > - const std::string &deviceNode() const { return entity_->deviceNode(); }
> > - const std::string &entityName() const { return entity_->name(); }
> > + const MediaEntity *entity() const { return entity_; }
> >
> > int setCrop(unsigned int pad, Rectangle *rect);
> > int setCompose(unsigned int pad, Rectangle *rect);
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 4ddd1ede1c81..e3c79f93963e 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -579,7 +579,7 @@ int PipelineHandlerIPU3::registerCameras()
> > &IPU3CameraData::imguOutputBufferReady);
> >
> > /* Create and register the Camera instance. */
> > - std::string cameraName = cio2->sensor_->entityName() + " "
> > + std::string cameraName = cio2->sensor_->entity()->name() + " "
> > + std::to_string(id);
> > std::shared_ptr<Camera> camera = Camera::create(this,
> > cameraName,
> > @@ -1055,7 +1055,7 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> > }
> > }
> > if (maxSize_.width == 0) {
> > - LOG(IPU3, Info) << "Sensor '" << sensor_->entityName()
> > + LOG(IPU3, Info) << "Sensor '" << sensor_->entity()->name()
> > << "' detected, but no supported image format "
> > << " found: skip camera creation";
> > return -ENODEV;
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index e34cc1693b46..6b2d7cce6794 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -126,12 +126,12 @@ int V4L2Subdevice::open()
> > return -EBUSY;
> > }
> >
> > - ret = ::open(deviceNode().c_str(), O_RDWR);
> > + ret = ::open(entity_->deviceNode().c_str(), O_RDWR);
> > if (ret < 0) {
> > ret = -errno;
> > LOG(V4L2Subdev, Error)
> > - << "Failed to open V4L2 subdevice '" << deviceNode()
> > - << "': " << strerror(-ret);
> > + << "Failed to open V4L2 subdevice '"
> > + << entity_->deviceNode() << "': " << strerror(-ret);
> > return ret;
> > }
> > fd_ = ret;
> > @@ -161,17 +161,9 @@ void V4L2Subdevice::close()
> > }
> >
> > /**
> > - * \fn V4L2Subdevice::deviceNode()
> > - * \brief Retrieve the path of the device node associated with the subdevice
> > - *
> > - * \return The subdevice's device node system path
> > - */
> > -
> > -/**
> > - * \fn V4L2Subdevice::entityName()
> > - * \brief Retrieve the name of the media entity associated with the subdevice
> > - *
> > - * \return The name of the media entity the subdevice is associated to
> > + * \fn V4L2Subdevice::entity()
> > + * \brief Retrieve the media entity associated with the subdevice
> > + * \return The subdevice's associated media entity.
> > */
> >
> > /**
> > @@ -338,7 +330,7 @@ V4L2Subdevice *V4L2Subdevice::fromEntityName(const MediaDevice *media,
> >
> > std::string V4L2Subdevice::logPrefix() const
> > {
> > - return "'" + entityName() + "'";
> > + return "'" + entity_->name() + "'";
> > }
> >
> > int V4L2Subdevice::enumPadSizes(unsigned int pad,unsigned int code,
> > diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp
> > index 09dec9abe854..18dd8761a8ab 100644
> > --- a/test/v4l2_subdevice/list_formats.cpp
> > +++ b/test/v4l2_subdevice/list_formats.cpp
> > @@ -52,7 +52,7 @@ int ListFormatsTest::run()
> > formats = scaler_->formats(0);
> > if (formats.empty()) {
> > cerr << "Failed to list formats on pad 0 of subdevice "
> > - << scaler_->entityName() << endl;
> > + << scaler_->entity()->name() << endl;
> > return TestFail;
> > }
> > for (auto it = formats.begin(); it != formats.end(); ++it)
> > @@ -61,7 +61,7 @@ int ListFormatsTest::run()
> > formats = scaler_->formats(1);
> > if (formats.empty()) {
> > cerr << "Failed to list formats on pad 1 of subdevice "
> > - << scaler_->entityName() << endl;
> > + << scaler_->entity()->name() << endl;
> > return TestFail;
> > }
> > for (auto it = formats.begin(); it != formats.end(); ++it)
> > @@ -71,7 +71,7 @@ int ListFormatsTest::run()
> > formats = scaler_->formats(2);
> > if (!formats.empty()) {
> > cerr << "Listing formats on non-existing pad 2 of subdevice "
> > - << scaler_->entityName()
> > + << scaler_->entity()->name()
> > << " should return an empty format list" << endl;
> > return TestFail;
> > }
> > diff --git a/test/v4l2_subdevice/v4l2_subdevice_test.cpp b/test/v4l2_subdevice/v4l2_subdevice_test.cpp
> > index 06582969eb45..dfcf779af95e 100644
> > --- a/test/v4l2_subdevice/v4l2_subdevice_test.cpp
> > +++ b/test/v4l2_subdevice/v4l2_subdevice_test.cpp
> > @@ -66,7 +66,7 @@ int V4L2SubdeviceTest::init()
> > ret = scaler_->open();
> > if (ret) {
> > cerr << "Unable to open video subdevice "
> > - << scaler_->deviceNode() << endl;
> > + << scaler_->entity()->deviceNode() << endl;
> > media_->release();
> > return TestSkip;
> > }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list