[libcamera-devel] [PATCH 4/5] test: v4l2_device: Reset media links and set a resolution

[Niklas Söderlund]

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