[libcamera-devel] [PATCH v3 3/9] libcamera: media_device: Handle ancillary links in populateLinks()

[Laurent Pinchart]

Hi Dan,

On Wed, Dec 08, 2021 at 10:10:08PM +0000, Daniel Scally wrote:
> On 08/12/2021 08:26, Laurent Pinchart wrote:
> > On Tue, Dec 07, 2021 at 10:45:06PM +0000, Daniel Scally wrote:
> >> The populateLinks() function can't currently handle ancillary links
> >> which causes an error to be thrown in process() when the MediaObject
> >> cannot be cast to a MediaPad.
> >>
> >> Add explicit handling for the different link types, creating either
> >> pad-2-pad links or else storing the pointer to the ancillary device
> >> MediaEntity in the ancillaryEntities_ member of the primary.
> >>
> >> Signed-off-by: Daniel Scally <djrscally at gmail.com>
> >> ---
> >> Changes in v3:
> >>
> >> 	- Split out the new macro
> >> 	- Fixed some style errors and comments
> >> 	- Added a default case
> >>
> >>  src/libcamera/media_device.cpp | 55 ++++++++++++++++++++++++----------
> >>  1 file changed, 39 insertions(+), 16 deletions(-)
> > 
> > I'm afraid this patch now conflicts, could you please rebase it ?
> > 
> >> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> >> index 4df0a27f..fb332445 100644
> >> --- a/src/libcamera/media_device.cpp
> >> +++ b/src/libcamera/media_device.cpp
> >> @@ -693,45 +693,68 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology)
> >>  {
> >>  	struct media_v2_link *mediaLinks = reinterpret_cast<struct media_v2_link *>
> >>  					   (topology.ptr_links);
> >> +	MediaEntity *ancillary;
> >> +	MediaEntity *primary;
> >> +	MediaLink *link;
> >>  
> >>  	for (unsigned int i = 0; i < topology.num_links; ++i) {
> >>  		/*
> >> -		 * Skip links between entities and interfaces: we only care
> >> -		 * about pad-2-pad links here.
> >> +		 * Skip links between entities and interfaces: interfaces are
> >> +		 * not created as MediaObjects at this time, so the source and
> >> +		 * sink pointers would never be found.
> >>  		 */
> >>  		if ((mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) ==
> >>  		    MEDIA_LNK_FL_INTERFACE_LINK)
> >>  			continue;
> >>  
> >> -		/* Store references to source and sink pads in the link. */
> >> +		/* Look up the source and sink objects. */
> >>  		unsigned int source_id = mediaLinks[i].source_id;
> >> -		MediaPad *source = dynamic_cast<MediaPad *>
> >> -				   (object(source_id));
> >> +		MediaObject *source = object(source_id);
> >>  		if (!source) {
> >>  			LOG(MediaDevice, Error)
> >> -				<< "Failed to find pad with id: "
> >> +				<< "Failed to find MediaObject with id: "
> >>  				<< source_id;
> >>  			return false;
> >>  		}
> >>  
> >>  		unsigned int sink_id = mediaLinks[i].sink_id;
> >> -		MediaPad *sink = dynamic_cast<MediaPad *>
> >> -				 (object(sink_id));
> >> +		MediaObject *sink = object(sink_id);
> >>  		if (!sink) {
> >>  			LOG(MediaDevice, Error)
> >> -				<< "Failed to find pad with id: "
> >> +				<< "Failed to find MediaObject with id: "
> >>  				<< sink_id;
> >>  			return false;
> >>  		}
> >>  
> >> -		MediaLink *link = new MediaLink(&mediaLinks[i], source, sink);
> >> -		if (!addObject(link)) {
> >> -			delete link;
> >> -			return false;
> >> -		}
> >> +		switch (mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) {
> >> +		case MEDIA_LNK_FL_DATA_LINK:
> >> +			link = new MediaLink(&mediaLinks[i],
> >> +					     dynamic_cast<MediaPad *>(source),
> >> +					     dynamic_cast<MediaPad *>(sink));
> > 
> > We'll crash if the source or sink isn't a pad (dynamic_cast will return
> > nullptr in that case). This shouldn't happen unless the kernel reports
> > an incorrect topology, but it would still be nice to protect against
> > that. How about the following (untested) ?
> > 
> > 		case MEDIA_LNK_FL_DATA_LINK: {
> > 			MediaPad *sourcePad = dynamic_cast<MediaPad *>(source);
> > 			if (!sourcePad) {
> > 				LOG(MediaDevice, Error)
> > 					<< "Source MediaObject " << source_id
> > 					<< " is not a pad";
> > 				return false;
> > 			}
> > 
> > 			MediaPad *sinkPad = dynamic_cast<MediaPad *>(sink);
> > 			if (!sinkPad) {
> > 				LOG(MediaDevice, Error)
> > 					<< "Sink MediaObject " << sink_id
> > 					<< " is not a pad";
> > 				return false;
> > 			}
> > 
> > 			MediaLink *link = new MediaLink(&mediaLinks[i],
> > 							sourcePad, sinkPad);
> > 			if (!addObject(link)) {
> > 				delete link;
> > 				return false;
> > 			}
> > 
> > 			link->source()->addLink(link);
> > 			link->sink()->addLink(link);
> > 
> > 			break;
> > 		}
> > 
> > 		case MEDIA_LNK_FL_ANCILLARY_LINK: {
> > 			MediaEntity *primary = dynamic_cast<MediaEntity *>(source);
> > 			if (!primary) {
> > 				LOG(MediaDevice, Error)
> > 					<< "Source MediaObject " << source_id
> > 					<< " is not an entity";
> > 				return false;
> > 			}
> > 
> > 			MediaEntity *ancillary = dynamic_cast<MediaEntity *>(sink);
> > 			if (!ancillary) {
> > 				LOG(MediaDevice, Error)
> > 					<< "Sink MediaObject " << sink_id
> > 					<< " is not an entity";
> > 				return false;
> > 			}
> > 
> > 			primary->addAncillaryEntity(ancillary);
> > 			break;
> > 		}
> > 
> > Or is it overkill ?
> 
> I lean towards overkill to be honest; since creating data links on the
> kernel side is through media_create_pad_link(), which explicitly sets
> the source and sink to pads. So someone would have to invent a new
> function to add the link in a way that would be broken.
> 
> Happy to do it though if you prefer.

I agree that having detailed messages is overkill, but I'd like to at
least avoid crashing in case something goes wrong (during kernel
development for instance). Maybe the following ?

 		case MEDIA_LNK_FL_DATA_LINK: {
 			MediaPad *sourcePad = dynamic_cast<MediaPad *>(source);
 			MediaPad *sinkPad = dynamic_cast<MediaPad *>(sink);
 			if (!sourcePad || !sinkPad) {
 				LOG(MediaDevice, Error)
 					<< "Source or sink is not a pad";
 				return false;
 			}
 
 			MediaLink *link = new MediaLink(&mediaLinks[i],
 							sourcePad, sinkPad);
 			if (!addObject(link)) {
 				delete link;
 				return false;
 			}
 
 			link->source()->addLink(link);
 			link->sink()->addLink(link);
 
 			break;
 		}
 
 		case MEDIA_LNK_FL_ANCILLARY_LINK: {
 			MediaEntity *primary = dynamic_cast<MediaEntity *>(source);
 			MediaEntity *ancillary = dynamic_cast<MediaEntity *>(sink);
 			if (!primary || !ancillary) {
 				LOG(MediaDevice, Error)
 					<< "Source or sink is not an entity";
 				return false;
 			}
 
 			primary->addAncillaryEntity(ancillary);
 			break;
 		}

> >> +			if (!addObject(link)) {
> >> +				delete link;
> >> +				return false;
> >> +			}
> >> +
> >> +			link->source()->addLink(link);
> >> +			link->sink()->addLink(link);
> >> +
> >> +			break;
> >> +
> >> +		case MEDIA_LNK_FL_ANCILLARY_LINK:
> >> +			primary = dynamic_cast<MediaEntity *>(source);
> >> +			ancillary = dynamic_cast<MediaEntity *>(sink);
> >>  
> >> -		source->addLink(link);
> >> -		sink->addLink(link);
> >> +			primary->addAncillaryEntity(ancillary);
> >> +
> >> +			break;
> >> +
> >> +		default:
> >> +			LOG(MediaDevice, Warning)
> >> +				<< "Unknown media link type";
> >> +
> >> +			break;
> >> +		}
> >>  	}
> >>  
> >>  	return true;

-- 
Regards,

Laurent Pinchart