<div dir="ltr">Thanks a lot Matej for this in-depth review. I will address all the issues you pointed out and make another draft.<div style><br></div><div style>Reagards,</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Tue, Apr 30, 2013 at 2:11 AM, Matěj Laitl <span dir="ltr"><<a href="mailto:matej@laitl.cz" target="_blank">matej@laitl.cz</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On 29. 4. 2013 vedant agarwala wrote:<br>
> In the meantime, I have updated my project proposal as shown below. Your<br>
> inputs are is still as valuable so please comment :)<br>
<br>
</div>Hi, I'm very sorry to reply this late, I haven't unfortunately found time<br>
earlier, neither did any other developer apparently.<br>
<br>
Alberto, perhaps you'd be able to review this proposal (unless you want to<br>
submit a similar one) too? (as you've lately touched the code significantly)<br>
<div class="im"><br>
> Proposal Title: Improving and modularizing tag guessing<br>
><br>
> Motivation for Proposal / Goal: Currently, Amarok “guesses” tags of music<br>
> files through the MusicBrainz web service using either existing tags or<br>
> MusicIP (PUID) audio fingerprints. This method, however, is somewhat<br>
> outdated. MusicBrainz is phasing out MusicIP in favour of AcousID and other<br>
> methods of guessing tags have emerged. The aim of this project is to create<br>
> an abstract base class for tag guessing that can be suitably inherited by<br>
> other classes that aim to guess tags.<br>
><br>
> Getting tags from MusicBrainz is hard coded into the Amarok source code.<br>
> Hence tag guessing is not modularized; this makes it hard to add new Tag<br>
> Getters (like, for example, Last.fm). Moreover, MusicBrainz code is not<br>
> very well documented. So, it'll be very time consuming for a programmer<br>
> trying to add features even just to MusicBrainz since they will have to<br>
> read and understand the code. As of now, every track’s tags can only be<br>
> changed one by one that also after waiting a few seconds for each.<br>
<br>
</div>The last sentence is not true, you can change then all in batch using the Edit<br>
Tags dialog or throught existing MusicBrainz support. See also how Alberto<br>
Villa<br>
<div class="im"><br>
> More often than not, the tags returned by MusicBrainz is incorrect or tags<br>
> of different kinds are returned.<br>
<br>
</div>This might get better thanks to recent Alberto's patch. I view MusicBrainz as<br>
a good way for slight corrections to existing tags, but is often provides very<br>
bad results for files with no tags at all (which is expected).<br>
<div class="im"><br>
> Say when a user is listening to a particular song he/she realizes that some<br>
> tags to the song are missing. He/she will try to correct this by himself<br>
> and will be very happy to see a “Get tags from MusicBrainz” button. Rather<br>
> than updating the tags himself he/she clicks on the button and selects the<br>
> correct tags. But this is where it all ends. The user is only saved from<br>
> the trouble of typing. He/she still has to choose the correct tags. Many<br>
> times the user will not find any tags at all or they will be guessed wrong<br>
> entirely.<br>
<br>
</div>I don't think that this project, even is successful, will make the situation<br>
above significantly better.<br>
<div class="im"><br>
> Moreover, there is no way of updating/correcting tags of multiple files.<br>
<br>
</div>This is entirely false.<br>
<div class="im"><br>
> If<br>
> a user adds some new songs to the local collection with poorly formatted<br>
> tags. It is not uncommon for tracks to contain a track name in the format<br>
> “trackname - albumname - artistname” with the other details blank. Many<br>
> tracks contain names of websites (like downloaded from) at the end of one<br>
> or more tags of the file. A user will want these tags to be fixed but it<br>
> will be a real pain to do this one by one. If he/she clicks on the “guess<br>
> tags from file” he/she is greeted by another dialog that takes a long while<br>
> to understand. Its likely that he/she will just close this dialog. Even if<br>
> he/she gets tags from MusicBrainz (assuming the fetched tags are correct),<br>
> getting them from each file one by one will be too much of a task for him.<br>
<br>
</div>This paragraph is very misleading - such problem doesn't exist in current<br>
Amarok.<br>
<br>
> Implementation Details:<br>
<div class="im">> -<br>
> Creating a generic framework for tag getters: I will replace the<br>
> existing musicbrainz directory by a taggetter directory. It will contain<br>
> Controller and Provider classes (similar to StatSyncing) under the<br>
> namespace TagGetter. The settings UI and related classes will also be<br>
> present in this directory. I will change the musicbrainzTagger() in the<br>
> TagDialog class to tagGetterController(). Once this is called, the<br>
> TagGetter::Controller (a singleton class) will be invoked and process of<br>
> tag guessing will begin. The Controller will have a list (a QList of<br>
> ProviderPointers) of available Providers and it will create objects of<br>
> each of them. Each of those objects will implementing the abstract base<br>
> class Provider. As required, the Controller will be calling the methods<br>
> (and connecting signals to slots) of the Provider on the main thread and<br>
> via polymorphism appropriate methods of each Provider will be called.<br>
<br>
</div>Right.<br>
<div class="im"><br>
> Each TagGetter::Provider will have its own directory under the TagGetter<br>
> directory. They will contain classes and one that will implement the<br>
> Provider class. Data will be shared among the classes (in the TagGetter<br>
> namespace) probably via QExplicitlySharedDataPointer.<br>
<br>
</div>what data?<br>
<div class="im"><br>
> Each provider will<br>
> be running in parallel. As the different Providers do their work, results<br>
> and progress of the work will available to the Controller via the signals<br>
> connected to the slots of the Controller. Establishing a connection to the<br>
> web service server, converting the music into data sendable over the<br>
> internet, fetching results and the error handling will all be part of each<br>
> Provider’s work. Separate threads won’t be required because the network<br>
> operations will be handled by Qt (QNetworkRequest and QNetworkReply) and<br>
> they are the most time consuming. If something else takes time to run (like<br>
> decoding the audio and creating the fingerprint) then the provider will<br>
> have to manage this on another thread (probably by using the<br>
> ThreadWeaver::Job). As part of the contract of each provider, each of its<br>
> methods must return quickly. This is important because the Controller will<br>
> guarantee calling these methods on the main thread.<br>
<br>
</div>Right.<br>
<div class="im"><br>
> The Controller will provide the Providers with track details (through<br>
> the track pointer)such as track data (name, album, artist, length, file<br>
> location etc.). The framework will contain a TagGetterMeta class. Objects<br>
> of this class will store a TrackPtr to identify the the track in the<br>
> Collection and metadata (name, artist, album, etc.) returned by the<br>
> providers. Providers will return a QList of objects of this class. The<br>
> metadata that has been updated will be non-zero.<br>
<br>
</div>Oh, I'm getting a bit lost in this, but I hope I get the point, which seems<br>
okay.<br>
<div class="im"><br>
> Providers should not store<br>
> this data but they should keep the data that authenticates the provider<br>
> (probably an API key that is received on authentication) as long as the<br>
> objects of themselves exist since many lists of tracks can sent to be the<br>
> Provider in succession, either by the user or programmatically (refer to<br>
> last point of “implementation details”).<br>
<br>
> A AbstractSettings class will also<br>
> be present so that users can customize the settings of each Provider and<br>
> each Provider can store a small amount of data in the data using the<br>
> KGlobal::config(). This will also be beneficial for the programmer(s) who<br>
> add more Providers. It will also keep a consistent look and feel of all the<br>
> TagGetterProviders.<br>
<br>
</div>Hmmm, the original intent was to make the providers rather dumb and almost<br>
invisible to the user. What do you think would be need to be configurable by<br>
the user?<br>
<div class="im"><br>
> Rewriting MusicBrainz tag getter: I will rewrite MusicBrainz code<br>
> according to the above framework. All the existing classes will shifted<br>
> to the new musicbrainz subdirectory inside the taggetter directory. The<br>
> MusicBrainzFinder will become MusicBrainzProvider, implementing the<br>
> abstract Provider class.<br>
<br>
</div>Right, but I wouldn't say "rewrite", but rather "adapt to be an implementation<br>
of the new abstract class".<br>
<div class="im"><br>
> Other code will also be re-written but its work<br>
> will mainly remain the same, except for one major difference. Currently,<br>
> libofa is used to create the MusicIP (PUID) that is sent to MusicBrainz.<br>
> MusicBrainz is phasing out PUIDs in favour of AcustIDs.<br>
><br>
> Hence, the MusicBrainzProvide will use<br>
</div>> Chromaprint<<a href="http://acoustid.org/chromaprint" target="_blank">http://acoustid.org/chromaprint</a>>to compute AcustIDs rather<br>
<div class="im">> than the MusicIP generated by libofa.<br>
<br>
</div>Oh beware! The musizbrainz and musicip are rather independent and were<br>
intended to be factored out into 2 separate providers! (one changing to<br>
acoustid)<br>
<div class="im"><br>
> Codecs will identified using ffmpeg.<br>
<br>
</div>What/why?<br>
<div class="im"><br>
> Phonon will be used for decoding while Chromaprint will be used to generate<br>
> the AcoustID.<br>
<br>
</div>Have you checked this is possible? What Phonon methods will you use?<br>
<div class="im"><br>
> Chromaprint uses the<br>
> standard C<br>
</div>> library[*]<<a href="https://bitbucket.org/acoustid/chromaprint/src/master/src/chroma" target="_blank">https://bitbucket.org/acoustid/chromaprint/src/master/src/chroma</a><br>
> print.h>so using this won’t be a problem.<br>
<br>
Eh? What is a connection between a project using a standard C library and<br>
feasibility of its usage?<br>
<div class="im"><br>
> I will make Chromaprint a part of<br>
> the optional tag guessing package (replacing libofa) and update the<br>
> cmakelists, making Chromaprint as a requirement for MusicBrainz tag<br>
> guessing. Hence, the MusicBrainzProvider will be available only if<br>
> Chromaprint has been installed. A HAS_CHROMAPRINT macro will keep track of<br>
> this. The provider will be “available” only if the macro is defined. Just<br>
> as now, the “The::networkAccessManager()”<br>
> will handle the connection to the MusicBrainz server. The audio<br>
> fingerprint generated by Chromaprint will be sent to MusicBrainz via a<br>
> QNetoworkRequest and a slot will be connected to listen for its reply.<br>
<br>
</div>The above is obvious and it is perhaps redundant to mention it.<br>
<div class="im"><br>
> Creating Last.fm tag getter: Create the Last.fm tag getter. First, I<br>
> will add the tag based service and then, if schedule permits, I can add<br>
> the fingerprint based service. Hence, two LastFm Providers will be created.<br>
> Implementation will be very easy thanks to<br>
</div>> liblastfm<<a href="https://github.com/lastfm/liblastfm" target="_blank">https://github.com/lastfm/liblastfm</a>>-> A Qt C++ library for the Last.fm webservices. Both the Providers will<br>
<div class="im">> share the authentication data and reply so that network requests aren’t<br>
> needlessly duplicated. They will be in the same directory but will have two<br>
> available providers. The network replies by Last.fm is in XML format.<br>
> Hence, the QtXML module will parse the replies accordingly.<br>
<br>
</div>In fact, liblastfm does it. (and we already use it in Amarok)<br>
<div class="im"><br>
> Now the<br>
> similarities end. The network requests and replies will have to handled<br>
> differently by the different providers, since one will be tag based and the<br>
> other fingerprint based.<br>
><br>
> For the LastfmTagProvider the track/artist name has to sent to the<br>
> Last.fm webservice API. The webservice will then return the other track<br>
> metadata. After being correctly parsed it will signal the required slot<br>
> so that the Controller receives the tracks whose metadata has been fetched.<br>
> For the LastFmFingerprintProvider we will first have to generate a<br>
> “fingerprint” of the track using the Last Fm<br>
</div>> Fingerprinter<<a href="https://github.com/lastfm/Fingerprinter" target="_blank">https://github.com/lastfm/Fingerprinter</a>>.<br>
<div class="im">> This audio fingerprint will then be sent to the Last fm web service via a<br>
> call to the<br>
</div>> track.getFingerprintMetadata<<a href="http://www.last.fm/api/show/track.getFingerpri" target="_blank">http://www.last.fm/api/show/track.getFingerpri</a><br>
> ntMetadata>. Then the web service will return the track metadata along with<br>
<div class="im">> a “confidence rating” to specify how accurately the track has been<br>
> identified. All this data will be parsed by the QtXML module and sent to<br>
> the Controller in a manner similar to above.<br>
<br>
</div>^^ the above could be shortened, many things mentioned are rather obvious.<br>
<div class="im"><br>
> A better GUI: A better GUI is required. Though I will need to discuss<br>
> with the Amarok developers what the new GUI should like so that it<br>
> follows the general look and feel of Amarok, I am proving my<br>
> interpretation as follows: I will replace the existing "Get tags from<br>
> MusicBrainz" button by "Get tags from the following services: ". It will be<br>
> followed by a drop-down list with check-boxes for each item containing<br>
> names of the services.<br>
><br>
> [image: <a href="http://troll.ws/image/458a61bc#.UX0fS0GH7gx" target="_blank">http://troll.ws/image/458a61bc#.UX0fS0GH7gx</a>]<br>
<br>
</div>I don't like it. We should just query all enabled services, users don't want<br>
to think about it every time. If some service doesn't work well for them, they<br>
should be able to disable it in Plugins section.<br>
<div class="im"><br>
> In “Amarok Settings” there will be a tag guessing section and every<br>
> TagGetterProvider can create a tab to change settings of the individual<br>
> provider.<br>
<br>
</div>I don't like this either. Every setting just bothers the user, the providers<br>
should just work without any configuration.<br>
<div class="im"><br>
> As getting tags from the individual services will run in<br>
> parallel, results will be displayed as soon as they are fetched via the<br>
> TagGetter::Controller. This list will persist in memory so that if the<br>
> user closes the dialog and then reopens it, tags won’t have to be fetched<br>
> again (until Amarok restart).<br>
<br>
</div>Oh please not, this is very convoluted and may lead to very unexpected memory<br>
usage.<br>
<div class="im"><br>
> The user can choose the best tags from the<br>
> generated list. If tags from multiple sources are the same they should be<br>
> clubbed and appear higher up the list compared to the others.<br>
<br>
</div>clubbed: yes; higher in the lists: yes, but I propose using the max(a, b)<br>
rather than a + b function to combine the scores.<br>
<div class="im"><br>
> Tags from<br>
> different sources have to cross-checking for this similarity.<br>
<br>
</div>I don't understand this sentence.<br>
<div class="im"><br>
> Each set of<br>
> tags that have been fetched will be accompanied by a “confidence rating”-<br>
> between 0 and 1, and provided by the respective Provider. For similar tags<br>
> the ratings will be added (or maybe similar way of increasing the rating<br>
> that can be found out by testing). The list will be continually sorted<br>
> according to the rating of each tag. The list will be a bullet list with<br>
> colors depicting the “confidence rating”- 0 to 1 = red to green (as it is<br>
> now).<br>
<br>
</div>Well, this is largely what the current music brainz dialog is, isn't it?<br>
<div class="im"><br>
> All user visible strings (buttons and tool-tips) will be written<br>
> using i18n(). i18nc() will be used wherever requiered- like, for a label<br>
> stating "number of track(s)".<br>
<br>
</div>This is an obvious requirement, no need to mention it.<br>
<div class="im"><br>
> A GUI for getting tags of multiple songs simultaneously: Its a feature<br>
> by which tags for multiple tracks (if not the entire local collection)<br>
> can be changed. Getting tags requires many user clicks as of now,<br>
> especially for multiple tracks. To solve this, a "Tag Getter Wizard" will<br>
> select the best tags for every track.<br>
<br>
</div>This is already working, please try to understand *actual* Amarok features<br>
more!<br>
<div class="im"><br>
> Best ones are written to files<br>
> without user approval but in a reversible way. Tags will be saved to<br>
> database and can be reversed/changed anytime (even after amarok restart) in<br>
> both ways- 1) for individual tracks 2) through a list showing the changed<br>
> tags.<br>
<br>
</div>Oh, please don't. This would be really convoluted to implement and use.<br>
<div class="im"><br>
> A user can start the wizard select the playlists and/or tacks whose tags<br>
> need to be guesses, then make it run in the background or make it run in<br>
> the foreground showing the details of each track whose tags are being<br>
> changed (and missing ones added).<br>
<br>
</div>Hmm, no, please don't make this part of the project (+ it is already working<br>
to some extent).<br>
<div class="im"><br>
> Or, he/she can set it to run on the<br>
> entire local collection and/or as new tracks are added to the local<br>
> collection.<br>
<br>
</div>This doesn't look like a good idea to me.<br>
<div class="im"><br>
> I will make decoupled and modularized code so that support<br>
> for this wizard can easily be extended to other collections.<br>
> After the<br>
> wizard has completed its operation a list will show the user the updated<br>
> tags each a button that will reverse the the changes. Not all fetched<br>
> tags will be updated. Though I can achieve the right balance only by user<br>
> testing, as of now I set the default as "tags with confidence rating >1<br>
> will be updated". This can easily be changed by the user in the settings<br>
> page. Again, I will write every code according to the KDE<br>
> internationalization guildlines. The exact look of this wizard will be<br>
> decided after discussion with the Amarok team but a rough idea of<br>
> features is depicted below:[image:<br>
> <a href="http://troll.ws/image/438a77c9#.UX0fXEGH7gx" target="_blank">http://troll.ws/image/438a77c9#.UX0fXEGH7gx</a>]<br>
<br>
</div>No, please don't do this. I'm already pretty fine this is current "MusicBrainz<br>
Tags Dialog" - especially when recent Alberto's patch is applied.<br>
<div><div class="h5"><br>
> <--- GSoC commences---><br>
><br>
> week 3: Make the directories and write the abstract classes (with<br>
> documentation).<br>
><br>
> week 4: Polish, make cmakelists for, write make tests and compile the<br>
> written abstract classes.<br>
><br>
> July-<br>
><br>
> week 1: Re-write the MusicBrainz tag getter according to the framework with<br>
> better documentation<br>
><br>
> week 2: Add the features (like using AcoustID) to this MusicBrainz tag<br>
> getter<br>
><br>
> week 3: Compile and run the new MusicBrainz (displaying results in the old<br>
> UI itself)<br>
><br>
> week 4: Write the new GUI code. Run MusicBrainz tag getter in this fashion<br>
><br>
> <--- Mid term ---><br>
><br>
> August-<br>
><br>
> week 1: Write the Last.fm tag getter (as many features as possible)<br>
><br>
> week 2: Finish writing and test the Last.fm tag getter<br>
><br>
> week 3: Test the Last.fm and MusicBrainz tag getter together in the new GUI<br>
><br>
> week 4: Make the tag getter wizard<br>
><br>
> September-<br>
><br>
> week 1: Test the new wizard. Write make tests to make sure that new tag<br>
> getters follow the set framework<br>
><br>
> week 2: Improve documentation. Fix bugs that have been discovered over the<br>
> weeks<br>
><br>
> <--- suggested “pens down” ---><br>
><br>
> week 3: Fix Bugs, streamline and optimize the code.<br>
><br>
> <--- firm “pens down” ---><br>
><br>
> week 4: Do some code cleaning and fix bugs so that my project can be pushed<br>
> into the master branch.<br>
><br>
> October-<br>
><br>
> week 1: Improve documentation. Fix bugs.<br>
<br>
</div></div>Vedant, I really think that implementing all the stuff you've outlined is<br>
unachievable, at least not in a code quality we'd expect. Please take some<br>
time to significantly reduce the amount of work you plan to do. Many of the<br>
ideas you have on top of the idea (published by devs) simply don't align with<br>
Amarok goals and/or are based on misunderstanding of how current tag guessing<br>
works.<br>
<div class="im"><br>
> Other Obligations: I have no other obligations. I can easily spent about 50<br>
> hours a week: around 8 hrs a day in slots of 2 to 3 hrs, one each in the<br>
> afternoon (10am-1pm), evening (4pm-6pm) and at night (10pm-1am), with some<br>
> hours less on weekends.<br>
<br>
</div>Hehe, I don't think you need to be this detailed. :-)<br>
<div class="im"><br>
> Summer vacations will be going on till mid July.<br>
> Even after college starts, I will be following the same schedule for GSoC<br>
> coding. It is easily possible because very few classes are held in the<br>
> beginning of the semester and even if they do clash with my coding<br>
> time-period, the college teachers excuse GSoC students' absence from class.<br>
> Since I will have no other coursework obligations, I can continue to code<br>
> 50 hours a week (with a similar schedule) even up till September.<br>
><br>
> About Me: I am currently in my second undergraduate year in National<br>
> Institute of Technology, Durgapur, India, studying Computer Science and<br>
> Engineering. I have experience coding experience with C/C++, Java<br>
> (including Android and making GUI using Java swing) and web services. I<br>
> have submitted 3 patches (Junior Jobs, one each for<br>
</div>> Rekonq[1]<<a href="https://git.reviewboard.kde.org/r/107662/" target="_blank">https://git.reviewboard.kde.org/r/107662/</a>>and Amarok<br>
> [2] <<a href="https://git.reviewboard.kde.org/r/109283/" target="_blank">https://git.reviewboard.kde.org/r/109283/</a>> as well as a improved<br>
> formatting patch for Amarok[3] <<a href="https://git.reviewboard.kde.org/r/109295/" target="_blank">https://git.reviewboard.kde.org/r/109295/</a>>)<br>
<div class="im">> and 2 more are under review (both Junior Jobs for<br>
</div>> Amarok[4]<<a href="https://git.reviewboard.kde.org/r/110101/" target="_blank">https://git.reviewboard.kde.org/r/110101/</a>><br>
> [5] <<a href="https://git.reviewboard.kde.org/r/110082/" target="_blank">https://git.reviewboard.kde.org/r/110082/</a>>).<br>
<div class="im">><br>
> I love coding for open source. I’m sure working with a mentor who is<br>
> virtually present won’t be a problem. I have interacted (mainly with Matej<br>
> a.k.a. Strohel on IRC) over IRC, the amarok mailing list, reviewboard and<br>
> also the KDE bug tracking system. During the work period code can easily be<br>
> shared via GitHub.<br>
<br>
</div>We prefer personal scratch git repositories on KDE infrastructure for this.<br>
<div class="im"><br>
> If it is possible, meeting a mentor face to face will<br>
> obviously be much more helpful. I have worked with my college seniors in<br>
> this fashion who have introduced me to Linux and Open-Source. Countless<br>
> times I have been to their rooms for advice and to solve my problems.<br>
><br>
> After GSoC I plan to become an active developer for Amarok and also for<br>
> other projects of KDE.<br>
<br>
</div>As stated above, this looks to me like a way too much work to do. Perhaps you<br>
are trying to make the proposal more interesting, but I fear that the effect is<br>
the opposite - we'd actually appreciate if you wanted to implement much less<br>
things (ideally just what the initial idea was about), but make it interesting<br>
by level of insight, research and thought that has been put to it.<br>
<br>
Also please study/use the current tag guessing in Amarok more (and do have a<br>
look at Alberto's patch) to avoid a situation where you describe lack of<br>
Amarok feature while it is not the case.<br>
<br>
Regards,<br>
<div class="HOEnZb"><div class="h5"> Matěj<br>
_______________________________________________<br>
Amarok-devel mailing list<br>
<a href="mailto:Amarok-devel@kde.org">Amarok-devel@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/amarok-devel" target="_blank">https://mail.kde.org/mailman/listinfo/amarok-devel</a><br>
</div></div></blockquote></div><br></div>