Page MenuHomePhabricator

IOUtils::LoadSurface and similar methods do not try hard enough
Closed, ResolvedPublic

Description

IOUtils::LoadSurface(filename) currently does:

  • use the first reader that claims to load given filename
  • verify that the result is mitk::Surface, else return a nullptr

This is not sufficient in cases where there are multiple competing MIME types available for the specified file. Imagine that somebody implemented a new mitk::BaseData such as mitk::PrintableModel and provides a MIME type along with a reader for STL file for this new data type. When this new data type is registered with a higher service ranking, LoadSurface will subsequently fail to load STL files. Other methods like LoadImage, LoadPointSet exhibit identical behavior.

I am not sure about the best solution for this, would need input by the designers of the current system. Just two ideas:

  • IOUtil could filter MIME type candidates for _both_ "applies to this file" and "is in that category".
  • I am not sure about the relation between "category" and actually provided BaseData sub-class. If there is no fixed relation between them, IOUtil should probably continue with alternative readers if the first reader fails to provide the expected data type. This could lead to loading a file multiple times in multiple formats but would at least succeed in finally finding one reader that works.

If someone is available to advance this subject I could potentially provide some support.

Event Timeline

This might interest @floca as well as he is currently looking at providing an option to use a specific reader for e.g. tests or command line apps.

Yes, see T22780.

But I think that are somehow different use cases. In my and Peters case. There is problematic reader and prefered reader for an cl application (test or mini app) and we want to ensure that the prefered candidate is taken (if available) and the evil doer is never. So we (just) need an automatized pendant to the option dialog/interaction callback of QmitkIOutil.

If I understand Daniels use case correctly he is brainstorming of a more sophisticated selection strategie in IOutils itself, which is driven be properties given by the service implementer (e.g. via MIME type informations).

In T22608#98864, @floca wrote:

If I understand Daniels use case correctly he is brainstorming of a more sophisticated selection strategie in IOutils itself, which is driven be properties given by the service implementer (e.g. via MIME type informations).

My point is just that there might be multiple readers that read a certain file extension (e.g. STL) and produce different derivations of BaseData. This is fine and already handled correctly by Workbench by proposing the choices in a dialog to the user.

What is missing is a selection strategy in QmitkIOUtil::LoadSurface(). LoadSurface() just does not consider the posibility of multiple readers for the given file but takes the default-reader and gives up if this reader does not deliver a mitk::Surface. This behavior is problematic in the IGT for example where STL files are loaded via QmitkIOUtil::LoadSurface() and where no Surface is produced when an alternative STL reader is present (see T19825).

OK, then may be T22780 could be of interest for you, too. One of the steps will be to allow also in IOutil to specifiy a callback and therefore a decision strategy if multiple readers claim the same mime type.

I think the main difference is that T22780 expects you to know exactly which reader you are looking for. @maleike as I understand him does not care about selecting a specific reader as long as it returns the promised data type.

I agree that it is reasonable to assume LoadSurface() returns a Surface if in any way possible.

@goch I understood the difference the same way.

kislinsk triaged this task as Normal priority.May 30 2017, 8:15 AM

FYI: We removed all deprecated methods from IOUtil which include most overloads of LoadImage/Surface/PointSet.

kislinsk claimed this task.
kislinsk removed a project: Request for Discussion.

Please re-open if I made wrong assumptions:

Load() methods are now templated like IOUtil::Load<Surface>(...). If there's a new data type, it should also have its own MIME type. Also, Load() methods allow to select a certain reader now or at least allow for dynamically doing stuff in a callback function as far as I know. I think, the problems of this task may be addressed by all of this sufficiently?