GNOME Bugzilla – Bug 693048
sound-juicer3.5.0 does not write year tag to flac files
Last modified: 2014-04-17 12:22:17 UTC
This is an upstream report of sound-juicer3.5.0 malfunction., which occures in Ubuntu13.04 development branch. The original report can be found at: https://bugs.launchpad.net/ubuntu/+source/sound-juicer/+bug/1109234 Mario Vukelic who is affected of the malfunction has given a reliable reproducing procedure, which is in the header of the launchpad report. A stacktrace seems not to be needed(easy reproducing). The header of https://bugs.launchpad.net/ubuntu/+source/sound-juicer/+bug/1109234 will be copied here. If there is need to test any further please contact the 1 affected people automatically by simple commenting on the launchpad report(I by myself only doing the upstream reporting). Thanks ---------------------------------------------(header of launchpad bug): I think it has been since my upgrade to Raring (currently sound-juicer 3.5.0) that sound-juicer stopped writing the year tag to the flac files it ripped. I can reproduce by: * Insert CD into drive, s-j starts automatically. * Let tags be retrieved from MusicBrainz -> all tags, including year, are correctly displayed in s-j. * Rip CD * Check ripped files with EasyTag or Banshee -> the year tag is not set ProblemType: Bug DistroRelease: Ubuntu 13.04 Package: sound-juicer 3.5.0-0ubuntu1 ProcVersionSignature: Ubuntu 3.8.0-1.5-generic 3.8.0-rc4 Uname: Linux 3.8.0-1-generic x86_64 NonfreeKernelModules: wl nvidia ApportVersion: 2.8-0ubuntu2 Architecture: amd64 Date: Tue Jan 29 20:50:25 2013 EcryptfsInUse: Yes InstallationDate: Installed on 2012-09-15 (136 days ago) InstallationMedia: Ubuntu 12.04.1 LTS "Precise Pangolin" - Release amd64 (20120823.1) MarkForUpload: True SourcePackage: sound-juicer UpgradeStatus: Upgraded to raring on 2013-01-14 (15 days ago)
Thanks, melchiaros, for filing the bug here, I (original reporter) have subscribed myself.
Actually it's not just flac files that are affected. I just tested that mp3s are affected as well (didn't test other types, but would assume it's the same)
Created attachment 273779 [details] [review] Use GST_TAG_DATE_TIME to set 'year' tag GStreamer tagging elements only consider the GST_TAG_DATE_TIME and don't do anything with the GST_TAG_DATE tag sound-juicer was trying to set. This fixes
Created attachment 273780 [details] [review] Store release date as GstDateTime The big advantage over GDate/GDateTime is that it's possible to indicate that the month/day field is not set, which happens in metadata from musicbrainz.
Created attachment 273781 [details] [review] Remove unused helper With the switch to GstDateTime, we use gst_date_time_new_from_iso8601_string() and the sj_metadata_helper_scan_date() helper is no longer needed.
Review of attachment 273779 [details] [review]: This one doesn't compile, as it's replaced by the next patch in the series perhaps we should just squash it into that. ::: libjuicer/sj-extractor.c @@ +608,3 @@ gst_tag_setter_add_tags (tagger, GST_TAG_MERGE_APPEND, + GST_TAG_DATE_TIME, year, Where does year come from? @@ +611,2 @@ NULL); + gst_date_time_unref(year); I think you mean date
Review of attachment 273780 [details] [review]: Look fine to me apart from the null check, the date is set in flac files but not in mp3 files for some reason. ::: libjuicer/sj-metadata-musicbrainz5.c @@ +656,2 @@ GET (date, mb5_release_get_date, release); + album->release_date = gst_date_time_new_from_iso8601_string (date); We need to check if date is NULL here as that used to be done in sj_metadata_helper_scan_date. At the moment I get GStreamer-CRITICAL **: gst_date_time_new_from_iso8601_string: assertion 'string != NULL' failed if there isn't a date in MusicBrainz
Review of attachment 273779 [details] [review]: I'd prefer to keep 2 separate commits, one with the DATE -> DATE_TIME bugfix, the other with the mechanical GDate->GstDateTime change. ::: libjuicer/sj-extractor.c @@ +611,2 @@ NULL); + gst_date_time_unref(year); Ah my bad, rebase screw up.
Created attachment 273969 [details] [review] Use GST_TAG_DATE_TIME to set 'year' tag GStreamer tagging elements only consider the GST_TAG_DATE_TIME and don't do anything with the GST_TAG_DATE tag sound-juicer was trying to set. This fixes
Created attachment 273970 [details] [review] Store release date as GstDateTime The big advantage over GDate/GDateTime is that it's possible to indicate that the month/day field is not set, which happens in metadata from musicbrainz.
Created attachment 273971 [details] [review] Remove unused helper With the switch to GstDateTime, we use gst_date_time_new_from_iso8601_string() and the sj_metadata_helper_scan_date() helper is no longer needed.
Review of attachment 273780 [details] [review]: Ah, did not test mp3, I should look at that. ::: libjuicer/sj-metadata-musicbrainz5.c @@ +656,2 @@ GET (date, mb5_release_get_date, release); + album->release_date = gst_date_time_new_from_iso8601_string (date); Yup, thanks, fixed in v2
Review of attachment 273780 [details] [review]: ::: src/sj-main.c @@ +825,3 @@ + } + + g_warn_if_reached (); I don't think it's an error for two albums to have the same year and no month or day set, shouldn't we just return the comparison after comparing the days?
(In reply to comment #7) > the date is set in flac files but not in mp3 files for some reason. > If I change the tagger to id3mux from id3v2mux then the date is set
(In reply to comment #12) > Review of attachment 273780 [details] [review]: > > Ah, did not test mp3, I should look at that. It looks like id3v2mux is using GST_TAG_DATE rather than GST_TAG_DATE_TIME, although if that's so I don't know why it was broken for Mario in comment 2. Maybe we should set both GST_TAG_DATE and GST_TAG_DATE_TIME? > ::: libjuicer/sj-metadata-musicbrainz5.c > @@ +656,2 @@ > GET (date, mb5_release_get_date, release); > + album->release_date = gst_date_time_new_from_iso8601_string (date); > > Yup, thanks, fixed in v2 I've not look at the new version properly, did you change anything else?
(In reply to comment #15) > (In reply to comment #12) > > Review of attachment 273780 [details] [review] [details]: > > > > Ah, did not test mp3, I should look at that. > It looks like id3v2mux is using GST_TAG_DATE rather than GST_TAG_DATE_TIME, > although if that's so I don't know why it was broken for Mario in comment 2. > Maybe we should set both GST_TAG_DATE and GST_TAG_DATE_TIME? Most likely... :-/ I'll try to fix the gstreamer elements as well to use GST_TAG_DATE_TIME. > > > ::: libjuicer/sj-metadata-musicbrainz5.c > > @@ +656,2 @@ > > GET (date, mb5_release_get_date, release); > > + album->release_date = gst_date_time_new_from_iso8601_string (date); > > > > Yup, thanks, fixed in v2 > I've not look at the new version properly, did you change anything else? I've fixed the first patch so that it actually compiles :)
Hi, this is Mario. Thanks for looking into this. As for the mp3 question, what I can say is that I can still reproduce that year tag is not being written for me for mp3 files either, on Ubuntu 14.04 development version, Trusty Tahr, with sound-juicer version 3.5.0-0ubuntu1.
(In reply to comment #17) > Hi, this is Mario. Thanks for looking into this. As for the mp3 question, what > I can say is that I can still reproduce that year tag is not being written for > me for mp3 files either, on Ubuntu 14.04 development version, Trusty Tahr, > with sound-juicer version 3.5.0-0ubuntu1. Hi Mario, thanks for confirming that. Version 3.5 uses encodebin for mp3 files, I wonder if that uses id3mux rather than id3v2mux to write the tag which would explain why the year is missing.
Created attachment 274520 [details] [review] Use GST_TAG_DATE_TIME to set 'year' tag GStreamer tagging elements only consider the GST_TAG_DATE_TIME and don't do anything with the GST_TAG_DATE tag sound-juicer was trying to set. This fixes
Created attachment 274521 [details] [review] Store release date as GstDateTime The big advantage over GDate/GDateTime is that it's possible to indicate that the month/day field is not set, which happens in metadata from musicbrainz.
Created attachment 274522 [details] [review] Remove unused helper With the switch to GstDateTime, we use gst_date_time_new_from_iso8601_string() and the sj_metadata_helper_scan_date() helper is no longer needed.
These new patches set both GST_TAG_DATE_TIME and GST_TAG_DATE. I've tested that this sets the year tag with MP3, FLAC, Ogg/Vorbis and Ogg/Opus.
Review of attachment 274520 [details] [review]: Looks good, I think setting both is the safest option for now
Review of attachment 274521 [details] [review]: I've not tested it but it looks fine to me. Some of the new function calls in sj-extractor.c are missing a space between the function and the arguments.
Review of attachment 273780 [details] [review]: ::: src/sj-main.c @@ +825,3 @@ + } + + g_warn_if_reached (); I had forgotten to address that, this is now fixed.
Review of attachment 274521 [details] [review]: Ah thanks, I have fixed the missing spaces, hopefully I did not miss some.