DeaDBeeF-Player / deadbeef

DeaDBeeF Player
https://deadbeef.sourceforge.io/
Other
1.65k stars 178 forks source link

Album/track artist choice priority #1026

Closed Oleksiy-Yakovenko closed 9 years ago

Oleksiy-Yakovenko commented 9 years ago

Original issue 1124 created by Alexey-Yakovenko on 2014-05-30T18:51:12.000Z:

What steps will reproduce the problem?

  1. load a track with album artist, but no track artist

What is the expected output? What do you see instead? Expected: album artist evaluated to display album art Instead: track artist not found, no album art displayed

What version of the product are you using? On what operating system and CPU architecture? x86_64, arch linux, deadbeef-static_devel-1_x86_64 from yesterday.

How did you install the product? Gotten for the drone.io autobuilder, extracted folder

Please provide any additional information below.

Oleksiy-Yakovenko commented 9 years ago

Comment #1 originally posted by Alexey-Yakovenko on 2014-05-30T19:25:22.000Z:

for tracking purpose:

this needs fixing in 2 places:

  1. plugins/gtkui/plcommon.c only looks for "artist", while should look for all related filds, perhaps using %track_artist% from the new title formatting system
  2. ffmpeg plugin needs to map "Album artist" to "album artist" (case is important), otherwise %track_artist% won't work correctly.
Oleksiy-Yakovenko commented 9 years ago

Comment #2 originally posted by Alexey-Yakovenko on 2014-05-30T19:26:07.000Z:

entry point: get_cover_art_callb in plcommon.c

Oleksiy-Yakovenko commented 9 years ago

Comment #3 originally posted by Alexey-Yakovenko on 2014-05-30T22:12:24.000Z:

%B already searches through "band", "album artist", "albumartist", then "artist" and is not case sensitive. No good?

BAND is a standard ID3V2 tag. It is also commonly used in Flac and Ogg although the recommended tag is ENSEMBLE.

The track properties dialog doesn't recognise any of the four tags other than artist. Should all of them be mapped to a standard metadata key internally and displayed consistently?

Oleksiy-Yakovenko commented 9 years ago

Comment #4 originally posted by Alexey-Yakovenko on 2014-05-30T22:48:15.000Z:

Track artist field should only read the artist tag, I think.

If %B already displays track artist if no album artist is found, then I guess my second example is already working. Conditional display could be a separate wish, I guess.

Am I right to assume the title formatting syntax is being changed to eventually match the one of foobar2000?

Oleksiy-Yakovenko commented 9 years ago

Comment #5 originally posted by Alexey-Yakovenko on 2014-05-31T09:43:02.000Z:

yes

Oleksiy-Yakovenko commented 9 years ago

Comment #6 originally posted by Alexey-Yakovenko on 2014-06-23T21:11:49.000Z:

I'm working on Flac to make cflac_read_metadata work with VFS URIs. The mapping of "ARTIST", "BAND", "ALBUM ARTIST", and the special cuesheet "ARTIST[i]" tags is very inconsistent right now, different for regular inserts, inserts from a Flac CUESHEET metadata block, or a cuesheet tag buffer. The main theme is an attempt to map "ALBUM ARTIST" to the band metadata key, but not "ENSEMBLE" or "ALBUMARTIST". This seems a little pointless, since I don't think anywhere that uses band wouldn't use album artist metadata instead.

What is the preferred mapping? Leave everything "as is" and let the callers prioritise which tag they want to use? Playlist does, for shuffle and title formatting. The artwork plugin doesn't, or at least, the callers don't do anything except grab the artist metadata. I'd like to make Flac use the same oggedit_map_tag that Opus and Vorbis use, but want to make sure it is suitable first.

One consequence of mapping tags is that when they get written back to the file, they are re-mapped. If multiple tags are mapped to the same metadata key then the tag name gets changed for no good reason. Or maybe for a good reason, if an obsolete tag gets updated to a newer name. In the case of the "BAND" tag this means that it gets written back out as an "ALBUM ARTIST" tag. Not really noticeable in DeaDBeef, but could cause unexpected behaviour of the track is then opened in another program.

Also, should metadata keys be getting mapped to lower case? Generally, or just album artist? This is for the new title formatting, right? Is it doing away with the idea that tags and metadata keys are case insensitive?

Oleksiy-Yakovenko commented 9 years ago

Comment #7 originally posted by Alexey-Yakovenko on 2014-06-25T07:36:51.000Z:

@Ian, to many questions to answer in bugtracker.

In general, I prefer to NOT change anything mapping related in 0.6.x.

the 0.7 will get entirely new title formatting (fb2k-like), the one which can bee seen on "tf" branch, and this will most likely require some mapping changes.

This specific bug needs to be fixed in the GTKUI scope, that is, let the caller prioritize.

Oleksiy-Yakovenko commented 9 years ago

Comment #8 originally posted by Alexey-Yakovenko on 2014-06-25T07:39:17.000Z:

Oh, btw, sorry for adding VFS support to cflac_read_metadata while you were working on it. I tend to postpone reading long comments / emails, so please send some kind of short messages like "i'm working on flac meta from vfs right now" to email -- that will make sure i don't put them on backlog.

Oleksiy-Yakovenko commented 9 years ago

Comment #9 originally posted by Alexey-Yakovenko on 2014-06-25T14:21:41.000Z:

That's OK. I've been looking into Flac tagging but hadn't coded anything for this bug. Not a huge amount of work anyway. I've built with the new patch to run tests.

I'll leave tag mapping alone until tf lands. I'd like to have Flac, Vorbis, and Opus all using oggedit_map_tag, but it can wait.

I'm still concerned about the Flac metadata blocks at the end of the file. I can't make this happen. In your description you mentioned that the new block is created at the end of the chain. This isn't the end of the file, but simply the last metadata block, which is still before the audio frames. Usually it doesn't even end up as the last metadata block because padding comes after it. Have you had actual problems reading the metadata after re-creating a missing metadata block?

For Deadbeef, cflac_write_metadata doesn't work at all at the moment although no errors are reported because the error check after FLACmetadata_chain_write is inverted (since my oggedit patch). FLACmetadata_chain_write can't be called on a chain read with callbacks. Probably the VFS callbacks aren't needed here at all since the file must be real and local for us to be able to write it since there is no VFS wrote function.

Oleksiy-Yakovenko commented 9 years ago

Comment #10 originally posted by Alexey-Yakovenko on 2014-06-25T14:26:39.000Z:

Indeed, it is the last metadata block. So maybe it's not the end of file. I didn't check that. But decoder callbacks are not getting that block.

And yes, I noticed yesterday that flac tagging is broken, but didn't have time to look at it. Will fix later.

Oleksiy-Yakovenko commented 9 years ago

Comment #11 originally posted by Alexey-Yakovenko on 2014-06-25T14:43:27.000Z:

just fixed both the tag writing, and the error reporting.

Oleksiy-Yakovenko commented 9 years ago

Comment #12 originally posted by Alexey-Yakovenko on 2014-06-25T19:16:03.000Z:

Yes, writing metadata works again on trunk. Adding a metadata block in Deadbeef where there wasn't one adds it as the last metadata block, but still before the audio. It isn't legal for the metadata blocks to be after the audio (in Ogg Flac it isn't legal to have the VORBIS_COMMENT block missing at all). I don't know what was causing the problems, but probably not metadata blocks being at the end of the file.

Oleksiy-Yakovenko commented 9 years ago

Comment #13 originally posted by Alexey-Yakovenko on 2014-06-25T19:22:25.000Z:

I don't know what was causing the problems

  1. create (or find) a flac file without VORBISCOMMENT metadata block at all
  2. add VORBISCOMMENT ḃlock to it using metaflac
  3. open the file in deadbeef (version from few days ago)
  4. notice the VORBISCOMMENT is not being read

this is what the fix is for.

Oleksiy-Yakovenko commented 9 years ago

Comment #14 originally posted by Alexey-Yakovenko on 2014-06-25T20:18:53.000Z:

Can't reproduce with the previous 6 commits to flac.c, or in the fixed commit with the fix commented out. Did you add the VORBIS_COMMENT block just by setting a tag (--set-tag=), or in some other way? Did you include the underscore, because VORBISCOMMENT is not a valid block type.

Same when writing a new metadata block from Deadbeef.

Oleksiy-Yakovenko commented 9 years ago

Comment #15 originally posted by Alexey-Yakovenko on 2014-06-25T20:22:20.000Z:

I did it on a specific FLAC file, which was crashing on attempt to write tags. If you want to have it for test purposes - come to IRC, i'll /DCC SEND it to you.

Oleksiy-Yakovenko commented 9 years ago

This seems to be solved with the new title formatting