GNOME Bugzilla – Bug 732879
AcoustID source
Last modified: 2016-05-30 13:01:23 UTC
Cool project: http://acoustid.org/ This source provide a fingerprint by reading all the music-file and can resolve the mb-track-id [0] from this fingerprint. This source could provide more metadata from the webservice such as more MusicBrainz IDs like RecordingsIDs and AlbumsIDs; [1] The webservice also has a 'submit' operation that we could use as 'store' in grilo; [0] https://bugzilla.gnome.org/show_bug.cgi?id=732878 [1] http://acoustid.org/webservice
Created attachment 280104 [details] [review] acoustid: include the source
Created attachment 280105 [details] [review] acoustid: include simple tests Not mocked, with a real api-key for testing. The same api-key is going to the grilo-test-ui in the next patch.
Created attachment 280106 [details] [review] grilo-test-ui: include acoustid api-key
Review of attachment 280104 [details] [review]: ::: configure.ac @@ +147,3 @@ +PKG_CHECK_MODULES(CHROMAPRINT, libchromaprint, HAVE_CHROMAPRINTL=yes, HAVE_CHROMAPRINT=no) + +PKG_CHECK_MODULES(AVUTIL, libavutil, HAVE_AVUTIL=yes, HAVE_AVUTIL=no) Using libav/ffmpeg is out of the question in a grilo plugin, especially one shipped in core. ::: src/acoustid/grl-acoustid.c @@ +60,3 @@ +#define SOURCE_ID "grl-acoustid" +#define SOURCE_NAME "AcoustID" +#define SOURCE_DESC _("A plugin to calculate the fingerprint of musics</description") </description ? @@ +221,3 @@ +/* ======================= Utilities ==================== */ +static gchar * +get_path_to_file (const gchar *url) g_file_get_path() (but GStreamer has GIO support already)
Review of attachment 280105 [details] [review]: Looks fine
Review of attachment 280106 [details] [review]: Is this something we want to do? We should probably start looking at a way to enable/disable specific plugins in grilo-test-ui
Removing ffmpeg dependency. Using gstreamer now. GStreamer is way faster and less complicated then ffmepg libraries. GStreamer already has a chromaprint plugin[0] at gst-plugins-bad. I'm using it. [0] https://oxygene.sk/2011/01/chromaprint-plug-in-for-gstreamer/ (In reply to comment #4) > +#define SOURCE_ID "grl-acoustid" > +#define SOURCE_NAME "AcoustID" > +#define SOURCE_DESC _("A plugin to calculate the fingerprint of > musics</description") > > </description ? Fixed, thanks! > > @@ +221,3 @@ > +/* ======================= Utilities ==================== */ > +static gchar * > +get_path_to_file (const gchar *url) > > g_file_get_path() > (but GStreamer has GIO support already) I'm using GFile but I still try to resolve the url as a path and as an uri.
(In reply to comment #6) > Review of attachment 280106 [details] [review]: > > Is this something we want to do? With the acoustid we have the mb-track-id. It'll be easier to test/debug sources that needs musicbrainz IDs. > We should probably start looking at a way to enable/disable specific plugins in > grilo-test-ui With this [0] bug in my machine, I use the --grl-plugin-use option with the grilo-test-ui [0] https://bugzilla.gnome.org/show_bug.cgi?id=732259
Created attachment 280684 [details] [review] acoustid: include the source
Created attachment 280685 [details] [review] acoustid: include simple tests (same)
Review of attachment 280684 [details] [review]: Would be good to do the work for moving the chromaprint plugin to -good. Could you try and get a GStreamer developer to look over the GStreamer portion of the code? ::: src/acoustid/grl-acoustid.c @@ +44,3 @@ +/* --- URLs --- */ + +#define ACOUSTID_BASE "http://api.acoustid.org/v2/" Is there an https endpoint? @@ +48,3 @@ + "lookup?client=%s&duration=%d&fingerprint=%s" + +#define ACOUSTID_AUDIO_MAX_LENGTH 120 DURATION instead of length, and mention the unit in a comment after the value @@ +49,3 @@ + +#define ACOUSTID_AUDIO_MAX_LENGTH 120 +#define NANOSEC_TO_SEC(nsec) (nsec / G_GINT64_CONSTANT (1000000000)) GST_TIME_AS_SECONDS() I think @@ +125,3 @@ + spec = g_param_spec_string ("acoustid-fingerprint", + "acoustid-fingerprint", + "Fingerprint provided by Chromaprint library.", There's no need to mention the name of the library here. @@ +219,3 @@ +/* ======================= Utilities ==================== */ +static gchar * +get_path_to_file (const gchar *url) I still don't understand why you need a path here. You would use g_file_new_for_commandline_arg() anyway. @@ +311,3 @@ + +set_data_empty: + json_reader_end_member (reader); why do we care about that if we're going to unref it? @@ +333,3 @@ + res, &data, &len, &err); + if (err != NULL) { + GRL_WARNING ("Resolve operation failed due '%s'", err->message); due to @@ +335,3 @@ + GRL_WARNING ("Resolve operation failed due '%s'", err->message); + g_error_free (err); + goto lookup_done_end; Remove the goto, and do the rest in the other branch of the conditional. @@ +485,3 @@ + pipeline = gst_pipeline_new ("pipeline"); + + audiosrc = gst_element_factory_make ("filesrc", "audio-source"); giosrc @@ +486,3 @@ + + audiosrc = gst_element_factory_make ("filesrc", "audio-source"); + g_object_set (G_OBJECT (audiosrc), "location", audio_path, NULL); and pass it the GFile in the "file" property. @@ +489,3 @@ + g_free (audio_path); + + decoder = gst_element_factory_make ("decodebin", "decoder"); I would have used playbin to do the playback, set a "chromaprint + fakesink" bin as the audio sink, and disabled video decoding. Which might remove the need for the newpad_cb() function.
Review of attachment 280685 [details] [review]: Looks fine.
I talked to thiagoss about this code and about chromaprint plugin. * The chromaprint plugin seems in good shape but need better documentation and more tests in order to reach -good; * If we are loading non local files he suggested to use the uridecodebin instead of file ! decodebin; Or * It may be better to use playbin as it also accept uri and using playbin would be better to maintain in the long-term. -> If using playbin we could get the fingerprint as a tag on GST_MESSAGE_TAG instead of g_object_get() on GST_MESSAGE_EOS... but both methods are ok. One question that I didn't know how to answer: How should we handle if the music file has multiple streams? The example from chromaprint using libav only decode one stream to generate the fingerprint; and so does the chromaprint plugin on gstreamer.
(In reply to comment #11) > ::: src/acoustid/grl-acoustid.c > @@ +44,3 @@ > +/* --- URLs --- */ > + > +#define ACOUSTID_BASE "http://api.acoustid.org/v2/" > > Is there an https endpoint? I switched to https and it is working here. But there isn't anything on the documentation. > > @@ +48,3 @@ > + "lookup?client=%s&duration=%d&fingerprint=%s" > + > +#define ACOUSTID_AUDIO_MAX_LENGTH 120 > > DURATION instead of length, and mention the unit in a comment after the value Removed. > @@ +49,3 @@ > + > +#define ACOUSTID_AUDIO_MAX_LENGTH 120 > +#define NANOSEC_TO_SEC(nsec) (nsec / G_GINT64_CONSTANT (1000000000)) > > GST_TIME_AS_SECONDS() I think Awesome, thanks! > @@ +125,3 @@ > + spec = g_param_spec_string ("acoustid-fingerprint", > + "acoustid-fingerprint", > + "Fingerprint provided by Chromaprint library.", > > There's no need to mention the name of the library here. Fixed. > @@ +219,3 @@ > +/* ======================= Utilities ==================== */ > +static gchar * > +get_path_to_file (const gchar *url) > > I still don't understand why you need a path here. > > You would use g_file_new_for_commandline_arg() anyway. True.. I didn't know this one. > @@ +335,3 @@ > + GRL_WARNING ("Resolve operation failed due '%s'", err->message); > + g_error_free (err); > + goto lookup_done_end; > > Remove the goto, and do the rest in the other branch of the conditional. Fixed. > @@ +485,3 @@ > + pipeline = gst_pipeline_new ("pipeline"); > + > + audiosrc = gst_element_factory_make ("filesrc", "audio-source"); > > giosrc > > @@ +486,3 @@ > + > + audiosrc = gst_element_factory_make ("filesrc", "audio-source"); > + g_object_set (G_OBJECT (audiosrc), "location", audio_path, NULL); > > and pass it the GFile in the "file" property. > > @@ +489,3 @@ > + g_free (audio_path); > + > + decoder = gst_element_factory_make ("decodebin", "decoder"); > > I would have used playbin to do the playback, set a "chromaprint + fakesink" > bin as the audio sink, and disabled video decoding. Which might remove the need > for the newpad_cb() function. I didn't touch the gstreamer part yet, but I'll do implementing this way.
So, I was mistaken when I said that the AcoustID returns by default the MusicBrainz Track ID. It is a PUID too but reserved to AcoustID database only. http://tickets.musicbrainz.org/browse/MBS-6052 As the main goal of this plugin is helping the MusicBrainz plugin as much as possible, I'm requesting more metadata to AcoustID in order to get MusicBrainz PUID to Relase, Recording and for the Artists. So, the following patches fixes this mistake and gather more data from this source. Basically, we generate the fingerprint from the file then use this fingerprint to get the acoustid-track-id and all recordings related to it; For instance, Get Lucky from Daft Punk: http://acoustid.org/track/ae39a770-90f7-4219-97da-10c818656baf
Created attachment 283713 [details] [review] acoustid: include the source
Created attachment 283714 [details] [review] acoustid: inclue simple tests
BTW, It would be awesome a hint on how identify if the chromaprint plugin is installed... the PKG_CHECK_MODULES(GSTREAMER_BAD,...) is not right, probably.
(In reply to comment #18) > BTW, It would be awesome a hint on how identify if the chromaprint plugin is > installed... the PKG_CHECK_MODULES(GSTREAMER_BAD,...) is not right, probably. Look in totem's configure.ac which checks for the playback and videoscale plugins.
Review of attachment 283713 [details] [review]: Looks good, other than the GStreamer bits. ::: configure.ac @@ +1245,3 @@ + AC_MSG_ERROR([gstreamer-1.0 not found, install it or use --disable-acoustid]) + fi + if test "x$HAVE_GSTREAMER_BAD" = "xno"; then As per comment, use gst-inspect-1.0 and check for the existence of the plugin (see totem's configure.ac)
Review of attachment 283714 [details] [review]: You will also need to use mock data to make sure the test works without network access. ::: tests/acoustid/test_acoustid_utils.c @@ +34,3 @@ + + config = grl_config_new (ACOUSTID_ID, NULL); + grl_config_set_api_key (config, "isYcQm3L"); Use a #define
Add to the "request" component.
Okay, I can finish it. Just one question: Couldn't this be actually implemented in bug 749458 ? (Not sure how GstDiscover works, but this is source runs gstreamer with acoustid plugin...)
Working on it. I've ported to current API and I'll be addressing the last review. I'm still unsure if this will be soon deprecated due bug 749458 in case GstDiscover could provide the acoustid.
Created attachment 321623 [details] [review] acoustid: Include AcoustID source
Created attachment 321624 [details] [review] acoustid: Include tests
I did not touch the grilo-test-ui but that one should be quite simple. Besides the make test, I'm using grl-launch to get fingerprint from GRL_DEBUG <snip> GRL_DEBUG=acoustid:* grl-launch-0.3 -S --grl-plugin-path=/home/vtosodec/work/jhbuild/dev/lib/grilo-0.3 -C ~/.config/grilo-test-ui/tokens.conf -k duration resolve grlaudio://grl-acoustid/?url=sample.flac </snip> Changes from the version of 2 years ago: - rebased - using new api - checking gst elements on grl-acoustid.c against NULL upon creatoin - checking chromaprint in configure.ac (based on totem configure.ac)
The way I would have done it now is to separate the fetching of the fingerprint from the local file from the resolution with the web service. This is useful because we might be able to add the fingerprinting process to tracker directly, so files already have the fingerprint once on disk. The web service part of the resolution can then be implemented in Lua.
(In reply to Bastien Nocera from comment #28) > The way I would have done it now is to separate the fetching of the > fingerprint from the local file from the resolution with the web service. > > This is useful because we might be able to add the fingerprinting process to > tracker directly, so files already have the fingerprint once on disk. The > web service part of the resolution can then be implemented in Lua. Right, it does makes sense. I'll implement it this way. Regarding generating the fingerprint, should it be done in a similar way that I've been doing here or the GstDiscovery in bug 749458 should be the one doing it?
This other plugin could indeed be used for that, but it needs more work, and I'm not sure whether Matthieu will follow-up on this work.
Not sure if it is a hard dependency but I've created a bug to move the chromaprint plugin from -bad to -good https://bugzilla.gnome.org/show_bug.cgi?id=762452
Created attachment 322926 [details] [review] remove option: --enable-apple-trailers Since 8e3135b41f73e0 the C plugin does not exist anymore.
Created attachment 322927 [details] [review] tests: one lyric was changed in the metrolyrics
Created attachment 322928 [details] [review] gstreamer: include gstreamer plugin At this moment, this plugin get only information for audios but it can be extended to provide metadata from videos as well, using different types of plugins from gstreamer.
Created attachment 322929 [details] [review] gstreamer: include tests for this plugin At this moment, only testing audio metadata from two small music files attached to the tests
Created attachment 322930 [details] [review] lua-factory: not warn for unknown keys in source table At load time, lua-sources my rely on metadata-keys created in another plugin. The warning would make any test on lua sources to fail unless it loads all necessary plugins for its metadata-keys. e.g. (test_local_metadata:9549): Grilo-WARNING **: [lua-factory] grl-lua-factory.c:895: Unknown key 'acoustid-fingerprint' in property 'required' for source 'grl-acoustid'
Created attachment 322931 [details] [review] lua-factory: warn on sure mistakes at source table This is a good way to pinpoint simple mistakes when creating the global source table
Created attachment 322932 [details] [review] lua-factory: use requested-keys as keys for lua api We always provided the requested-keys to the lua source as an array with the metadata-keys as values. That's not so interesting as the source will need to walk in the array to check which keys were requested. As from commit 2bfcff90d589d43351 we started introducing the requested-keys as an argument, it would be good to use this moment and improve it. A table with the metadata-keys as key could be easily accessed in the source e.g if requested_keys.artist then media.artist = my_artist end
Created attachment 322933 [details] [review] acoustid: add lua sources for audio identification Based on chromaprint fingerprint the AcoustID source can provice important metadata related to the recording.
Created attachment 322934 [details] [review] acoustid: including tests for the lua source I'm including real fingerprint of four recordings in order to make this test an easy way to improve acoustid in the future. For daily tests, the net is mocked. It is also important to notice that acoustid needs a metadata-key created by gstreamer which makes necessary to load grl-gstreamer for this test.
Review of attachment 322926 [details] [review]: Sure.
Review of attachment 322927 [details] [review]: Yes.
Review of attachment 322928 [details] [review]: ::: src/gstreamer/grl-gstreamer.c @@ +203,3 @@ +/* ======================= Utilities ==================== */ +static gchar * +get_path_to_file (const gchar *url) Why do you need the path? A file on HTTP will not have a path through gvfs, but GStreamer will happily handle it. @@ +369,3 @@ + + /* Create the elemtens */ + audiosrc = gst_element_factory_make ("filesrc", "audio-source"); Why not use playbin, so that the source is autoplugged, and add chromaprint as an audio filter. @@ +467,3 @@ + +static gboolean +grl_gstreamer_source_may_resolve (GrlSource *source, Maybe check for the presence of the GStreamer plugins in the GstRegistry?
Review of attachment 322929 [details] [review]: ::: tests/gstreamer/test_gstreamer_resolve.c @@ +75,3 @@ + + struct { + gchar *url; Those aren't URLs, those are filepaths.
Review of attachment 322928 [details] [review]: Also, not sure that the plugin should be called gstreamer, there's another plugin that Matthieu wanted to get in which uses GstDiscoverer instead.
Review of attachment 322930 [details] [review]: > lua-sources my "may"? > warning would make any test on lua sources to fail "would cause [...] to fail" or "would make [...] fail" Are you sure this patch doesn't break something else in the Lua plugins? Note that we do have a "register_keys" method for plugins, which might not be used as much as it should (even if it doesn't fix the whole problem).
Review of attachment 322931 [details] [review]: > warn on sure mistakes at source table "Warn on certain mistakes in the source table"
Review of attachment 322932 [details] [review]: That looks fine to me.
Review of attachment 322933 [details] [review]: Looks fine to me. > can provice can provide ::: src/lua-factory/sources/grl-acoustid.lua @@ +58,3 @@ +--------------------------------- +function grl_source_init(configs) + if not configs or not configs.api_key then We have a required config_keys of "api-key", will this even be called?
Review of attachment 322934 [details] [review]: Looks good. > It is also important to notice that acoustid to note
(In reply to Bastien Nocera from comment #45) > Review of attachment 322928 [details] [review] [review]: > > Also, not sure that the plugin should be called gstreamer, there's another > plugin that Matthieu wanted to get in which uses GstDiscoverer instead. I talked to him sometime ago, he does not have much plans for it at the moment it seems. I don't mind if the GstDiscoverer deprecates this one as long as it can still provide the metadata and the tests keep working fine! Maybe grl-gstplaybin for the time being?
Thanks for the review! (In reply to Bastien Nocera from comment #43) > Review of attachment 322928 [details] [review] [review]: > > ::: src/gstreamer/grl-gstreamer.c > @@ +203,3 @@ > +/* ======================= Utilities ==================== */ > +static gchar * > +get_path_to_file (const gchar *url) > > Why do you need the path? A file on HTTP will not have a path through gvfs, > but GStreamer will happily handle it. Indeed, I'll change it > @@ +369,3 @@ > + > + /* Create the elemtens */ > + audiosrc = gst_element_factory_make ("filesrc", "audio-source"); > > Why not use playbin, so that the source is autoplugged, and add chromaprint > as an audio filter. Agreed! > > @@ +467,3 @@ > + > +static gboolean > +grl_gstreamer_source_may_resolve (GrlSource *source, > > Maybe check for the presence of the GStreamer plugins in the GstRegistry? What do you think should be runtime check and buildtime check? For me, we can have a runtime check for chromaprint and if we have the plugin we can provide the acoustid-fingerprint (maybe I should rename it to chromaprint as well?)
(In reply to Bastien Nocera from comment #49) > Review of attachment 322933 [details] [review] [review]: > > Looks fine to me. > > > can provice > > can provide Fixed > > ::: src/lua-factory/sources/grl-acoustid.lua > @@ +58,3 @@ > +--------------------------------- > +function grl_source_init(configs) > + if not configs or not configs.api_key then > > We have a required config_keys of "api-key", will this even be called? Yes! when grl_source_init() is defined, lua-factory calls it if source has provided all required config_keys. For later use, we are storing this on a global table which I don't like that much now...
(In reply to Bastien Nocera from comment #46) > Review of attachment 322930 [details] [review] [review]: > > > lua-sources my > > "may"? > > > warning would make any test on lua sources to fail > > "would cause [...] to fail" > or > "would make [...] fail" Thanks! > > > Are you sure this patch doesn't break something else in the Lua plugins? I'll double check to be sure > > Note that we do have a "register_keys" method for plugins, which might not > be used as much as it should (even if it doesn't fix the whole problem). I don't think we use it in any upstream source but I guess this patch will make easier to use it. I'll double check if it does not break anything. I'll push later the accepted patches and rework the others. Thanks again for the review
Attachment 322926 [details] pushed as 00fd2be - remove option: --enable-apple-trailers Attachment 322927 [details] pushed as 471b782 - tests: one lyric was changed in the metrolyrics Attachment 322930 [details] pushed as fa5539b - lua-factory: not warn for unknown keys in source table Attachment 322932 [details] pushed as 72a4e9d - lua-factory: use requested-keys as keys for lua api
I pushed the fixes and lua-factory improvements but I'll rework the acoustid after Bug 763046 is solved and then rework the grl-gstplaybin :)
Created attachment 327426 [details] [review] chromaprint: include chromaprint plugin At this moment, this plugin get only information for audios but it can be extended to provide metadata from videos as well, using different types of plugins from gstreamer. ~~~~ changes in this version ~~~~ * renamed grl-gstreamer to grl-chromaprint as suggested. The main reason for that is that this source is to providing the chromaprint fingerprint only; * grl-chromaprint using playbin * using uri instead of file-path * renamed the fingerprint key from acoustid-fingerprint to chromaprint; chromaprint is the specific fingerprint key from acoustid and also the new name of the grl-gstreamer plugin.. looks better to me. * small leaks fixed * Copyright to Grilo Project
Created attachment 327427 [details] [review] chromaprint: include tests for this plugin At this moment, only testing audio metadata from two small music files attached to the tests
Created attachment 327428 [details] [review] acoustid: add lua sources for audio identification Based on chromaprint fingerprint the AcoustID source can provice important metadata related to the recording. ~~~~ changes in this version ~~~~ * Ported to the current lua-factory api and adapt the changes from grl-chromaprint (chromaprint key)
Created attachment 327429 [details] [review] acoustid: including tests for the lua source I'm including real fingerprint of four recordings in order to make this test an easy way to improve acoustid in the future. For daily tests, the net is mocked. It is also important to note that acoustid needs a metadata-key created by chromaprint which makes necessary to load grl-chromaprint for this test.
Review of attachment 322931 [details] [review]: I probably forgot to git-bz push this one. Pushed as https://git.gnome.org/browse/grilo-plugins/commit/?id=fc9cfe1dc167c6a7b843b91786e9d94b3d0b94a5
Created attachment 327430 [details] [review] chromaprint: include chromaprint plugin At this moment, this plugin get only information for audios but it can be extended to provide metadata from videos as well, using different types of plugins from gstreamer. ~~~~ changes in this version ~~~~ small fix... better to check Comment 57 for the changes
Review of attachment 327430 [details] [review]: Looks fine to me, but could you get a GStreamer hacker to review it as well?
Review of attachment 327429 [details] [review]: Looks fine to me.
Review of attachment 327427 [details] [review]: Looks good.
Review of attachment 327428 [details] [review]: Looks fine otherwise. ::: src/lua-factory/sources/grl-acoustid.lua @@ +49,3 @@ +------------------ +acoustid = {} +ACOUSTID_LOOKUP = "https://api.acoustid.org/v2/" .. This would be nicer in one line. @@ +52,3 @@ + "lookup?client=%s" .. + "&meta=recordings+releasegroups" .. + "&duration=%d&fingerprint=%s" Could you add a link to the API documentation, so that we can know in which unit the "duration" parameter is? @@ +70,3 @@ + not media.duration or + not media.chromaprint or + #media.chromaprint == 0 then Is it a constant length? Might be worth checking for that length.
(In reply to Bastien Nocera from comment #66) > Review of attachment 327428 [details] [review] [review]: > > Looks fine otherwise. > > ::: src/lua-factory/sources/grl-acoustid.lua > @@ +49,3 @@ > +------------------ > +acoustid = {} > +ACOUSTID_LOOKUP = "https://api.acoustid.org/v2/" .. > > This would be nicer in one line. Ok > > @@ +52,3 @@ > + "lookup?client=%s" .. > + "&meta=recordings+releasegroups" .. > + "&duration=%d&fingerprint=%s" > > Could you add a link to the API documentation, so that we can know in which > unit the "duration" parameter is? I'll include in the patch, it is: https://acoustid.org/webservice#lookup > > @@ +70,3 @@ > + not media.duration or > + not media.chromaprint or > + #media.chromaprint == 0 then > > Is it a constant length? Might be worth checking for that length. Not really, the length depends on the music duration. I'm just checking to see if it's not an empty string.
(In reply to Bastien Nocera from comment #63) > Review of attachment 327430 [details] [review] [review]: > > Looks fine to me, but could you get a GStreamer hacker to review it as well? Sure :) Thanks for the reviews
Review of attachment 327430 [details] [review]: The GStreamer part looks good (didn't run it, though). Any chance a file with video could slip through this code? It would mean that playbin would try to play the video and, likely, a video window would pop-up to show it. You can use the playbin flags to disable the video.
Created attachment 327880 [details] [review] chromaprint: include chromaprint plugin At this moment, this plugin get only information for audios but it can be extended to provide metadata from videos as well, using different types of plugins from gstreamer. ~~ changes in this version * fix leak on chromaprint element * fix source description * disable video from playbin diff: http://paste.fedoraproject.org/366336/14632318/
Created attachment 327881 [details] [review] chromaprint: include tests for this plugin At this moment, only testing audio metadata from two small music files attached to the tests ~~no changes~~
Created attachment 327882 [details] [review] acoustid: add lua sources for audio identification Based on chromaprint fingerprint the AcoustID source can provice important metadata related to the recording. ~~changes: * include documentation for LOOKUP method from acoustid * moved string to one line * diff: http://paste.fedoraproject.org/366337/46323210/
Created attachment 327883 [details] [review] acoustid: including tests for the lua source I'm including real fingerprint of four recordings in order to make this test an easy way to improve acoustid in the future. For daily tests, the net is mocked. It is also important to note that acoustid needs a metadata-key created by chromaprint which makes necessary to load grl-chromaprint for this test. ~~no changes~~
Review of attachment 327880 [details] [review]: Looks good.
Review of attachment 327881 [details] [review]: Looks good.
Review of attachment 327882 [details] [review]: ::: src/lua-factory/sources/grl-acoustid.lua @@ +57,3 @@ +--------------------------------- +function grl_source_init (configs) + -- FIXME: https://bugzilla.gnome.org/show_bug.cgi?id=766094 I don't think we need to add that FIXME, it's already filed, and there are more sources to fix. "git get grl_source_init" will take care of finding the ones to fix.
Review of attachment 327883 [details] [review]: I would have preferred having this in the same directory as the chromaprint tests, as they're clearly related. Up to you. ::: tests/lua-factory/sources/test_lua_acoustid.c @@ +162,3 @@ +/* FIXME: As we need to load grl-chromaprint in order to grl-acoustid to work, + * we are creating another _setup function with custom values from now. + * If this gets common, the test_lua_factory_utils should be improved */ There's already a bug about this one as well, right? Worth removing too.
I've removed the FIXMEs but kept the grl-acoustid and chromaprint tests separated. Pushing! :)
Attachment 327880 [details] pushed as db1f2d5 - chromaprint: include chromaprint plugin Attachment 327881 [details] pushed as b93248e - chromaprint: include tests for this plugin Attachment 327882 [details] pushed as d8f26f2 - acoustid: add lua sources for audio identification Attachment 327883 [details] pushed as 8a638bc - acoustid: including tests for the lua source