After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 652972 - (ngs) Update Musicbrainz code for new web service API
(ngs)
Update Musicbrainz code for new web service API
Status: RESOLVED FIXED
Product: sound-juicer
Classification: Applications
Component: metadata
2.32.x
Other Linux
: Normal normal
: ---
Assigned To: Sound Juicer Maintainers
Sound Juicer Maintainers
: 651516 654201 654756 655734 655738 655740 656998 657782 661139 661141 661230 662483 663456 666901 667486 667728 668060 669449 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2011-06-20 05:50 UTC by James Henstridge
Modified: 2012-02-06 09:06 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description James Henstridge 2011-06-20 05:50:47 UTC
I was trying to rip a disc from a 2 disc release, and ran into some strange behaviour.

While the disc only has 17 tracks, Sound Juicer listed all 34 tracks from the full 2 disc release.  Clicking the "Extract" button resulted in 34 ogg files, where the last 17 contained the same audio as the first track.

I have sound-juicer 2.32.0 on my system and libmusicbrainz3 3.0.3.  The disc I was trying to rip had the MusicBrainz Disc ID "lFPr4443KHGblPKwl4gdVoKK5qA-", which comes from the following release:

  http://musicbrainz.org/release/6fbcbabb-5f58-41e3-8aec-ac976a6a2def

This problem is probably related to the recent change to the "next generation schema" at MusicBrainz.
Comment 1 James Henstridge 2011-06-20 05:52:16 UTC
Also, Sound Juicer did not fill in the disc number entry box after retrieving the metadata.
Comment 2 James Henstridge 2011-06-20 06:06:25 UTC
Bug 651516 in Rhythmbox looks similar.  Also, the following MusicBrainz bug seems to be relevant:

    http://tickets.musicbrainz.org/browse/MBS-2407

If they manage to fix up the old web service, then this problem might go away.  If not, then Sound Juicer will need to move to the new web service to fix the problem.
Comment 3 Bastien Nocera 2011-06-20 13:56:05 UTC
The old web service isn't coming back, it's been deprecated for ages.

And yes, the "get ID, use ID to look up contents" is what the code seems to do. Patches welcome.
Comment 4 Bastien Nocera 2011-06-20 13:56:50 UTC
*** Bug 651516 has been marked as a duplicate of this bug. ***
Comment 5 Bastien Nocera 2011-06-20 13:59:30 UTC
An easy way to test this is using "mb-test" in tests/. You can also force the detected "ID" using the MUSICBRAINZ_FORCE_DISC_ID envvar.

This probably needs changing as well.
Comment 6 James Henstridge 2011-06-21 01:58:50 UTC
I agree that the RDF web service has been deprecated for ages.

As far as I can tell, libmusicbrainz3 uses the version 1 XML web service, which has only been deprecated for a month or so.

Given that there doesn't seem to be much development on libmusicbrainz3, it might be easier to access the new version 2 web service directly.  We could either continue to use libmusicbrainz for disc ID calculation or switch to libdiscid.
Comment 7 James Henstridge 2011-06-21 03:33:50 UTC
From a quick look, it should still be possible to do the metadata lookup in two web service requests.

First look up the disc ID:

  http://musicbrainz.org/ws/2/discid/lFPr4443KHGblPKwl4gdVoKK5qA-?toc=...

Then look up the chosen release:

  http://musicbrainz.org/ws/2/release/6fbcbabb-5f58-41e3-8aec-ac976a6a2def?inc=artist-credits+discids+recordings

As opposed to the v1 API, this groups the recordings by medium allowing us to match the disc ID to a medium within the release.

I've got no idea what should be done if the disc ID is not known to Musicbrainz and we get a fuzzy match from the first query.
Comment 8 James Henstridge 2011-06-22 04:50:33 UTC
The following upstream bug seems to cover the problem of fuzzy matched disc IDs from multi-disc releases:

http://tickets.musicbrainz.org/browse/MBS-2318

Doesn't look like they have a solution yet.
Comment 9 Andy Hawkins 2011-06-29 15:59:53 UTC
I'm currently working on libmusicbrainz4, which is an interface into the new web service for NGS. Picking out disc information for a multi-disk set is a little more involved now, but is definitely possible.

I expect to make an early beta release in the next day or two.
Comment 10 Andy Hawkins 2011-07-06 16:01:50 UTC
Beta has now been released:

http://blog.musicbrainz.org/?p=965

Andy
Comment 11 Christophe Fergeau 2011-07-31 22:38:09 UTC
*** Bug 654756 has been marked as a duplicate of this bug. ***
Comment 12 Christophe Fergeau 2011-07-31 22:39:00 UTC
*** Bug 654201 has been marked as a duplicate of this bug. ***
Comment 13 Christophe Fergeau 2011-07-31 22:48:31 UTC
I've started adding support for libmusicbrainz4 to sound-juicer, so far I've written most of the metadata retrieval code and tested it with mb-test. However, I didn't port the code which generates a disc id from the disc present in the cdrom since mb_read_disc is gone. I think this is the job of libdiscid now, I'll have to look into that.
My work so far is available from git://people.freedesktop.org/~teuf/ (should be in cgit at http://cgit.freedesktop.org/~teuf/sound-juicer if all goes well). It's in no way ready for testing, I'm only commenting here to avoid other people starting the same work, and so that the code is "publicly" available/known in case it's useful
Comment 14 Christophe Fergeau 2011-08-01 18:01:00 UTC
*** Bug 655738 has been marked as a duplicate of this bug. ***
Comment 15 Christophe Fergeau 2011-08-01 23:37:06 UTC
I did some more work on this, and it should know be functionally equivalent to the musicbrainz3 metadata reader.I ran a few quick tests with both mb-test and sound-juicer which worked for me. I'll have to test it some more, cleanup the patches and put them here. It's all in the git repository mentioned above for now.
Comment 16 Ross Burton 2011-08-02 07:08:37 UTC
All hail teuf, new maintainer of sound-juicer!! :)

Thanks Christophe, that work is much appreciated.
Comment 17 Christophe Fergeau 2011-08-06 12:38:51 UTC
There is only one big issue remaining before I can propose these patches for merging, and I'd need input on that. libmusicbrainz4 provides more detailed metadata about the CD, in particular when multiple artists authored a particular track, we can get a list of artist names with their nationality, gender, ... There's also a bit more metadata about albums, for example a link to the album lyrics.
Exposing this new metadata would mean breaking libjuicer ABI, but keeping libjuicer ABI stable is easy, it just means we lose a bit of metadata. If libjuicer ABI is broken, we probably could use this as an opportunity to rename AlbumDetails and make it more easily extensible in the future, and make the GList * of AlbumDetails returned from the metadata signal a boxed type.

I don't have a strong feeling either way, so let me know where your preference go :)

For reference, here is the additional data I currently have in the musicbrainz4 code :

struct _SjMb4AlbumDetails {
    AlbumDetails parent;
    GList *artists;
    char *status;
    char *quality;
    char *disambiguation;
    char *packaging;
    char *country;
    char *barcode;
    char *type;
    char *format;
    char *lyrics_url;
};

struct _SjMb4ArtistDetails {
  char *id;
  char *name;
  char *sortname;
  char *disambiguation;
  char *gender;
  char *country;

  /* doesn't belong in here, prevent sharing the artist structure between
   * distinct ReleaseGroups - more convenient for now */
  char *joinphrase;
};
Comment 18 Ross Burton 2011-08-06 13:30:06 UTC
libjuicer is noinst, feel free to break ABI/API to add functionality just as long as the UI is updated at the same time.
Comment 19 James Henstridge 2011-08-13 00:21:06 UTC
Note that the "joinphrase" is a per-artist-credit field rather than per release.

Consider the artist credit on track 10 of this release:

http://musicbrainz.org/ws/2/release/08751945-fad1-4a8d-815f-2957a26eba00?inc=artist-credits+discids+recordings

Also note that the name that the artist can be credited under may now differ from the canonical artist name.  For example, consider this album:

http://musicbrainz.org/ws/2/release/94ada60f-857f-38e5-8adc-878ebdf08943?inc=artist-credits+discids+recordings

The artist is "Shihad", but is credited as "Pacifier".  The "joinphrase" belongs at the same level as this per release or per track credit information.

Alternatively, it might be fine to just expose a combined credit string (concatenate all the name credits with the appropriate join phrases) along with a list of the artist structures.
Comment 20 Christophe Fergeau 2011-08-13 18:56:05 UTC
(In reply to comment #19)
> Note that the "joinphrase" is a per-artist-credit field rather than per
> release.

Do you mean "rather than per artist"? I know the way the "joinphrase" is currently stored is suboptimal, when I coded it I was too lazy to add an ArtistCredit indirection containing the Artist list and the list of join phrases.

> 
> Also note that the name that the artist can be credited under may now differ
> from the canonical artist name.  For example, consider this album:
> 
> http://musicbrainz.org/ws/2/release/94ada60f-857f-38e5-8adc-878ebdf08943?inc=artist-credits+discids+recordings
> 
> The artist is "Shihad", but is credited as "Pacifier".  The "joinphrase"
> belongs at the same level as this per release or per track credit information.

Ah thanks for the link. Currently this name field is only used as a fallback if there no artist list can be found. I should be doing the opposite, use the name from artist credit as something overriding the names of the artists in the credit list.

> 
> Alternatively, it might be fine to just expose a combined credit string
> (concatenate all the name credits with the appropriate join phrases) along with
> a list of the artist structures.

I like this, I'll change it to do that some day (might have to wait after I'm back from holidays in a month from now...). Do you have a use for the detailed information that is fed from musicbrainz (artist list, ...)? I'm considering dropping some of it (or hiding it well) since it's overkill for sound-juicer.
Comment 21 James Henstridge 2011-08-16 07:07:45 UTC
I guess the main metadata I'd want stored in the ripped files include:

 * track title
 * album title
 * track artist
 * album artist
 * MusicBrainz IDs for the above two.  I guess this would involve repeated
   tags for tracks with more than one artist.
 * sortable version of artist credits.  Not sure how that works for tracks
   with more than one artist.
 * disc number for when disc comes from a multi-disc set.
 * release date
 * MusicBrainz IDs for recording and album.

In addition, some nice to have metadata would be:

 * Label and catalogue number.
 * Barcode.

It is also worth noting that the "choose a release" dialogue is going to pop up a lot more often with the new MusicBrainz schema.  Whereas previously an album in MusicBrainz represented all releases with the same track list, now each individual release is a separate entity.

Simply showing the album title and artist is not going to be enough to distinguish the different releases.  The Musicbrainz website shows the release date, country, label, catalogue number and barcode for this purpose, although you can probably do without the label and barcode.
Comment 22 Christophe Fergeau 2011-08-21 10:04:59 UTC
*** Bug 656998 has been marked as a duplicate of this bug. ***
Comment 23 Christophe Fergeau 2011-09-28 06:52:06 UTC
*** Bug 657782 has been marked as a duplicate of this bug. ***
Comment 24 Christophe Fergeau 2011-10-07 08:13:07 UTC
*** Bug 661141 has been marked as a duplicate of this bug. ***
Comment 25 Christophe Fergeau 2011-10-08 08:29:20 UTC
*** Bug 661230 has been marked as a duplicate of this bug. ***
Comment 26 Ross Burton 2011-10-10 15:24:05 UTC
*** Bug 655734 has been marked as a duplicate of this bug. ***
Comment 27 Ross Burton 2011-10-10 15:24:54 UTC
Christophe, what is the state of your branch?  Ready to merge? Anything outstanding?
Comment 28 Ross Burton 2011-10-10 15:27:13 UTC
*** Bug 661139 has been marked as a duplicate of this bug. ***
Comment 29 Ross Burton 2011-10-10 15:28:03 UTC
*** Bug 655740 has been marked as a duplicate of this bug. ***
Comment 30 Christophe Fergeau 2011-10-10 15:39:04 UTC
Nope this is not ready for a merge, as it is currently, it leaks "by design". It can be fixed either by removing some of the extra fields from musicbrainz4, by breaking some ABI (changing AlbumDetails/ArtistDetails/... to add the extra fields), or by changing the API sound-juicer uses to handle these structures.

I think the easiest/less confusing way is to go with a mix of #1 and #2, I'm planning to get back to this "soon"
Comment 31 Ross Burton 2011-10-17 15:18:08 UTC
libjuicer has a totally unstable API, and we might as well drop the musicbrainz3 backend, so I'm totally happy to break what needs to be broken so that we can merge the MB4 backend asap.

Would you be able to look at this, or if not can you summarise what you would do so I can work on it?
Comment 32 Christophe Fergeau 2011-10-17 19:50:40 UTC
The structures that needs to be worked on are the ones that can be found at http://cgit.freedesktop.org/~teuf/sound-juicer/tree/libjuicer/sj-metadata-musicbrainz4.c?h=musicbrainz#n91

To avoid changing AlbumDetails, I embedded them in an extended SjMb4XXXDetails where I added the new fields. However, there's a caveat with that (in addition to fact that it's a bit complicated), the memory allocated for these struct is not currently freed, and I wanted to get some feedback on the way to go before doing more changes.

As I understand it, the way to go is to move the extra fields to AlbumDetails/TrackDetails. As James mentioned, "joinphrase" doesn't really belong in SjMb4ArtistDetails but it's easier this way...

I also haven't managed yet to talk to the author of libmusicbrainz4 about his release plans, I'm a bit worried that some API changes are still being proposed :) And I'm not sure the library is packaged in distros. In short, even if this gets merged it's early to drop libmusicbrainz3 support.
Comment 33 Christophe Fergeau 2011-10-23 08:19:10 UTC
*** Bug 662483 has been marked as a duplicate of this bug. ***
Comment 34 Christophe Fergeau 2011-11-05 20:01:45 UTC
*** Bug 663456 has been marked as a duplicate of this bug. ***
Comment 35 James Broadhead 2011-11-05 20:37:07 UTC
From Bug #663456: 

* sound-juicer should never display / try to rip more tracks than exist on the CD in the tray, regardless of data returned by MBZ. 
* sound-juicer should error-out if it is requested to rip track N+1 or above
for a CD with N tracks 
* It should have better support for tagging / filenames of multiple disc
collections in general
Comment 36 Bruce Cowan 2012-01-05 14:59:51 UTC
*** Bug 666901 has been marked as a duplicate of this bug. ***
Comment 37 sam tygier 2012-01-07 18:25:28 UTC
http://cgit.freedesktop.org/~teuf/sound-juicer is gone. has it been moved or merged? I have some double CDs to rip, and would be happy to test.
Comment 38 Ross Burton 2012-01-07 21:07:01 UTC
There is a musicbrainz branch at gnome.org for which testing would be much appreciated!  Coding too, there's still work to be done.
Comment 39 Christophe Fergeau 2012-01-08 11:28:57 UTC
*** Bug 667486 has been marked as a duplicate of this bug. ***
Comment 40 Christophe Fergeau 2012-01-08 11:31:29 UTC
Sam, this must have been a temporary hiccup at freedesktop since it's present now. But as Ross recommends, using the branch from gnome.org is better :)
Comment 41 sam tygier 2012-01-08 17:17:48 UTC
with the musicbrainz4 branch from gnome git, and libmuscbrainz4 from the musicbrainz site i was able to rip a double cd with all the correct data.

there are lots of:
Gtk-CRITICAL **: gtk_icon_set_render_icon_pixbuf: assertion `icon_set != NULL' failed
in the terminal, but no other issues.
Comment 42 Christophe Fergeau 2012-01-08 21:11:33 UTC
Could you run sound-juicer with the --g-fatal-warnings command line argument and get a gdb backtrace when that happens? And can you test if you get the same warnings with git master?
Comment 43 Pierre Hily-Blant 2012-01-11 22:52:05 UTC
Dear all,
  I am sorry, but I don't know how to use the branch from gnome.org in order to install muscibrainz4. Regarding libmuscbrainz4, how do I install it ? Manually, or is there a rpm ? Is this ok with f15 ?

  Pierre
Comment 44 sam tygier 2012-01-11 23:03:07 UTC
here is the backtrace for the musicbrainz4 branch. Let me know if you need more detail.

Gtk-CRITICAL **: gtk_icon_set_render_icon_pixbuf: assertion `icon_set != NULL' failed

Program received signal SIGTRAP, Trace/breakpoint trap.
0x00007ffff53b68eb in g_logv () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
(gdb) bt
  • #0 g_logv
    from /lib/x86_64-linux-gnu/libglib-2.0.so.0
  • #1 g_log
    from /lib/x86_64-linux-gnu/libglib-2.0.so.0
  • #2 gtk_icon_set_render_icon_pixbuf
    at /build/buildd/gtk+3.0-3.3.6/./gtk/gtkiconfactory.c line 1705
  • #3 ensure_pixbuf_for_icon_set
    at /build/buildd/gtk+3.0-3.3.6/./gtk/gtkiconhelper.c line 279
  • #4 ensure_pixbuf_for_icon_set
    at /build/buildd/gtk+3.0-3.3.6/./gtk/gtkiconhelper.c line 567
  • #5 _gtk_icon_helper_ensure_pixbuf
    at /build/buildd/gtk+3.0-3.3.6/./gtk/gtkiconhelper.c line 302
  • #6 _gtk_icon_helper_get_size
    at /build/buildd/gtk+3.0-3.3.6/./gtk/gtkiconhelper.c line 334
  • #7 gtk_cell_renderer_pixbuf_get_size
    at /build/buildd/gtk+3.0-3.3.6/./gtk/gtkcellrendererpixbuf.c line 422
  • #8 gtk_cell_renderer_real_get_preferred_size
    at /build/buildd/gtk+3.0-3.3.6/./gtk/gtkcellrenderer.c line 1233
  • #9 gtk_cell_renderer_get_preferred_width
    at /build/buildd/gtk+3.0-3.3.6/./gtk/gtkcellrenderer.c line 1456
  • #10 gtk_cell_area_request_renderer
    at /build/buildd/gtk+3.0-3.3.6/./gtk/gtkcellarea.c line 3600
  • #11 compute_size
    at /build/buildd/gtk+3.0-3.3.6/./gtk/gtkcellareabox.c line 1533
  • #12 gtk_cell_area_box_get_preferred_width
    at /build/buildd/gtk+3.0-3.3.6/./gtk/gtkcellareabox.c line 1822
  • #13 gtk_tree_view_column_cell_get_size
    at /build/buildd/gtk+3.0-3.3.6/./gtk/gtktreeviewcolumn.c line 2908
  • #14 validate_row
    at /build/buildd/gtk+3.0-3.3.6/./gtk/gtktreeview.c line 6107
  • #15 validate_visible_area
  • #16 do_presize_handler
    at /build/buildd/gtk+3.0-3.3.6/./gtk/gtktreeview.c line 6783
  • #17 presize_handler_callback
    at /build/buildd/gtk+3.0-3.3.6/./gtk/gtktreeview.c line 6806
  • #18 gdk_threads_dispatch
    at /build/buildd/gtk+3.0-3.3.6/./gdk/gdk.c line 745
  • #19 g_main_context_dispatch
    from /lib/x86_64-linux-gnu/libglib-2.0.so.0
  • #20 ??
    from /lib/x86_64-linux-gnu/libglib-2.0.so.0
  • #21 g_main_loop_run
    from /lib/x86_64-linux-gnu/libglib-2.0.so.0
  • #22 gtk_main
    at /build/buildd/gtk+3.0-3.3.6/./gtk/gtkmain.c line 1164
  • #23 main
    at sj-main.c line 1939

Comment 45 sam tygier 2012-01-11 23:08:18 UTC
I get the same warning and backtrace with master
Comment 46 sam tygier 2012-01-11 23:13:07 UTC
@Pierre

get libmuscbrainz4 from http://musicbrainz.org/doc/libmusicbrainz
get the libmuscbrainz4 beta2 tar.gz, build with 
cmake . && make
sudo make install

get sound juicer from git:
git clone git://git.gnome.org/sound-juicer
cd sound-juicer
git checkout remotes/origin/musicbrainz4
./autogen
make
sudo make install
Comment 47 Pierre Hily-Blant 2012-01-11 23:32:58 UTC
To Sam: thank's a lot. That helped.
pierre
Comment 48 Christophe Fergeau 2012-01-12 08:31:57 UTC
*** Bug 667728 has been marked as a duplicate of this bug. ***
Comment 49 Christophe Fergeau 2012-01-12 23:57:25 UTC
(In reply to comment #32)
> The structures that needs to be worked on are the ones that can be found at
> http://cgit.freedesktop.org/~teuf/sound-juicer/tree/libjuicer/sj-metadata-musicbrainz4.c?h=musicbrainz#n91
> 
> To avoid changing AlbumDetails, I embedded them in an extended SjMb4XXXDetails
> where I added the new fields. However, there's a caveat with that (in addition
> to fact that it's a bit complicated), the memory allocated for these struct is
> not currently freed, and I wanted to get some feedback on the way to go before
> doing more changes.


I moved some of these new fields to AlbumDetails and killed the others since they seemed useless to me for a cd ripper, and I got rid of the SjMb4XXXDetails structures which means that the memory handling should be saner now. I've pushed this to http://cgit.freedesktop.org/~teuf/sound-juicer/commit/?h=musicbrainz (force push to remove one commit and remove an unused function from another one).

> 
> As I understand it, the way to go is to move the extra fields to
> AlbumDetails/TrackDetails. As James mentioned, "joinphrase" doesn't really
> belong in SjMb4ArtistDetails but it's easier this way...

This hasn't changed.
Comment 50 Santeri Paavolainen 2012-01-16 12:12:19 UTC
I've tested libmusicbrainz-4.0.0beta2.tar.gz and sound-juicer checkout "remotes/origin/musicbrainz4" (git describe: 2.32.0-91-gb7b8a95) on Ubuntu 10.10 and it works with multi-cd releases.

The only warning I get is "** (sound-juicer:10988): WARNING **: multiple artists
" (in case of multi-artist cd only).
Comment 51 Christophe Fergeau 2012-01-17 09:39:07 UTC
*** Bug 668060 has been marked as a duplicate of this bug. ***
Comment 52 Ross Burton 2012-01-19 10:46:54 UTC
Merged to master!

Thanks Christophe, you've been a hero.
Comment 53 Christophe Fergeau 2012-01-19 10:56:57 UTC
Ah great to see this merged! Now bug #666836 needs to be addressed :p
Comment 54 Ross Burton 2012-02-06 09:04:57 UTC
*** Bug 669449 has been marked as a duplicate of this bug. ***