Acoustic Fingerprinting and QueryMaker (Bart and Max: read this)

[Maximilian Kossick]

see comments below

On Fri, Jul 10, 2009 at 2:18 PM, Soren Harward<stharward at gmail.com> wrote:
> Bart and Max raised issues with the way that the acoustic
> fingerprinting patch affects the QueryMaker, which needs to be figured
> out before I commit the patch.  I'm moving the discussion here because
> I find it easier to handle discussions on the mailing list than on
> Fisheye.  So if you don't care about the acoustic fingerprinting
> patch, feel free to ignore this thread.
>
> The acoustic fingerprinting system needs to provide two "use cases"
> for programmers:
>
> 1: Given two tracks, determine how similar one is to another.
> 2: Given a track, find other tracks in the collection that are similar to it.
>
> Use case #1 is currently handled in the Fingerprint class, called by
> way of FingerprintCapability, and implemented only in SqlMeta because
> local database collections are the only collections capable of storing
> fingerprints (as of right now; if Nepomuk collections ever become a
> reality, I'll add the functionality there).  Other than some
> sloppiness with the database schema which has been fixed, nobody seems
> to have an issue with how I have implemented this.

Yes, I have no issues with FingerprintCapability.

> Use case #2 is currently handled by QueryMaker.  More precisely,
> there's a "do nothing" function stub in QueryMaker, and only
> SqlQueryMaker overrides this stub to implement it.  Bart and Max,
> contrary to what both of you stated in your comments, adding a
> function stub to the QueryMaker base class neither adds a dependency
> on Fingerprint.h to the base class and all its children, nor does it
> require anyone who subclasses QueryMaker to implement the
> addSimilarityFilter function.

Which is not what I said. To quote myself: "Adding this method to
QueryMaker adds a dependency on Fingerprint.h to the querymaker in any
collection that wants to support it ... which is a sure sign that this
is wrong from an architectural point of view."

> I have designed the modifications to be as low-impact as I possibly
> could, and I don't know how they could have less impact without
> removing the functionality altogether.  It doesn't add a significant
> burden on anyone who wants to write a new QueryMaker for Amarok,
> because either the collection does not support fingerprinting, in
> which case you just ignore the function and let the base class do its
> job, or the collection does support fingerprinting, in which case
> you're going to have to write fingerprint-handling code anyway.

Wrong. The point is that it *adds* burden to anybody who wants to
write a new QueryMaker. If you look at the code in SqlQueryMaker, all
it does is reading a number of tracks and discards any that are not
similar. You would have to reimplement that functionality in any
QueryMaker that wants to support fingerprints. Moving this into a
cross-collection component in libamarok requires you to implement this
only once. Oh, and it allows you to compare the similarity of tracks
from *different* collections, e.g. a track from a SqlCollection and
one from a NepomukCollection.

> Removing the filter from QueryMaker altogether would prove more
> complicated that it's worth because then any code that wanted to find
> similar tracks would then have to check whether the collection
> supports fingerprinting.  So you'd basically end up with a
> Capabilities interface for collections, which I think would be far
> more invasive than the QueryMaker change.

The whole point of Capabilities is that they are not invasive.

> The alternative would be to add something like a "tracksSimilarToMe"
> function to FingerprintCapability.  This makes less sense to me,
> though.  "Find tracks like this one" is a QUERY of the collection, so
> conceptually it belongs with the rest of the collection queries (ie,
> in QueryMaker).  This approach would also make the Fingerprint class
> vastly more complicated, plus slow down the queries significantly.

"Find tracks like this one" is a use case. This does not mean that is
has to be implemented in QueryMaker. And I have no idea why you think
that this would make the Fingerprint class more complicated.

Cheers, Max