[libcamera-devel] [PATCH 4/5] test: v4l2_device: Reset media links and set a resolution
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sat May 11 13:03:48 CEST 2019
Hi Kieran, Laurent,
Thanks for your feedback.
On 2019-05-11 05:27:31 +0300, Laurent Pinchart wrote:
> Hello,
>
> On Thu, May 09, 2019 at 10:30:41AM +0100, Kieran Bingham wrote:
> > On 08/05/2019 17:58, Niklas Söderlund wrote:
> > > When initializing the test reset any media links and set a know
> > > resolutions. This is needed to put the device under test into known
> > > state and not have the v4l2 device tests depend on that no one have
> > > touched the device before the test is executed.
> > >
> > > The resolution is picked purely at random and could possibly be moved to
> > > each test case if there is a need for different resolutions for a
> > > specific one.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > > test/v4l2_device/v4l2_device_test.cpp | 19 ++++++++++++++++++-
> > > 1 file changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp
> > > index ee5a8e009bef2a5e..5bd80a6c68d796b6 100644
> > > --- a/test/v4l2_device/v4l2_device_test.cpp
> > > +++ b/test/v4l2_device/v4l2_device_test.cpp
> > > @@ -54,7 +54,24 @@ int V4L2DeviceTest::init()
> > > if (!capture_)
> > > return TestFail;
> > >
> > > - return capture_->open();
> > > + media_->acquire();
> >
> > Should you check the return code of acquire?
> >
> > Does it block using lockf() internally?
>
> It won't block, it will return an error, so I think it's a good idea to
> check for that.
Good catch, I will add a check for the return code.
>
> > > + if (media_->disableLinks())
> >
> > Where do we recreate / re-enable the links?
> >
> > or does it reset to a sane default?
>
> It won't enable links, so any test depending on this would have to
> enable links manually.
Yes, each test who inherits from the base would need to enable links if
it depends on them being enabled.
>
> > > + return TestFail;
> >
> > Is it an issue here that the media_ is not released? I now can't recall
> > if the acquire/release is about locking globally using lockf() or if
> > it's just a state machine state change.
> >
> > If it must be released - perhaps we should do some sort of auto-lock
> > release by using the destructor of a local object, so that it releases
> > when it goes out of scope.
>
> lockf() will not persist once the file descriptor is closed, so it
> shouldn't be a problem, but proper error handling is still a good idea.
I feel that handling the error here and call release() if we can't reset
the links is a bit overkill for a test case and reduces the readability
of the code for little gain. I do however not feel strongly about this
and will do so in the next version.
>
> With those issues fixed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > > + media_->release();
> > > +
> > > + if (capture_->open())
> > > + return TestFail;
> > > +
> > > + V4L2DeviceFormat format = {};
> > > + if (capture_->getFormat(&format))
> > > + return TestFail;
> > > +
> > > + format.size.width = 640;
> > > + format.size.height = 480;
> > > + if (capture_->setFormat(&format))
> > > + return TestFail;
> > > +
> > > + return TestPass;
> > > }
> > >
> > > void V4L2DeviceTest::cleanup()
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list