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

[Laurent Pinchart]

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 ?

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