GNOME Bugzilla – Bug 741230
MusicBrainz source to Grilo
Last modified: 2018-09-24 09:31:43 UTC
Patchs: include 'mb-recording-id'; include grl-musicbrainz source; musicbrainz tests. I'm actually trying to improve the tests with other types of music (eletronic, jazz, orchestra, ...) in order to find problems or improvements. The current state of this source only 'resolve' metadata from one of its ids: mb-artist-id, mb-recording-id and mb-release-id. The webservice has search/browse API which could be implemented. At this stage, I want to finish the integration with AcoustID (#732879) which provides the necessary ids to resolve.
Created attachment 292272 [details] [review] core: add GRL_METADATA_KEY_MB_RECORDING_ID MusicBrainz recording identifier. From its documentation: A recording is an entity in MusicBrainz which can be linked to tracks on releases. Each track must always be associated with a single recording, but a recording can be linked to any number of tracks.
Created attachment 292273 [details] [review] musicbrainz: Include MusicBrainz source Source for MusicBrainz
Created attachment 292274 [details] [review] musicbrainz: Include MusicBrainz tests Tests for MusicBrainz source
Review of attachment 292272 [details] [review]: Looks good to me.
Review of attachment 292273 [details] [review]: I didn't dwell inside the resolution code though. Is it valgrind clean? Does it work? Then good ;) ::: src/musicbrainz/grl-musicbrainz.c @@ +153,3 @@ + spec = g_param_spec_string ("mb-artist-country", + "mb-artist-country", + "Country which an artist is primarily identified with", There's already GRL_METADATA_KEY_REGION for that. @@ +159,3 @@ + grl_registry_register_metadata_key (registry, spec, NULL); + + spec = g_param_spec_string ("mb-artist-gender", This would need to be an enum instead. @@ +194,3 @@ + grl_registry_register_metadata_key (registry, spec, NULL); + + spec = g_param_spec_string ("mb-artist-type", An enum as well. @@ +211,3 @@ + grl_registry_register_metadata_key (registry, spec, NULL); + + spec = g_param_spec_string ("mb-album-barcode", In which format will the barcode be? @@ +219,3 @@ + grl_registry_register_metadata_key (registry, spec, NULL); + + spec = g_param_spec_string ("mb-album-country", Ditto about using the REGION. @@ +227,3 @@ + grl_registry_register_metadata_key (registry, spec, NULL); + + spec = g_param_spec_boxed ("mb-album-date", GRL_METADATA_KEY_PUBLICATION_DATE ? @@ +283,3 @@ + "source-name", SOURCE_NAME, + "source-desc", SOURCE_DESC, + "supported-media", GRL_MEDIA_TYPE_AUDIO, Add tags? @@ +343,3 @@ + + mb_source->priv->supported_keys = g_list_copy (mb_source->priv->artist_keys); + last = g_list_last (mb_source->priv->supported_keys); Use g_list_concat instead of doing it by hand? @@ +588,3 @@ + gchar *country = xml_get_country_from_area (it); + mb_set_string_if_key (GRL_DATA (os->media), os->keys, + GRL_MB_METADATA_KEY_RELEASE_COUNTRY, Shouldn't you be using related keys for those? @@ +852,3 @@ + for (it = os->keys; it != NULL; it = it->next) { + if ((os->mb_operation & MB_OP_ARTIST) == 0 + && g_list_find (mb_source->priv->artist_keys, it->data)) { This conditional and the one below could probably live in a helper. @@ +913,3 @@ + + /* Check if media type match and if key is supported*/ + if (!media || !GRL_IS_MEDIA_AUDIO (media) Line feed after || @@ +914,3 @@ + /* Check if media type match and if key is supported*/ + if (!media || !GRL_IS_MEDIA_AUDIO (media) + || !g_list_find (mb_source->priv->supported_keys, gp_key_id)) { || at the end of the line. @@ +921,3 @@ + * with the name of the album. */ + if (g_list_find (mb_source->priv->release_keys, gp_key_id)) { + if (grl_data_has_key (GRL_DATA (media), GRL_METADATA_KEY_MB_ALBUM_ID) Can you split that conditional into 2? It's not very readable as-is. @@ +923,3 @@ + if (grl_data_has_key (GRL_DATA (media), GRL_METADATA_KEY_MB_ALBUM_ID) + || (grl_data_has_key (GRL_DATA (media), GRL_METADATA_KEY_MB_RECORDING_ID) + && grl_data_has_key (GRL_DATA (media), GRL_METADATA_KEY_ALBUM))) { Ditto about the operator on the end of the line. @@ +937,3 @@ + + /* With mb-recording-id we can get all other metadata */ + if (grl_data_has_key (GRL_DATA (media), GRL_METADATA_KEY_MB_RECORDING_ID)) { No need for braces for one-line conditionals. @@ +943,3 @@ + /* Without mb-recording-id we can only resolve artist's keys with mb-artist-id */ + if (g_list_find (mb_source->priv->artist_keys, gp_key_id)) { + if (grl_data_has_key (GRL_DATA (media), GRL_METADATA_KEY_MB_ARTIST_ID)) { Ditto.
Review of attachment 292274 [details] [review]: Looks good overall. ::: tests/musicbrainz/test_musicbrainz_resolve_artist.c @@ +36,3 @@ + * don't provide precise Date for all cases */ + if (g_strcmp0 (a->life_begin, b->life_begin) != 0) + g_message ("(life-begin of %s) '%s' does ont match '%s'", not. @@ +40,3 @@ + + if (g_strcmp0 (a->life_end, b->life_end) != 0) + g_message ("(life-end of %s) '%s' does ont match '%s'", Ditto. @@ +45,3 @@ + +static void +free_mb_artist (mb_artist *a) Move the free to the utils?
Comment on attachment 292272 [details] [review] core: add GRL_METADATA_KEY_MB_RECORDING_ID Attachment 292272 [details] pushed as 5c1eb9e - core: add GRL_METADATA_KEY_MB_RECORDING_ID
Add to the "request" component.
Review of attachment 292273 [details] [review]: It would be great to have MusicBrainz support in Grilo. I've had a quick look through from the perspective of using this in sound-juicer to get rid of libmusicbrainz. There are a few things that stand out. 1 - It would be good to use https for the queries 2 - For sound-juicer we'd need a way to add extra includes to the MusicBraniz queries to get the information we need so there would need to be support for callers to specify the keys they wanted to be returned. It would probably be quite a bit of work to support all the keys that sound-juicer uses. 3 - As MusicBraniz does not support just retrieving one disc from an album some of the queries generate large amounts of data¹ if the disc belongs to a box set. Therefore the data might need to be processed in chunks to avoid blocking the main loop ¹https://musicbrainz.org/ws/2/release/8a4365ca-02ca-49e2-af3c-676eac91018c?inc=aliases+artists+artist-credits+labels+recordings+release-groups+url-rels+discids+recording-level-rels+work-level-rels+work-rels+artist-rels returns 4MB
Created attachment 331741 [details] [review] Musicbrainz: Add lua source Only fetches data using musicbrainz recording-id but makes use of different relations to obtain comprehensive metadata for a music piece. However, it should be noted: 1. This plugin can still be extended to provide a lot of data from artist-id and release-id (as well as release-group and work-id). 2. Musicbrainz is currently undergoing some schema change and reorganisation. Their current hosting service gets overloaded easily but will be upgraded later. For the moment a lot of webservice requests are being answered with 'Limit reached' or 'Service unavailable' replies.[0] 3. This plugin might send two requests consecutively if a valid work-id is found, further leading to the occasional 'source is broken' warning, when failed with a failed grl.fetch result (i.e. 'Unavailable service' or 'Limit reached'). Some help here would be appreciated. [0] https://community.metabrainz.org/t/something-wrong-with-the-webservice/48561/4
Created attachment 331743 [details] [review] Musicbrainz: Add tests for lua source Including tests for the lua source provided. I feel these mocked samples should provide proof of functionality. If the samples can be better in any way, do tell. Also I'm resusing the config.ini in the sources/data directory. Its probably a better option than a config file in the sources directory itself. Hence, also changed the default config.ini path for the test_init function in lua_factory_utils.c.
Review of attachment 331741 [details] [review]: Hey, So, you wrote the plugin based on the documentation at [0] which was updated just after Morse's patch were in [1] but part of that was reverted in code [2] but not in the documentation, sorry about that. With that being said, there isn't much to change regarding that, I'll point it out for you. The best examples are the sources upstream which we keep up to date with the lua-factory API. ::: src/lua-factory/sources/Makefile.am @@ +18,3 @@ grl-guardianvideos.lua \ grl-musicbrainz.lua \ + grl-musicbrainz-md.lua \ You need an extra tab here. Now, I would really prefer that the grl-musicbrainz file would be renamed to grl-musicbrainz-coverart which is already its source-id and keep this new source with grl-musicbrainz filename. ::: src/lua-factory/sources/grl-musicbrainz-md.lua @@ +19,3 @@ + "mb-artist-id", + "mb-album-id", + "mb-release-date" You should only include the keys you can set at this moment @@ +25,3 @@ + ["type"] = "audio", + required = { + "mb-recording-id" You can have mb-recording-id as required but this will not work well in the future, when you want to extend this plugin to support others mb-*-id I would include this in the optional field instead and check for it in the grl_source_resolve function. @@ +45,3 @@ +MUSICBRAINZ_LOOKUP_RELEASEGROUP = MUSICBRAINZ_BASE .. "release-group/%s?inc=tags" +MUSICBRAINZ_LOOKUP_WORK = MUSICBRAINZ_BASE .. "work/%s?inc=artist-rels" +MUSICBRAINZ_QUERY = MUSICBRAINZ_BASE .. "%s?query=%s" Not being used: LOOKUP_ARTIST, LOOKUP_RELEASE, LOOKUP_RELEASEGROUP, _QUERY From the C source I @@ +51,3 @@ +--------------------------------- + +function grl_source_resolve(media, options, callback) lua-factory API on grl_source_resolve has no arguments. That means that all the three above will be nil. You can remove it without issues. @@ +52,3 @@ + +function grl_source_resolve(media, options, callback) + media = grl.get_media_keys() Use `local media = ...` as you don't want to define a global variable @@ +54,3 @@ + media = grl.get_media_keys() + + if not media or As you have source.resolve_keys.type set, you should not need to check for media. @@ +68,3 @@ +--------------- + +function musicbrainz_execute_resolve(rec_id) maybe musicbrainz_resolve_by_recording_id is more clear? @@ +71,3 @@ + --[[Resolve recording related keys first as it can provide artist and + release related metadata if it were requested]] + work_id, rg_id = nil You are not using those variables here. @@ +72,3 @@ + release related metadata if it were requested]] + work_id, rg_id = nil + if rec_id then You are checking this twice (here and in grl_source_resolve). @@ +85,3 @@ +function fetch_recording_cb(results) + local recording = get_xml_entity(results, 'recording') + local media = {} In case you pass a media table to grl.callback, it will 'merge' this values with GrlMedia provided by user so there is not issue here but I often prefer to get the GrlMedia using grl.get_media_keys() and set it. @@ +89,3 @@ + if not recording then + return + else You don't need the else as it would return anyway. @@ +99,3 @@ + artist = recording['artist-credit']['name-credit'].artist + media.artist = artist.name.xml + media.mb_artist_id = artist.id You also need to check if the user requested to set this metadata. You can use grl.get_requested_keys() for that, like: local keys = grl.get_requested_keys() if keys.artist then media.artist = artist.name.xml end if keys.mb_artist_id then media.mb_artist_id = artist.id end @@ +104,3 @@ + if recording['relation-list'] and + recording['relation-list'].relation then + work_id = recording['relation-list'].relation.work.id There is not need to this to be global @@ +135,3 @@ + + if release['release-group'] then + rg_id = release['release-group'].id you move this check where it will be used, below @@ +178,3 @@ + +function fetch_work_relations_cb(results, media) + local work = get_xml_entity(results, 'work') Particularly, I think it is simpler to just get what you want instead of a generic function, like: local xml = grl.lua.xml.string_to_table(results) local work = xml and xml.metadata and xml.metadata.work But again, that's just personal preference. @@ +180,3 @@ + local work = get_xml_entity(results, 'work') + if not work or not work['relation-list'] then + grl.callback(media) you want to return after the grl.callback() -- always in a resolve operation. @@ +191,3 @@ + table.insert(media.author, rel.artist.name.xml) + end + end You did a internal function on purpose? @@ +196,3 @@ + media.author = {} + relation = work['relation-list'].relation + if relation.type then add_song_artist(relation) function in the line below @@ +198,3 @@ + if relation.type then add_song_artist(relation) + else + for key, val in pairs(relation) do you are not using key so you can use _ instead
> 1. This plugin can still be extended to provide a lot of data from artist-id > and release-id (as well as release-group and work-id). You plan to extend it after some feedback or is not in the plan for the moment? > 2. Musicbrainz is currently undergoing some schema change and > reorganisation. Their current hosting service gets overloaded easily but > will be upgraded later. For the moment a lot of webservice requests are > being answered with 'Limit reached' or 'Service unavailable' replies.[0] > 3. This plugin might send two requests consecutively if a valid work-id is > found, further leading to the occasional 'source is broken' warning, when > failed with a failed grl.fetch result (i.e. 'Unavailable service' or 'Limit > reached'). Some help here would be appreciated. Right. So, the problem is that you do two fetch operations faster then the webservice allows. This is not uncommon request by webservice... maybe we should have some extra configuration for this in the netopts like 'delay = 1' meaning that each request should have a second of difference. What do you think? > > [0] > https://community.metabrainz.org/t/something-wrong-with-the-webservice/48561/ > 4
(In reply to Victor Toso from comment #12) > Review of attachment 331741 [details] [review] [review]: > > Hey, > > So, you wrote the plugin based on the documentation at [0] which was updated > just after Morse's patch were in [1] but part of that was reverted in code > [2] but not in the documentation, sorry about that. With that being said, > there isn't much to change regarding that, I'll point it out for you. The > best examples are the sources upstream which we keep up to date with the > lua-factory API. And I forgot to put the link, bad bad toso. [0] https://people.freedesktop.org/~victortoso/html/ [1] https://bugzilla.gnome.org/show_bug.cgi?id=753141 [2] https://bugzilla.gnome.org/show_bug.cgi?id=763046
Review of attachment 331743 [details] [review]: Looks good ::: tests/lua-factory/sources/test_lua_factory_utils.c @@ +36,3 @@ * (e.g. if source rely on a website) */ if (net_mocked) { + g_setenv ("GRL_NET_MOCKED", LUA_FACTORY_SOURCES_DATA_PATH "config.ini", TRUE); hmm, before it was: $(abs_top_builddir)/tests/lua-factory/sources/ now it is: $(abs_top_srcdir)/tests/lua-factory/sources/data/ So this is correct. Could you please do it in a separated patch? @@ +89,2 @@ grl_deinit (); } line was removed on purpose?
(In reply to Victor Toso from comment #13) > > 2. Musicbrainz is currently undergoing some schema change and > > reorganisation. Their current hosting service gets overloaded easily but > > will be upgraded later. For the moment a lot of webservice requests are > > being answered with 'Limit reached' or 'Service unavailable' replies.[0] > > 3. This plugin might send two requests consecutively if a valid work-id is > > found, further leading to the occasional 'source is broken' warning, when > > failed with a failed grl.fetch result (i.e. 'Unavailable service' or 'Limit > > reached'). Some help here would be appreciated. > > Right. So, the problem is that you do two fetch operations faster then the > webservice allows. > > This is not uncommon request by webservice... maybe we should have some > extra configuration for this in the netopts like 'delay = 1' meaning that > each request should have a second of difference. Until recently sound-juicer has never had any problems with rate limiting and it does at least 3 queries and often more, I think they use an average when implementing the rate-limit. The current problems seem to be that they refuse connections because they're overloaded not because that particular client is exceeding its rate-limit. > > [0] > > https://community.metabrainz.org/t/something-wrong-with-the-webservice/48561/ > > 4
Created attachment 332305 [details] [review] Musicbrainz: Add lua source
Created attachment 332306 [details] [review] Musicbrainz: Add tests for lua source
Created attachment 332307 [details] [review] Change path for config.ini used in mock tests
(In reply to Phillip Wood from comment #16) > (In reply to Victor Toso from comment #13) > > > 2. Musicbrainz is currently undergoing some schema change and > > > reorganisation. Their current hosting service gets overloaded easily but > > > will be upgraded later. For the moment a lot of webservice requests are > > > being answered with 'Limit reached' or 'Service unavailable' replies.[0] > > > 3. This plugin might send two requests consecutively if a valid work-id is > > > found, further leading to the occasional 'source is broken' warning, when > > > failed with a failed grl.fetch result (i.e. 'Unavailable service' or 'Limit > > > reached'). Some help here would be appreciated. > > > > Right. So, the problem is that you do two fetch operations faster then the > > webservice allows. > > > > This is not uncommon request by webservice... maybe we should have some > > extra configuration for this in the netopts like 'delay = 1' meaning that > > each request should have a second of difference. > > Until recently sound-juicer has never had any problems with rate limiting > and it does at least 3 queries and often more, I think they use an average > when implementing the rate-limit. The current problems seem to be that they > refuse connections because they're overloaded not because that particular > client is exceeding its rate-limit. Sure, there are several reasons for throttling (see [1]), but my suggestion is based on IP throttling, based on: | The rate at which your IP address is making requests is measured. If that | rate is too high, all your requests will be declined (http 503) until the | rate drops again. Currently that rate is (on average) 1 request per second. [1] https://musicbrainz.org/doc/XML_Web_Service/Rate_Limiting
> > > Right. So, the problem is that you do two fetch operations faster then the > > > webservice allows. > > > > > > This is not uncommon request by webservice... maybe we should have some > > > extra configuration for this in the netopts like 'delay = 1' meaning that > > > each request should have a second of difference. > > > > Until recently sound-juicer has never had any problems with rate limiting > > and it does at least 3 queries and often more, I think they use an average > > when implementing the rate-limit. The current problems seem to be that they > > refuse connections because they're overloaded not because that particular > > client is exceeding its rate-limit. > > Sure, there are several reasons for throttling (see [1]), but my suggestion > is based on IP throttling, based on: > > | The rate at which your IP address is making requests is measured. If that > | rate is too high, all your requests will be declined (http 503) until the > | rate drops again. Currently that rate is (on average) 1 request per > second. > > [1] https://musicbrainz.org/doc/XML_Web_Service/Rate_Limiting We already have that implemented in GrlNetWc (with a small bug); Now it is implemented/fixed in lua-factory too, see Bug 769331
Created attachment 332807 [details] [review] MusicBrainz: Add throttling to lua source Add throttling for inducing intervals between consequtive requests. This addition depends on Bug 769331.
Review of attachment 332305 [details] [review]: I'm looking at this from the perspective of ensuring it can be extended in the future for use with sound-juicer without breaking existing applications. Bear in mind that I'm familiar with the MusicBrainz web service but not with lua beyond reading some of the language spec on their website and I don't know much about grilo. I think this would be better broken into two commits, first rename the cover-art source and explain why in the commit message and then add the metadata source. That would make the diffs clearer. ::: src/lua-factory/sources/grl-musicbrainz-coverart.lua @@ +38,3 @@ +} + +MUSICBRAINZ_DEFAULT_QUERY = "http://coverartarchive.org/release/%s/front" I think the cover art archive supports https. (that should be separate from the renaming though) ::: src/lua-factory/sources/grl-musicbrainz.lua @@ -20,3 @@ - * ---]] - You'll want a copyright and license header like all the other plugins. @@ +84,3 @@ + recording['artist-credit']['name-credit'] and + recording['artist-credit']['name-credit'].artist then + artist = recording['artist-credit']['name-credit'].artist Should that be 'local artist = ...'? @@ +108,3 @@ + + if recording['release-list'] then + local release = recording['release-list']['release'][1] It would be good to give applications all the matching releases so they can decide which is most appropriate. Can this return a list instead? @@ +133,3 @@ + release_date = release_date .. '-01-01' + elseif #release_date == 7 then + release_date = release_date .. '-01' It might be clearer to handle these cases in the regex below. What happens if the date is something like 2004-4-4 which is 7 characters? @@ +140,3 @@ + + if release['medium-list'] then + medium = release['medium-list'].medium local medium = ...? @@ +153,3 @@ + --Add recording independent metadata like composer, writer, etc. to songs + if work_id then + song_work_url = string.format(MUSICBRAINZ_LOOKUP_WORK, work_id) It might be worth testing if any of the keys associated with this query have been requested first to speedup queries where the application isn't interested in the composer or lyricist. @@ +173,3 @@ + media.author = {} + relation = work['relation-list'].relation + if relation.type then add_song_artist(relation, media) I think this is safe at the moment but it won't be if more relationships are requested in the future as that would give more than one relation-list. e.g. https://musicbrainz.org/ws/2/work/73c797c4-e008-3eb0-924b-65f972f6cd0e?inc=aliases+artist-rels+work-rels @@ +186,3 @@ + if type == "composer" then + table.insert(media.composer, rel.artist.name.xml) + --[[For now writer and lyricist are stored in 'author' until seperate key for lyricist exists]] The missing keys should probably be added before this patch is pushed so that applications will not need updating in the future. Note that the writer can be the composer or the lyricist[1] [1] https://musicbrainz.org/relationships/artist-work
Created attachment 335774 [details] [review] Rename existing source from 'grl-musicbrainz' to 'grl-musicbrainz-coverart' A standalone patch for renaming the musicbrainz coverart plugin to an appropriate name.
Created attachment 335775 [details] [review] Musicbrainz: Add lua source
Review of attachment 335775 [details] [review]: This is looking better, I think splitting it up has made the changes clearer and checking the work keys are required before performing the extra query is good. I've just got a couple of comments. I think the date handling definitely needs to be fixed, I could be persuaded about leaving the others for now depending on what other people think. When I post an updated patch I try to write a short summary of the changes. As a patch author I find this useful as a check that I've addressed all of the reviewer's comments. For the reviewer it makes it easy to see which suggestions I agree with and provides me with an opportunity to explain why I disagree with any suggestions that I haven't implemented (or simply note I haven't got round to them yet). ::: src/lua-factory/sources/grl-musicbrainz.lua @@ +117,3 @@ + if recording['relation-list'] and + recording['relation-list'].relation then + work_id = recording['relation-list'].relation.work.id At the weekend I realized that a recording can have more than one work relation. This would be better if it returned a list. At the moment if there's more than one work I don't think it returns anything. @@ +134,3 @@ + + if recording['release-list'] then + local release = recording['release-list']['release'][1] I still think it would be better to return a list here to accurately represent what's in MusicBrainz @@ +159,3 @@ + while not y do + y, m, d = string.match(release_date, "(%d+)-(%d+)-(%d+)") + release_date = release_date .. "-01" I think you need to do release_date = release_date .. "01-01" y, m, d = string.match(release_date, "(%d+)-(%d+)-(%d+)") to make sure you can match 1906 1906-12 1906-12-9
(In reply to Phillip Wood from comment #26) > Review of attachment 335775 [details] [review] [review]: > When I post an updated patch I try to write a short summary of the changes. > As a patch author I find this useful as a check that I've addressed all of > the reviewer's comments. For the reviewer it makes it easy to see which > suggestions I agree with and provides me with an opportunity to explain why > I disagree with any suggestions that I haven't implemented (or simply note I > haven't got round to them yet). Thanks Phillip for reviewing these for me. I'm sorry I forgot to write a proper reply to the earlier review (although I had intended to). Some of the issues mentioned in the previous review were indeed not addressed. I'll cover them here. > ::: src/lua-factory/sources/grl-musicbrainz.lua > @@ +117,3 @@ > + if recording['relation-list'] and > + recording['relation-list'].relation then > + work_id = recording['relation-list'].relation.work.id > > At the weekend I realized that a recording can have more than one work > relation. This would be better if it returned a list. At the moment if > there's more than one work I don't think it returns anything. I was originally writing this plugin for using it in my own tag editor widget for GNOME Music, but I definitely understand that this should be a more general plugin for use in other applications. That said I don't think we can return a list of works related to a music recording. Work relations are being solely asked for the purpose of getting artists such as 'lyricist', 'composer', etc. We can have more than one of these artists, but I cannot think of a way to return multiple works each of which has its own set of artists. Ultimately a resolve operation in grilo has to return a GrlMedia[0] object with all the relevant details filled in. We might be looking at a situation where we need to browse a list of works. However a browse operation for the MusicBrainz plugin should not just be able to browse works for the recording. right? > @@ +134,3 @@ > + > + if recording['release-list'] then > + local release = recording['release-list']['release'][1] > > I still think it would be better to return a list here to accurately > represent what's in MusicBrainz Again, I don't know of a way of returning lists but we can have a browse operation. But only one browse operation. Maybe we can extend keys to support multiple MusicBrainz releases though it wouldn't be very helpful since we are more concerned with the attributes of each of those releases than the release itself. I'll need some help here but with a few changes maybe we can figure out a way of accurately representing whats in MusicBrainz for each recording. BTW I thought multiple recordings can be a part of the same work and not the other way around[1]. > @@ +159,3 @@ > + while not y do > + y, m, d = string.match(release_date, "(%d+)-(%d+)-(%d+)") > + release_date = release_date .. "-01" > > I think you need to do > release_date = release_date .. "01-01" > y, m, d = string.match(release_date, "(%d+)-(%d+)-(%d+)") > to make sure you can match > 1906 > 1906-12 > 1906-12-9 Wow! This is so much simpler. I'll update this in the next patch. [0]https://developer.gnome.org/grilo/unstable/GrlMedia.html [1]https://musicbrainz.org/doc/How_to_Use_Works
(In reply to Saiful B. Khan from comment #27) > Thanks Phillip for reviewing these for me. I'm sorry I forgot to write a > proper reply to the earlier review Don't worry, thanks for replying. > > ::: src/lua-factory/sources/grl-musicbrainz.lua > > @@ +117,3 @@ > > + if recording['relation-list'] and > > + recording['relation-list'].relation then > > + work_id = recording['relation-list'].relation.work.id > > > > At the weekend I realized that a recording can have more than one work > > relation. This would be better if it returned a list. At the moment if > > there's more than one work I don't think it returns anything. > > I was originally writing this plugin for using it in my own tag editor > widget for GNOME Music, but I definitely understand that this should be a > more general plugin for use in other applications. That said I don't think > we can return a list of works related to a music recording. Work relations > are being solely asked for the purpose of getting artists such as > 'lyricist', 'composer', etc. We can have more than one of these artists, but > I cannot think of a way to return multiple works each of which has its own > set of artists. Right I'd forgotten that the work-id wasn't returned to the caller. In that case can we walk the list of works appending the composers etc to the their respective lists as we do now and then remove any duplicates when we've looped over all the works. > Ultimately a resolve operation in grilo has to return a GrlMedia[0] object > with all the relevant details filled in. We might be looking at a situation > where we need to browse a list of works. However a browse operation for the > MusicBrainz plugin should not just be able to browse works for the > recording. right? Maybe it should browse the recording-id - see below. > > @@ +134,3 @@ > > + > > + if recording['release-list'] then > > + local release = recording['release-list']['release'][1] > > > > I still think it would be better to return a list here to accurately > > represent what's in MusicBrainz > > Again, I don't know of a way of returning lists but we can have a browse > operation. But only one browse operation. Maybe we can extend keys to > support multiple MusicBrainz releases though it wouldn't be very helpful > since we are more concerned with the attributes of each of those releases > than the release itself. > > I'll need some help here but with a few changes maybe we can figure out a > way of accurately representing whats in MusicBrainz for each recording. Perhaps Victor or Bastien have some suggestions? Maybe this plugin should be implementing browse rather than resolve? Then it could return a list of matches. Another possibility might be a custom key which gives a list of matching recordings but I'm not sure that's a good route. At the moment this plugin will always resolve the recording-id, if in the future it supports resolving other entities how does the caller specify which ones they’re interested in? BTW > I thought multiple recordings can be a part of the same work and not the > other way around[1]. Sometimes an artist records more than one work in a single track - see https://musicbrainz.org/recording/115686df-fb77-4339-93ce-2d1c6ca1fc51 for example. > > > @@ +159,3 @@ > > + while not y do > > + y, m, d = string.match(release_date, "(%d+)-(%d+)-(%d+)") > > + release_date = release_date .. "-01" > > > > I think you need to do > > release_date = release_date .. "01-01" > > y, m, d = string.match(release_date, "(%d+)-(%d+)-(%d+)") > > to make sure you can match > > 1906 > > 1906-12 > > 1906-12-9 > > Wow! This is so much simpler. I'll update this in the next patch. Glad you like it. It’s a shame grilo doesn’t use GstDate then we wouldn’t have to fake the missing data.
(In reply to Phillip Wood from comment #28) > (In reply to Saiful B. Khan from comment #27) > > > > ::: src/lua-factory/sources/grl-musicbrainz.lua > > > @@ +117,3 @@ > > > + if recording['relation-list'] and > > > + recording['relation-list'].relation then > > > + work_id = recording['relation-list'].relation.work.id > > > > > > At the weekend I realized that a recording can have more than one work > > > relation. This would be better if it returned a list. At the moment if > > > there's more than one work I don't think it returns anything. > > > > I was originally writing this plugin for using it in my own tag editor > > widget for GNOME Music, but I definitely understand that this should be a > > more general plugin for use in other applications. That said I don't think > > we can return a list of works related to a music recording. Work relations > > are being solely asked for the purpose of getting artists such as > > 'lyricist', 'composer', etc. We can have more than one of these artists, but > > I cannot think of a way to return multiple works each of which has its own > > set of artists. > > Right I'd forgotten that the work-id wasn't returned to the caller. In that > case can we walk the list of works appending the composers etc to the their > respective lists as we do now and then remove any duplicates when we've > looped over all the works. Yep we can do that definitely. This I will add as well in the next patch. > > > @@ +134,3 @@ > > > + > > > + if recording['release-list'] then > > > + local release = recording['release-list']['release'][1] > > > > > > I still think it would be better to return a list here to accurately > > > represent what's in MusicBrainz > > > > Again, I don't know of a way of returning lists but we can have a browse > > operation. But only one browse operation. Maybe we can extend keys to > > support multiple MusicBrainz releases though it wouldn't be very helpful > > since we are more concerned with the attributes of each of those releases > > than the release itself. > > > > I'll need some help here but with a few changes maybe we can figure out a > > way of accurately representing whats in MusicBrainz for each recording. > > Perhaps Victor or Bastien have some suggestions? Maybe this plugin should be > implementing browse rather than resolve? Then it could return a list of > matches. Another possibility might be a custom key which gives a list of > matching recordings but I'm not sure that's a good route. For a browse operation to happen we need to have a top level container (if I am not mistaken) which probably contains multiple media entities (with its set of attributes) or other browsable containers (yes like release lists) as well. But for a single recording-ID don't you think its a bit misleading to call it browsing. I'm not sure if the root container (recording in this case) is supposed to have properties (track number, artist, date) of its own. But to think of it all these properties are contained inside 'lists' in the MB XML data. > At the moment this plugin will always resolve the recording-id, if in the > future it supports resolving other entities how does the caller specify > which ones they’re interested in? I do plan to extend this to fetch release and artist metadata at least. The resolve (later browse maybe?) operation is called on a GrlMedia with a musicbrainz-recording-ID key filled in to fetch only recording related metadata. For getting data relevant to other entities we just set the value for that entity's MB ID and leave out other keys. I'll think of some mechanism to resolve only for that entity when I extend support. What other entities would you like support for. > BTW > > I thought multiple recordings can be a part of the same work and not the > > other way around[1]. > > Sometimes an artist records more than one work in a single track - see > https://musicbrainz.org/recording/115686df-fb77-4339-93ce-2d1c6ca1fc51 for > example. Classical music was very different from what I understand about music albums. I'll keep this in mind.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/grilo/issues/55.