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 697899 - [PATCH] libdiscid read() is slow starting with libdiscid 0.3.1
[PATCH] libdiscid read() is slow starting with libdiscid 0.3.1
Status: RESOLVED FIXED
Product: sound-juicer
Classification: Applications
Component: ripping
unspecified
Other Linux
: Normal minor
: ---
Assigned To: Sound Juicer Maintainers
Sound Juicer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-04-12 17:06 UTC by Johannes Dewender
Modified: 2013-09-23 09:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
use discid_read_sparse when available (1.35 KB, patch)
2013-04-12 17:19 UTC, Johannes Dewender
reviewed Details | Review

Description Johannes Dewender 2013-04-12 17:06:05 UTC
Starting with libdiscid 0.3.1 discid_read() doesn't just read the TOC (which is cached anyways), but reads ISRCs and the MCN.
This is unnecessary, since this data is not used while ripping and takes more time. (plain libdiscid read() now takes 3, rather than 0.5 seconds)

Starting with libdiscid 0.5.0 there is the option to use read_sparse() and only read the TOC again.

It would also be possible to read the MCN (= EAN/UPC/"barcode") and use that to decide which MusicBrainz release to use.


Since the whole point of sound-juicer is to read the whole disc, this is a small performance problem.
This is mainly about perceived performance, since sound-juicer reads the disc right at the start.
Comment 1 Johannes Dewender 2013-04-12 17:08:13 UTC
The upstream issue is at http://tickets.musicbrainz.org/browse/LIB-29
Comment 2 Johannes Dewender 2013-04-12 17:19:50 UTC
Created attachment 241374 [details] [review]
use discid_read_sparse when available

This patch uses discid_read_sparse() when available (a define falls back to read() otherwise) to only read the TOC.

Tested on my machine (and with some additional debug out added to libdiscid).
Without the patch ISRCs and MCNs are read and it takes a bit longer, with the patch the disc reading is faster and no ISRCs and MCNs are read.

Tested on Arch Linux using sound-juicer 3.5.0 and libdiscid 0.5.0.
(prior to 0.5.0 there is no difference without the patch).


The request to musicbrainz also took a bit of time and had lots of "Unrecognised relation attribute: 'type-id'" messages. That is a different problem though, I guess.
Comment 3 Johannes Dewender 2013-04-13 21:30:49 UTC
I posted a similar patch for goobox in
https://bugzilla.gnome.org/show_bug.cgi?id=697966
Comment 4 Bastien Nocera 2013-04-15 10:23:47 UTC
Review of attachment 241374 [details] [review]:

Looks fine otherwise.

::: libjuicer/sj-metadata-musicbrainz5.c
@@ +47,3 @@
 }
 
+#ifndef DISCID_HAVE_SPARSE_READ

If that defined in the header normally?
Comment 5 Johannes Dewender 2013-04-15 10:41:06 UTC
Yes, that is defined in discid/discid.h starting with libdiscid 0.5.0.
It was created for this exact purpose. There are no defines for other functions.

See:
http://jonnyjd.github.io/libdiscid/discid_8h.html
Comment 6 Bastien Nocera 2013-04-15 10:46:05 UTC
Looks good to me.
Comment 7 Christophe Fergeau 2013-09-23 09:52:25 UTC
I've pushed it, thanks for the patch and the review.
https://git.gnome.org/browse/sound-juicer/commit/?id=1a55823d38bf450d1280e6c6842cdaa19e7ea5bb