| Message ID | 20211207224512.753979-4-djrscally@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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;
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@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; >
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@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;
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)); + 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;
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@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(-)