Fix use of custom resolvers with iostream

[chouquette]

The current code enforces a code path that will use fopen to convert a FileName to an actual File instance, even when FileRef::parse(IOStream*, ...) is used, which defeats the purpose of having an IOStream overload.

For reference, the associated issue on VLC's side is https://code.videolan.org/videolan/vlc/-/issues/26602

[sbooth]

These changes make sense to me. I'll leave the merge decision to @ufleisch.

[ufleisch]

I will look at it a bit more thoroughly, but adding a new abstract method to FileTypeResolver looks like breaking all client code implementing a custom FileTypeResolver.

[chouquette]

Indeed it would.

I can think of two options:

• Add a default implementation for the IOStream overload. That should prevent build breakages, but is likely to be a bit kludgy and will still break the ABI
• Add a new interface, which shouldn't break the API nor the ABI

If that's OK with you I'll try to add a new interface and see how it looks

[ufleisch]

Possibly a third option might be:

• Add a new class (e.g. StreamTypeResolver) which derives from FileTypeResolver and adds your new abstract method (maybe it would be better to give it another name, e.g. createFileFromStream()). Instances of such a class could still be registered using FileRef::addFileTypeResolver(), but internally we would have to do some dynamic_cast checks and probably have another list of StreamTypeResolvers. Maybe the FileTypeResolver could be marked deprecated and then be removed when TagLib makes the ABI breaking step. I have not verified this idea with code, I am just thinking loud...

[chouquette]

Indeed that seems much cleaner to me! Thanks for the suggestion, I updated the MR with this

[ufleisch]

Suggested change

	File *detectByResolvers(IOStream* stream, bool readAudioProperties,
	AudioProperties::ReadStyle audioPropertiesStyle)
	{
	ResolverList::ConstIterator it = fileTypeResolvers.begin();
	for(; it != fileTypeResolvers.end(); ++it) {
	const FileRef::StreamTypeResolver* streamResolver =
	dynamic_cast<const FileRef::StreamTypeResolver*>(*it);
	if (!streamResolver)
	continue;
	File *file = streamResolver->createFileFromStream(stream, readAudioProperties, audioPropertiesStyle);
	if(file)
	return file;
	}
	return 0;
	File *detectByResolvers(IOStream* stream, bool readAudioProperties,
	AudioProperties::ReadStyle audioPropertiesStyle)
	{
	for(ResolverList::ConstIterator it = fileTypeResolvers.begin();
	it != fileTypeResolvers.end(); ++it) {
	if(const FileRef::StreamTypeResolver *streamResolver =
	dynamic_cast<const FileRef::StreamTypeResolver*>(*it)) {
	if(File *file = streamResolver->createFileFromStream(
	stream, readAudioProperties, audioPropertiesStyle)) {
	return file;
	}
	}
	}
	return 0;

I think it is better to narrow the scope of variables and only use continue when it would result in more readable code.

[chouquette]

I'm fine with that change, but if I'm not mistaken this would require C++17 while taglib only requires C++11

[scotchi]

I don't think TagLib actually requires C++11. At least in the not terribly recent past, it was all still C++98, and I'd removed the couple places that had C++11-isms.

[chouquette]

Oh my bad, I was looking at the tests CMakeLists.txt, not the one from the library itself.

Also my bad, I just realized that the proposed sample is not in a if statement with initializer so it's fine in C++98/03, I'll push a fix

[ufleisch]

If there is only a FileTypeResolver registered (and not a StreamTypeResolver!), detectByResolvers() will return 0 instead of detecting by file name. So I would keep the check by file type after checking by stream.

  // Try user-defined stream type resolvers.

  d->file = detectByResolvers(stream, readAudioProperties, audioPropertiesStyle);
  if(d->file)
    return;

  // Try user-defined file type resolvers.

  d->file = detectByResolvers(stream->name(), readAudioProperties, audioPropertiesStyle);
  if(d->file)
    return;

[chouquette]

Very good point, fixed

[ufleisch]

Suggested change

	{
	return new Ogg::Vorbis::File(fileName);
	}
	virtual File *createFile(IOStream* s, bool, AudioProperties::ReadStyle) const
	{
	return new Ogg::Vorbis::File(s);
	}
	};
	{
	return new Ogg::Vorbis::File(fileName);
	}
	};
	class DummyStreamResolver : public FileRef::StreamTypeResolver
	{
	public:
	virtual File *createFile(FileName, bool, AudioProperties::ReadStyle) const
	{
	return 0;
	}
	virtual File *createFileFromStream(IOStream *s, bool, AudioProperties::ReadStyle) const
	{
	return new MP4::File(s);
	}
	};

This will not really work to test your resolver. First you have to implement a StreamTypeResolver and not just add a virtual method to a FileTypeResolver (and the method must be createFileFromStream()). You should also keep the test with a file type resolver to make sure it still works with them. The other problem is that with the existing test, createFileFromStream() will never be called, you have to explicitly create a FileRef by stream, so add another test to testFileResolver:

  void testFileResolver()
  {
    {
      FileRef f(TEST_FILE_PATH_C("xing.mp3"));
      CPPUNIT_ASSERT(dynamic_cast<MPEG::File *>(f.file()) != NULL);
    }

    DummyResolver resolver;
    FileRef::addFileTypeResolver(&resolver);

    {
      FileRef f(TEST_FILE_PATH_C("xing.mp3"));
      CPPUNIT_ASSERT(dynamic_cast<Ogg::Vorbis::File *>(f.file()) != NULL);
    }

    DummyStreamResolver streamResolver;
    FileRef::addFileTypeResolver(&streamResolver);

    {
      FileStream s(TEST_FILE_PATH_C("xing.mp3"));
      FileRef f(&s);
      CPPUNIT_ASSERT(dynamic_cast<MP4::File *>(f.file()) != NULL);
    }
  }

[chouquette]

Indeed, thanks for providing the test code! Should be fixed in the new PR version

[chouquette]

All remarks should be addressed now, let me know if you want me to squash some changes together