Created attachment 274764 [details] [review]
don't use Gstreamer to tag a ripped (MP3) file, use taglib# instead

This patch changes how a ripped file gets its tags: don't use the Gstreamer pipeline, use taglib# after the ripping is finished.

data/audio-profiles/mp3-lame.xml.in:
* remove the id3v2mux/id3mux elements from the "cd-import" pipeline

src/Backends/Banshee.GStreamer/Banshee.GStreamer/AudioCdRipper.cs:
* pass an empty TagList to br_rip_track()

src/Backends/Banshee.GStreamerSharp/Banshee.GStreamerSharp/AudioCdRipper.cs:
* remove all tag processing (which was ported from br_rip_track())

src/Extensions/Banshee.OpticalDisc/Banshee.OpticalDisc.AudioCd/AudioCdRipper.cs:
* in OnTrackFinished: instead of merging the file's tags (which now don't
  exist anymore) into the TrackInfo, use StreamTagger.SaveToFile() to
  write the TrackInfo into the file's tags (note that this will also update
  track.FileSize and track.FileModifiedStamp)

Cheers, Roderich

Created attachment 274803 [details] [review]
Proposed patch

Since we are iterating over all available elements having a ITagSetter interface, tags set on previous iterations would be overriden because TagMergeMode.ReplaceAll was specified.
This patch fixes this issue in both the managed and native backend by using TagMergeMode.Replace instead of ReplaceAll.

Comment on attachment 274764 [details] [review]
don't use Gstreamer to tag a ripped (MP3) file, use taglib# instead

Hey Roderich, given that this patch is lacking some tags as you say in the mailing-list (https://mail.gnome.org/archives/banshee-list/2014-April/msg00080.html), I'm going to mark it as needs-work for now, thanks for your work!

As you can see, Stephan seems to have found the culprit of the original issue that the user reported in the mailing list. Do you mind testing it? If it works, we should apply his patch first (and backport it to stable branch), and then we can go on investigating the best way to switch to taglib-sharp for the master branch.

Thanks!

> As you can see, Stephan seems to have found the culprit of the original issue
that the user reported in the mailing list.

Huh? IMHO, the original issue was that Banshee writes ID3 v2.4 tags into tracks
it has ripped. The reason is that Banshee doesn't add these tags itself, but
the Gstreamer ripping pipeline does it (and it unconditionally writes v2.4 tags).
When Banshee writes tags by itself, it uses taglib# which produces ID3 v2.3.

So we want to prevent Gstreamer from writing tags at all. Stephan's patch might
be correct, but it's beside the point. If we remove the id3*mux element from
the pipeline, then Gstreamer will not write any tags. Hence we could
remove all tagsetting code from ripping.
However, this may only be true when ripping to MP3, e.g. the AAC pipeline
using faac has a muxer at the end that also is a TagSetter.

Sorry for bringing up the "missing" tags - it's actually a red herring:

- TXXX[MusicBrainz DiscID] and TXXX[CDDB DiscID] are actually inserted
  into the ripping pipeline by Gstreamer itself; but they are also pretty
  irrelevant (who cares for the MusicBrainz or CDDB "hash" computed from
  the physical CD that was used to rip the track?). Note that 
  "MusicBrainz DiscID" is *not* the MusicBrainz unique ID for the "release" 
  (in MusicBrainz terms) AKA "album" (in layman's terms). The latter is
  definitely written to the MP3 file with my patch 
  (as TXXX[MusicBrainz Album Id]).

- TSSE ("software used for encoding"): it's nice to have "Banshee <version>" 
  here, but it's also pretty irrelevant, too (and AFAICT there's no way 
  to make taglib# write it)

Cheers, Roderich

Banshee is not under active development anymore and had its last code changes more than three years ago. Its codebase has been archived.

Closing this report as WONTFIX as part of Bugzilla Housekeeping to reflect
reality. Please feel free to reopen this ticket (or rather transfer the project
to GNOME Gitlab, as GNOME Bugzilla is being shut down) if anyone takes the
responsibility for active development again.
See https://gitlab.gnome.org/Infrastructure/Infrastructure/issues/264 for more info.