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

[Daniel Scally]

Hi Laurent

On 08/12/2021 08:26, Laurent Pinchart wrote:
> Hi Dan,
> 
> Thank you for the patch.
> 
> 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.

>> +			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;
>