Review Request: EngineController: fixes to canDecode() and supportedMimeTypes(): make them non-static, thread-safe even on first call. (squached patches, recent on top)

Thu Jul 12 23:04:20 UTC 2012

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105524/#review15763
-----------------------------------------------------------

In principle ok, but please have a look at my comments.
The Semaphore stuff is a little complex and would need some more explaining comments in my opinion.

src/core-impl/collections/support/CollectionManager.cpp
<http://git.reviewboard.kde.org/r/105524/#comment12247>

    the call to canDecode had me baffled.
    Is MetaFile::Track only representing tracks that can be currently played?
    The canDecode call goes into a lot of code and the benefit of this condition is still doubtfull.
    Why are only tracks considered valid that I currently have support for?
    Would that mean that I might run into troubles when (for whatever reason) phonon temporarily decides that it can't play a certain mimetype?
    Would my tracks be removed from the playlist?

    Also the comment to the trackForUrl function does not explain this in enought details for me.

    I personally would remove the canDecode call here.

tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp
<http://git.reviewboard.kde.org/r/105524/#comment12248>

    If we need to have special code here to ensure a sensible cleanup, might we not run into troubles in Amarok itself?
    Could you add the old comment back?
    The "Wait for other jobs, ..."

tests/core/collections/support/TestTrackForUrlWorker.cpp
<http://git.reviewboard.kde.org/r/105524/#comment12249>

    What a sorry excuse for a singleton we have here.
    We should look into that after the release and check if we can't do it better.

tests/core/collections/support/TestTrackForUrlWorker.cpp
<http://git.reviewboard.kde.org/r/105524/#comment12250>

    And these two lines had me confused.
    Why do we explicitely need to register the types? Isn't the Amarok library doing it on it's own?

tests/playlistmanager/sql/TestSqlUserPlaylistProvider.cpp
<http://git.reviewboard.kde.org/r/105524/#comment12251>

    Is having the code in four places in the test cases really better than an additional check in CollectionManager::trackForUrl?

    And again, it's just because of the one function call in trackForUrl that implements a function that is not documented and could be surprising for the user in  some corner cases.

- Ralf Engels

On July 12, 2012, 2:15 p.m., Matěj Laitl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105524/
> -----------------------------------------------------------
> 
> (Updated July 12, 2012, 2:15 p.m.)
> 
> 
> Review request for Amarok, Ralf Engels and Sven Krohlas.
> 
> 
> Description
> -------
> 
> EngineController: use QFileInfo instead of apparently non-reentrant KFileItem
> 
> ...remote files are not needed here, so QFileInfo does a good job. This
> is the cause of last crashes in testm3uplaylist. (I have about 25%
> probability of it crashing until this commit)
> 
> EngineController: make supportedMimeTypes() thread-safe even on first call
> 
> If supportedMimeTypes() is called for the first time in a thread,
> Phonon::BackendCapabilities::availableMimeTypes() creates some QObject
> subclasses in non-main thread and that causes problems later on, and is
> the cause of testm3uplaylist failures. Add signal/slot/QSemaphore
> trickery that causes Phonon::BackendCapabilities::availableMimeTypes()
> is called from the main thread without performance penalty for 2nd and
> next calls.
> 
> Test is added for EngineController::supportedMimeTypes() so that this
> fragile code (hopefully) never breaks again.
> 
> This fixes (perhaps along with the next commit) 2 failing tests (
> testm3uplaylist and apparently testplaylistmodels), so the fail count
> is down to 4 here.
> 
> EngineController: make canDecode() and supportedMimeTypes() non-static
> 
> Static methods have no sense in a singleton class. Additionally, it was
> very hacky (and impossible in corner-cases) to keep these thread-safe,
> see testm3uplaylist failures. Making them non-static will allow us to
> do some tricks so that the calls are more robust.
> 
> Never-used and confusing destory() method is removed, too.
> 
> Care is taken not to break existing tests.
> v2: fix even more tests that have been added or fixed while this patch
>     slept.
> 
> Revert "Prevent hang in testmetamultitrack."
> 
> This reverts commit 115cb80f9bd94b23640ca9245c97d6c8e25d2c97.
> 
> Proper fix will follow, see next commits.
> 
> 
> Diffs
> -----
> 
>   src/EngineController.h ad2e0c4a5e7c80c79bf448bf74cd6b52cd1f0ed3 
>   src/EngineController.cpp 83f0a6ed0a92ae992e1809800cee65d9349dc680 
>   src/browsers/filebrowser/FileBrowser.cpp 567ff799df7ef8bfcd93de73ee120bfc5be634b7 
>   src/browsers/filebrowser/FileView.cpp 8eae4b6731ebfcb7310d3d719687989be125de92 
>   src/core-impl/collections/support/CollectionManager.cpp 9085b5a27d9a7ce94d2325d94bec5fce8d126abe 
>   tests/CMakeLists.txt 901c716a600bca63639939f4e62dbd89d9db707f 
>   tests/TestEngineController.h PRE-CREATION 
>   tests/TestEngineController.cpp PRE-CREATION 
>   tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp 4bbf29be01aef9043a64075a196ac04a544134cb 
>   tests/core/collections/support/TestTrackForUrlWorker.h PRE-CREATION 
>   tests/core/collections/support/TestTrackForUrlWorker.cpp PRE-CREATION 
>   tests/core/meta/TestMetaTrack.cpp 0adb7633538533202c50487b1eaff8e4ead150e9 
>   tests/dynamic/TestDynamicModel.h ce9934cf464172b75b1858f884a64c00cdcf4e24 
>   tests/dynamic/TestDynamicModel.cpp 7387fbcbf28ad838fbf56076553da56d630451af 
>   tests/playlistmanager/file/TestPlaylistFileProvider.cpp 931eefba03e55f4ae2beec089369087958311013 
>   tests/playlistmanager/sql/TestSqlUserPlaylistProvider.cpp bb09515d617e8149e7e66e804ff46aa5ddb76045 
> 
> Diff: http://git.reviewboard.kde.org/r/105524/diff/
> 
> 
> Testing
> -------
> 
> Works, fixes testm3uplaylist
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120712/3356ae2d/attachment-0001.html>