SoundSource Plugin API: Version 8 #578

uklotzde · 2015-05-07T15:10:22Z

Fixes https://bugs.launchpad.net/mixxx/+bug/1452651

File names for SoundSource plugins were truncated at the first occurrence of '#' due to an implicit conversion from QString to QUrl.

A simple C++ API that reduces the chance of memory leaks that were imminent in the previous C/C++ version. Error handling and logging in SoundSourceProxy while loading plugins has been improved.

uklotzde · 2015-05-14T17:23:47Z

I've bumped the SoundSource plugin API version from 7 to 8, re-defined the public library functions, and did some cleanup.

SoundSourceProvider is a generic interface that will implemented both by plugin libraries as well as built-in sound sources. This will simplify the selection of an appropriate SoundSource in SoundSourceProxy and is useful for future extensions like selecting one out of multiple providers for each file type.

uklotzde · 2015-05-18T17:55:09Z

What has begun as a simple bug fix is now a refactoring of the SoundSource plugin framework ;)

At least this is how it works in 1.12.

daschuer · 2015-05-20T20:19:21Z

src/sources/urlresource.h

    inline QString getLocalFileName() const {
+        DEBUG_ASSERT(isLocalFile());


What happens on windows with network shares?
There is a pending bug for SMB locations
https://bugs.launchpad.net/mixxx/+bug/918222
Will this fail here?

uklotzde · 2015-06-04T11:42:03Z

@daschuer: I've reset the branch to the previous state without the priority extension. Adding the priority later will require to bump the plugin version from 8 to 9, but this is ok. We will not run out of version numbers soon ;)

The priority is only a "hint" to provide a reasonable default behavior without requiring any extra configuration by the user or some sort of GUI.

rryan · 2015-06-04T13:35:42Z

src/library/coverartutils.h

-        if (trackLocation.isEmpty()) {
-            return QImage();
-        }
+        DEBUG_ASSERT(!trackLocation.isEmpty());


Generally we should prefer DEBUG_ASSERT_AND_HANDLE.

DEBUG_ASSERT_AND_HANDLE(...) { return QImage(); }

Empty file names and non-existent files are handled gracefully by SoundSourceProxy, we don't need to handle these errors here again. Instead of handling it twice I would prefer to remove the DEBUG_ASSERT entirely.

Empty file names are handled gracefully by SoundSourceProxy.

rryan · 2015-06-04T14:11:37Z

src/sources/soundsourceprovider.h

+
+#include "sources/soundsource.h"
+
+#include <QStringList>


Missing include for QUrl, QSharedPointer, QString

rryan · 2015-06-04T14:27:21Z

Thanks @uklotzde !

When Albert and I worked on the original plugin API we took care not to pass heap allocated objects across the DLL / binary boundary and free them on the other side. On Windows, this will cause immediate segfaults if the DLL and the binary are linked to different runtimes. You can see an example of this style with the old supportedFileExtensions/freeFileExtensions methods.

I notice that the 1.12 branch (pre-SS-refactor) commits a huge sin here by allocating the heap-allocated SoundSource itself in the plugin and freeing it in Mixxx so I think we've gotten sloppy over the years in this regard.

I'm worried about the use of QSharedPointer/QStrings in this version of the API. When the DLL allocates a QSharedPointer/QString and passes it to the main Mixxx binary, I believe the code executing to handle that object is the Mixxx binary's copy of QString/QSharedPointer code. Correct me if I'm wrong!

So when the QSharedPointer destructor runs, the Mixxx binary QSharedPointer destructor is what is actually running, which will free the SoundSourceProvider using the Mixxx heap, not the DLL heap.

Have you given this any thought?

uklotzde · 2015-06-04T15:25:35Z

@rryan
Well, I know that special care has to be taken with DLLs and memory allocation. But my development experience on Windows lies more than 10 years behind in the past ;)

As you mentioned I've seen that many resources are allocated on one side and freed on the other: The SoundSource itself, QString with metadata, QImage with cover art, .... Only doing it right for the file extensions didn't seem to make any sense to me?! So I assumed that this doesn't matter here.

uklotzde · 2015-06-04T22:22:04Z

The C++ operators new and delete are now called symmetrically from within the external plugin libraries and should use the same C/C++ runtime. Unfortunately I'm not able to test if this works as expected for Windows DLLs.

uklotzde · 2015-06-05T07:16:54Z

@rryan
All invocations of operator new are now complemented by the corresponding invocation of operator delete within the same DLL. Allocations within Qt shouldn't matter here if Qt is doing it correctly in their DLL.

daschuer · 2015-06-05T08:09:47Z

plugins/soundsourcem4a/soundsourcem4a.cpp

+
+// Singleton: SoundSourceProviderM4A is stateless
+// and a single instance can safely be shared
+Mixxx::SoundSourceProviderM4A SOUNDSOURCE_PROVIDER;


static variables should be camel case and prefixed with s

In general, we should avoid static variables in libraries.
Can't we make it a static local of Mixxx_SoundSourcePluginAPI_getSoundSourceProvider

@daschuer Ok, using a static local variable is a viable option

daschuer · 2015-06-05T08:39:34Z

Allocations within Qt shouldn't matter here if Qt is doing it correctly in their DLL.

I want to try to understand the issue. Here some thoughts, please correct me If I am wrong.

I wonder how Qt can do this with implicit sharing lets say in QString.
I think this will be linked by the Application and by the DLL to the QT DLL with their matching, but different memory model, finally to different implementations to malloc.
If now a QString is transfered from the APP to the dll, the QString content is allocated in the senders
heap model but might be deleted in the receivers model -> crash!

Is that correct?

If yes, We can use Qt containers only, if the linked used QT dlls are compiled with the same memory model. This means we cannot switch the memory model if we (only hypothetically) split out the plug-in builds. Since we can't do this anyway, we do not need to worry about a different memory models.

So we have finally no issues here, Right?

uklotzde · 2015-06-05T08:59:15Z

@daschuer Both app and plugin must be linked against the same shared Qt DLL. Then alll internal invocations of operator new/delete in Qt classes use the same C++ runtime and match. What happens in this library stays in this library ;)

We only need to take care of managed pointers. They have to be fed with the appropriate deleter upon construction. This ensures that operator delete from the plugin is called instead of that from the app. This approach should work even when the plugins are compiled separately.

daschuer · 2015-06-05T10:15:22Z

Thanks for clarifying!

rryan · 2015-06-05T13:17:30Z

I'm considering the case where some external party releases a soundsource plugin DLL. I had been assuming they were statically linking their own version of Qt -- but I guess that's prone to causing other problems anyway so we may require that they dynamically link to Qt.

If we accept that they have to dynamically link to Qt (so they are using our Qt DLLs once we load their DLL) -- then I agree the heap used for shared Qt objects will be the one we chose when we compiled Qt.

rryan · 2015-06-05T13:26:35Z

Thanks for the changes @uklotzde!

SoundSource Plugin API: Version 8

uklotzde added 5 commits May 7, 2015 16:54

Log warning about unreadable tags in audio files

b5de6e2

Change parameter type of getSoundSource() from QString to QUrl

9819302

Merge branch 'master' into SoundSourcePluginUrlFileNameFix

c597263

Add an assertion to detect invalid conversions QString <-> QUrl

e40c574

New streamlined SoundSource plugin API version 8

525205c

A simple C++ API that reduces the chance of memory leaks that were imminent in the previous C/C++ version. Error handling and logging in SoundSourceProxy while loading plugins has been improved.

uklotzde changed the title ~~SoundSource Plugins: Fix invalid truncation of file names~~ SoundSource Plugin API: Version 8 May 14, 2015

uklotzde added 15 commits May 15, 2015 14:53

Detach dynamic library management from SoundSourceProxy

e2dfe93

Add and register a SoundSourceProvider for each SoundSource

a0fec6a

Fix FFmpeg build

e12dae4

Give SoundSource providers a name

094b9b5

Delete obsolete #include directive

569134d

Wrap static members from SoundSourceProxy into class

5ecaa8f

Fix static initialization of SoundSource providers in tests

ba6187f

Log detailed debug info about SoundSource providers

6ba1702

Fix SoundSource plugin warning

956028c

Move final debug logging out of loop

2dd3797

Delete obsolete member from SoundSourceProxy

d4c3b9d

Avoid unnecessary locking in SoundSourceProxy

e4287ae

Eliminate duplicate initialization in tests

9571e62

Add debug logging for seek test

8d3e7f0

uklotzde added 3 commits May 19, 2015 08:11

Simplify SoundSource plugin registration

df2a577

Delete obsolete member functions and code cleanup

cd9bb73

Use plugin file name filter only for Apple OS X

7fcd712

At least this is how it works in 1.12.

daschuer reviewed May 20, 2015
View reviewed changes

Initialize FFmpeg before registering providers and loading plugins

05eaa7e

rryan reviewed Jun 4, 2015
View reviewed changes

Delete redundant DEBUG_ASSERT

835189c

Empty file names are handled gracefully by SoundSourceProxy.

rryan reviewed Jun 4, 2015
View reviewed changes

uklotzde added 4 commits June 4, 2015 23:46

Restore comment about QApplication in tests

c6a45e6

Reorder and add missing #include directives

e21a476

Static allocation of SoundSourceProvider in plugins

9252d7d

Symmetrically allocate and delete SoundSource from within plugins

6e26a18

uklotzde added 3 commits June 5, 2015 08:24

Merge branch 'master' into SoundSourcePluginUrlFileNameFix

5785703

Use C+11 override keyword

c4fdd0a

Add comments about singleton/shared SoundSourceProvider instances

9ae9f84

daschuer reviewed Jun 5, 2015
View reviewed changes

Unify export of SoundSources from plugin libraries

0e916ff

Prefer local static variables for shared data in plugins

4f39074

rryan added a commit that referenced this pull request Jun 5, 2015

Merge pull request #578 from uklotzde/SoundSourcePluginUrlFileNameFix

c4532a0

SoundSource Plugin API: Version 8

rryan merged commit c4532a0 into mixxxdj:master Jun 5, 2015

uklotzde deleted the SoundSourcePluginUrlFileNameFix branch July 18, 2015 12:15

This was referenced Aug 22, 2022

Character '#' in file path is not handled correctly for SoundSource plugins #8014

Closed

Support decoding of MP4 DASH files #8078

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SoundSource Plugin API: Version 8 #578

SoundSource Plugin API: Version 8 #578

uklotzde commented May 7, 2015

uklotzde commented May 14, 2015

uklotzde commented May 18, 2015

daschuer May 20, 2015

uklotzde commented Jun 4, 2015

rryan Jun 4, 2015

uklotzde Jun 4, 2015

rryan Jun 4, 2015

rryan commented Jun 4, 2015

uklotzde commented Jun 4, 2015

uklotzde commented Jun 4, 2015

uklotzde commented Jun 5, 2015

daschuer Jun 5, 2015

daschuer Jun 5, 2015

uklotzde Jun 5, 2015

daschuer commented Jun 5, 2015

uklotzde commented Jun 5, 2015

daschuer commented Jun 5, 2015

rryan commented Jun 5, 2015

rryan commented Jun 5, 2015

		inline QString getLocalFileName() const {
		DEBUG_ASSERT(isLocalFile());

SoundSource Plugin API: Version 8 #578

SoundSource Plugin API: Version 8 #578

Conversation

uklotzde commented May 7, 2015

uklotzde commented May 14, 2015

uklotzde commented May 18, 2015

daschuer May 20, 2015

Choose a reason for hiding this comment

uklotzde commented Jun 4, 2015

rryan Jun 4, 2015

Choose a reason for hiding this comment

uklotzde Jun 4, 2015

Choose a reason for hiding this comment

rryan Jun 4, 2015

Choose a reason for hiding this comment

rryan commented Jun 4, 2015

uklotzde commented Jun 4, 2015

uklotzde commented Jun 4, 2015

uklotzde commented Jun 5, 2015

daschuer Jun 5, 2015

Choose a reason for hiding this comment

daschuer Jun 5, 2015

Choose a reason for hiding this comment

uklotzde Jun 5, 2015

Choose a reason for hiding this comment

daschuer commented Jun 5, 2015

uklotzde commented Jun 5, 2015

daschuer commented Jun 5, 2015

rryan commented Jun 5, 2015

rryan commented Jun 5, 2015