[libcamera-devel] [PATCH v2 3/3] test: media_device: Add link handling test

[Laurent Pinchart]

Hi Jacopo,

On Friday, 11 January 2019 20:06:09 EET Jacopo Mondi wrote:
> On Fri, Jan 11, 2019 at 07:20:30PM +0200, Laurent Pinchart wrote:
> > On Friday, 11 January 2019 15:27:05 EET Jacopo Mondi wrote:
> >> Add a test unit that exercise link handling on the VIMC media graph.
> >> 
> >> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >> ---
> >> 
> >>  test/media_device/media_device_link_test.cpp | 188 +++++++++++++++++++
> >>  test/media_device/meson.build                |   1 +
> >>  2 files changed, 189 insertions(+)
> >>  create mode 100644 test/media_device/media_device_link_test.cpp
> >> 
> >> diff --git a/test/media_device/media_device_link_test.cpp
> >> b/test/media_device/media_device_link_test.cpp new file mode 100644
> >> index 0000000..a335a1b
> >> --- /dev/null
> >> +++ b/test/media_device/media_device_link_test.cpp

[snip]

> >> +class MediaDeviceLinkTest : public Test
> >> +{
> >> +	int init()
> >> +	{
> >> +		enumerator_ = DeviceEnumerator::create();
> > 
> > Should you check for !enumerator_ ?
> > 
> >> +		if (enumerator_->enumerate())
> > 
> > You may want to delete enumerator_ here, or possibly better use
> > std::unique_ptr<>.
> 
> Good point, it also does not need to be class member as it is used at
> initialization time only.

Except that it owns the media device pointers, so it has to stay alive until 
the end of the test.

> >> +			return TestFail;
> >> +
> >> +		DeviceMatch dm("vimc");
> >> +		dev_ = enumerator_->search(dm);
> >> +		if (!dev_)
> >> +			return TestSkip;
> > 
> > How about printing a message to explain the reason ? Same for the two
> > failure cases in this function.
> 
> Indeed.
> 
> >> +
> >> +		dev_->acquire();
> >> +
> >> +		if (dev_->open())
> >> +			return TestFail;
> 
> The library already logs this.
> 
> >> +
> >> +		return 0;
> >> +	}
> >> +
> > > +	int run()
> >> +	{
> >> +		/* First of all reset all links in the media graph. */
> > 
> > How about "First of all disable all links in the graph to ensure we start
> > from a known state." ?
> > 
> >> +		int ret = dev_->disableLinks();
> >> +		if (ret)
> > 
> > Please print a message explaining the failure.
> 
> Those errors are logged by the library with proper printing of the
> ioclt returned error.

Shouldn't a message still be printed here ? Errors in the library don't 
necessarily translate to test failures (think about Kieran's double open test 
that succeeds if the second open call fails), so it would make sense to keep 
the two separate. The library log could also be directed to a file, and I 
think it would then make sense for the test output to be self-contained.

> >> +			return TestFail;
> >> +
> >> +		/*
> >> +		 * Test if link can be consistently retrieved through the
> >> +		 * different methods the media device offers.
> >> +		 */
> >> +		MediaLink *link = dev_->link("Debayer A", 1, "Scaler", 0);
> >> +		if (!link) {
> >> +			cerr << "Unable to find link \"Debayer A\"[1] ->"
> > 
> > Would it be more readable to use single quotes instead of double quotes,
> > to avoid the escape sequences ?
> 
> To me is equally readable, if you prefer to I can change this.

Up to you. I think the most important part is to decide on a syntax and stick 
to it throughout the code.

> >> +			     << "\"Scaler\"[0]" << endl
> > 
> > Maybe adding a "using lookup by name" ? (and "lookup by entity" and
> > "lookup by pad" below)
> > 
> >> +			     << "This link exists in VIMC media graph"
> > 
> > Do you think the second sentence is needed ?
> 
> With the comment at file beginning it is not.
> 
> >> +			     << endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		MediaEntity *source = dev_->getEntityByName("Debayer A");
> >> +		if (!source) {
> >> +			cerr << "Unable to find entity \"Debayer A\"" << endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		MediaEntity *sink = dev_->getEntityByName("Scaler");
> >> +		if (!sink) {
> >> +			cerr << "Unable to find entity \"Scaler\"" << endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		MediaLink *link2 = dev_->link(source, 1, sink, 0);
> >> +		if (!link2) {
> >> +			cerr << "Unable to find link \"Debayer A\"[1] ->"
> >> +			     << "\"Scaler\"[0]" << endl
> >> +			     << "This link exists in VIMC media graph"
> >> +			     << endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		if (link != link2) {
> >> +			cerr << "The returned link does not match what expected"
> > 
> > "Link lookup by name and by entity don't match" ?
> 
> Better, gives more context indeed.
> 
> >> +			     << endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		link2 = dev_->link(source->getPadByIndex(1),
> >> +				   sink->getPadByIndex(0));
> >> +		if (!link2) {
> >> +			cerr << "Unable to find link \"Debayer A\"[1] ->"
> >> +			     << "\"Scaler\"[0]" << endl
> >> +			     << "This link exists in VIMC media graph"
> >> +			     << endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		if (link != link2) {
> >> +			cerr << "The returned link does not match what expected"
> > 
> > "Link lookup by name and by pad don't match" ?
> > 
> >> +			     << endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		/* After reset the link shall not be enabled. */
> >> +		if (link->flags() & MEDIA_LNK_FL_ENABLED) {
> >> +			cerr << "Link \"Debayer A\"[1] -> \"Scaler\"[0]"
> >> +			     << " should not be enabled after a device reset"
> > 
> > Is this a link that was enabled by default by the driver ? If not, is
> > there a link enabled by default that you could use instead of this one ?
> 
> It is:
> 
> - entity 5: Debayer A (2 pads, 2 links)
>             type V4L2 subdev subtype Unknown flags 0
>             device node name /dev/v4l-subdev2
> 	pad0: Sink
> 		[fmt:RGB888_1X24/640x480 field:none]
> 		<- "Sensor A":0 [ENABLED,IMMUTABLE]
> 	pad1: Source
> 		[fmt:RGB888_1X24/640x480 field:none]
> 		-> "Scaler":0 [ENABLED]

Perfect :-)

> >> +			     << endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		/* Enable the link and test if enabling was successful. */
> >> +		ret = link->setEnabled(true);
> >> +		if (ret)
> > 
> > Please log the cause of failure.
> 
> Failed operations on the media device are logged by the library with
> proper printout of the returned errno.
> 
> >> +			return TestFail;
> >> +
> >> +		if (!(link->flags() & MEDIA_LNK_FL_ENABLED)) {
> >> +			cerr << "Link \"Debayer A\"[1] -> \"Scaler\"[0]"
> > 
> > Maybe you want a const std::string linkName("'Debayer A'[1] ->
> > 'Scaler'[0]") declared at the top to use it through the code, to avoid
> > repeating this over and over ? On the other hand you lookup other links
> > below, so you'd have to change the linkName. Starting test blocks with
> > 
> > 	linkName = "'Debayer A'[1] -> 'Scaler'[0]";
> > 
> > maybe clarify what the code below does. Up to you.
> 
> I'll see how it looks like.
> 
> >> +			     << " should now be enabled" << endl;
> > 
> > This sound more like a recommendation than an error to me. Maybe "Link ...
> > was enabled but is reported as disabled" ?
> 
> Ok
> 
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		/* Disable the link and test if disabling was successful. */
> >> +		ret = link->setEnabled(false);
> >> +		if (ret)
> > 
> > Missing error message here too. I like Kieran's idea of creating a
> > TestFail object that would require a message :-)
> 
> I see. In case where the library fails vocally already, I don't think
> it is necessary.
> 
> >> +			return TestFail;
> >> +
> >> +		if (link->flags() & MEDIA_LNK_FL_ENABLED) {
> >> +			cerr << "Link \"Debayer A\"[1] -> \"Scaler\"[0]"
> >> +			     << " should now be disabled" << endl;
> > 
> > Same as above.
> > 
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		/* Try to get a non existing link. */
> >> +		link = dev_->link("Sensor A", 1, "Scaler", 0);
> >> +		if (link) {
> >> +			cerr << "Link \"Sensor A\"[1] -> \"Scaler\"[0]"
> >> +			     << " does not exist but something was returned"
> > 
> > "Lookup succeeded for link ... that does not exist in the graph" ?
> > 
> >> +			     << endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		/* Now get an immutable link and try to disable it. */
> >> +		link = dev_->link("Sensor A", 0, "Raw Capture 0", 0);
> >> +		if (!link) {
> >> +			cerr << "Unable to find link \"Sensor A\"[0] -> "
> >> +			     << "\"Raw Capture 0\"[0]" << endl
> >> +			     << "This link exists in VIMC media graph"
> >> +			     << endl;
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		if (!(link->flags() & MEDIA_LNK_FL_IMMUTABLE)) {
> >> +			cerr << "Link \"Sensor A\"[0] -> \"Raw Capture 0\"[0]"
> >> +			     << " should have been 'IMMUTABLE'" << endl;
> > 
> > "should be" ?
> > 
> >> +			return TestFail;
> >> +		}
> >> +
> >> +		/* Disabling an immutable link shall fail. */
> >> +		ret = link->setEnabled(false);
> >> +		if (!ret) {
> >> +			cerr << "Link \"Sensor A\"[0] -> \"Raw Capture 0\"[0]"
> >> +			     << " is 'IMMUTABLE', it shouldn't be disabled"
> > 
> > "Disabling immutable ... link incorrectly succeeded" ?
> > 
> >> +			     << endl;
> >> +			return TestFail;
> >> +		}
> > 
> > I think one last test that enables a link, calls disableLinks() and
> > verifies that the link is disabled would be useful.
> 
> Isn't this already verified by making sure a link enabled by default
> is disabled after a disableLinks() call?

Partly only as the vimc device could have that link disabled already (and will 
if you run this test multiple times).

> I can add it here though, it does not hurt.
> 
> >> +		return 0;
> >> +	}
> >> +
> >> +	void cleanup()
> >> +	{
> >> +		dev_->close();
> >> +		dev_->release();
> >> +
> >> +		delete dev_;
> > 
> > The media device belongs to the enumerator, you should not delete it.
> 
> Correct, my bad. I wonder why the following delete didn't fail (or did
> it silently maybe)

Try running the test with valgrind :-)

> >> +		delete enumerator_;
> >> +	}
> >> +
> >> +private:
> >> +	DeviceEnumerator *enumerator_;
> >> +	MediaDevice *dev_;
> >> +};
> >> +
> >> +TEST_REGISTER(MediaDeviceLinkTest);

[snip]

-- 
Regards,

Laurent Pinchart