Review Request: Changes in processing playlist files

[Bart Cerneels]

> On Nov. 27, 2012, 8:08 a.m., Bart Cerneels wrote:
> > src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp, line 218
> > <http://git.reviewboard.kde.org/r/107473/diff/1/?file=96233#file96233line218>
> >
> >     Here is the real reason why lazy loading is needed. Reading playlist-file contents from disk is too fast to bother, but getting the right Track objects from CollectionManager::trackForUrl() is problematic. It should either block until the track is ready (depends in TrackProviders) or using proxy loading like it is now. The proxy is temporary and gets initialized with metadata we know from the contents of this playlist file.
> 
> Myriam Schweingruber wrote:
>     That is a totally wrong assumption, don't you remember that we introduced it because reading playlists on start-up is a PITA? Please don't assume that all users have their collection and playlists on an SSD.
>     Playlists should never be read unless you actually load it in the play queue, not a second earlier. We reduced start up time by over a minute, and some users reported start up delays of several minutes because playlist were read on start. See also https://bugs.kde.org/show_bug.cgi?id=245654, you introduced that yourself :)
> 
> Bart Cerneels wrote:
>     The delay is coming from loading the tracks from NFS, not the text content of the playlist file. I guess I needed to clarify that lazy loading of tracks using ProxyTrack is what we currently have in SQL and XSPF playlists and is what all playlist implementations need to do.
> 
> Matěj Laitl wrote:
>     > The delay is coming from loading the tracks from NFS, not the text content of the playlist file. I guess I needed to clarify that lazy loading of tracks using ProxyTrack is what we currently have in SQL and XSPF playlists and is what all playlist implementations need to do.
>     
>     No. We need *both*: lazy-loading of tracks (MetaProxy::Track), and lazy-loading of playlists themselves. Bart, please mount an USB 1.1 stick with 1000 playlists, then speak.

Why not create the playlists (i.e. constructor and/or calling PlaylistFile::load()) in batches so it won't block the UI thread? I don't see how delayed loading of playlist contents (as opposed to loading it's tracks) will solve the scaling problem.
We need the full playlist contents before anything can be done with them (source of title, #tracks estimate, etc) hence, this can't be deferred.

At one point this was implemented and working. It's possible playlist syncing is causing a premature load anyway. If so it's a regression.


- Bart


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


On Dec. 29, 2012, 12:46 p.m., Tatjana Gornak wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107473/
> -----------------------------------------------------------
> 
> (Updated Dec. 29, 2012, 12:46 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> I've started my changes with an implementation of a ASXPlaylist class,
> due this work I've found that implementation of different playlist file
> types has different logic, as result I end up with a rewriting
> playlist's implementations.
> 
> As example of difference:
>   -- Constructor M3UPlaylist::M3UPlaylist( const KUrl &url ) sets a url, but
>      does not load playlist, therefore loading of playlist are
> posponed till data from playlist are actualy needed (lazy loading)
>      On the other hand constructor XSPFPlaylist::XSPFPlaylist( const
> KUrl &url, bool autoAppend )  loads playlist.
> 
> The main idea of proposed changes is to create a unify code for
> processing playlist files:
>   -- lazy loading through LazyLoadPlaylist
>   -- common functionality was moved to PlaylistFile.
> 
> It is worth to be noticed that this patch changes structure of
> playlist's classes, bit does not changes their behavior, even if this behavior
> is inconsistent in some cases.
> 
> In following patch-requests I plan to submit ASX Playlist parser and
> organize playlists processing in more consistent way.
> 
> 
> Diffs
> -----
> 
>   src/core-impl/playlists/types/file/PlaylistFile.h 4322da9 
>   src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 
>   src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 2dcc0cd 
>   src/core-impl/playlists/types/file/m3u/M3UPlaylist.h 3176c45 
>   src/core-impl/playlists/types/file/m3u/M3UPlaylist.cpp 2395260 
>   src/core-impl/playlists/types/file/pls/PLSPlaylist.h e974539 
>   src/core-impl/playlists/types/file/pls/PLSPlaylist.cpp 3ba154a 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h ebf3235 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 
>   src/core/playlists/Playlist.h 8fd1ffb 
>   src/playlistmanager/PlaylistManager.h cbf65b0 
>   src/playlistmanager/PlaylistManager.cpp 89c754b 
>   src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 
>   tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 
>   tests/core-impl/playlists/types/file/TestPlaylistObserver.h PRE-CREATION 
>   tests/core-impl/playlists/types/file/TestPlaylistObserver.cpp PRE-CREATION 
>   tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp 0f19e1b 
>   tests/core-impl/playlists/types/file/xspf/TestXSPFPlaylist.cpp f4643ca 
> 
> Diff: http://git.reviewboard.kde.org/r/107473/diff/
> 
> 
> Testing
> -------
> 
> 1) All unit-tests were passed.
> 2) For all playlists I've also checked loading and
>    saving through gui.
> 
> 
> Thanks,
> 
> Tatjana Gornak
> 
>

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