[kyungrak]

Hello,

I have a question for DaveTBlake if he doesn't mind. I'm still struggling to implement replaygain for dsf files.

In the patch I've submitted, Krypton: add replaygain to dsf files #12049, you've pointed out that I should not make TagLoaderTaglib dependant on FFmpeg tag loader. So here's my issue: the only call to FFmpeg tag loader when encountering a dsf file lies in MusicInfoTagLoaderFactory. But from my tests, it seems TagLoaderTaglib is called instead of the FFmpeg tag loader when reading a DSF file. Is that normal behaviour or am I missing something? Shouldn't MusicInfoTagLoaderFactory points to the correct tag loader while scanning and reading music files? (for scanning no issue, FFmpeg tag loader is correctly used)

Taglib hasn't implemented dsf files yet, so I can't call Taglib library to treat dsf files within TagLoaderTaglib like for instance Flac files. Hence my workaround to call FFmpeg tag loader within TagLoaderTaglib.

[DaveBlake]

Happy to talk here about Kodi development, even more happy to have someone working on the code and submitting PRs

Since Taglib hasn't implemented dsf files then it sounds like TagLoaderTaglib should not be being called, the FFmpeg tag loader should be. But I will need to trace through what happend before I can comment further. Most of my work has been with mp3 or FLAC files using Taglib, so I am only vaguely aware of FFmpeg tag loading. It sounds that the factory approach may have already become a little knotted, as can so easily happen with multiple contributors over time on an undocumented design!

Let's see if we can unknot it as well as get replay gain working for dsf.

[kyungrak]

Thanks for your reply, and for all your efforts to the music section. I will try to investigate on my side too, but I'm no developper so you will probably have results before I do. It's nice to learn though (quite the hardway nonetheless).

[kyungrak]

There is no call to MusicInfoTagLoaderFactory in paplayer (VideoPlayerCodec.cpp), only to TagLoaderTaglib:

// Extract ReplayGain info
// tagLoaderTagLib.Load will try to determine tag type by file extension, so set fallback by contentType
std:tring strFallbackFileExtension = "";
if (m_strContentType == "audio/aacp" ||
m_strContentType == "audio/aac")
strFallbackFileExtension = "m4a";
else if (m_strContentType == "audio/x-ms-wma")
strFallbackFileExtension = "wma";
else if (m_strContentType == "audio/x-ape" ||
m_strContentType == "audio/ape")
strFallbackFileExtension = "ape";
CTagLoaderTagLib tagLoaderTagLib;
tagLoaderTagLib.Load(strFile, m_tag, strFallbackFileExtension);

[ironic_monkey]

the problem is that idiot software keeps littering dsf files with id3 tags. ffmpeg does the aiff tags, but lots of shit does not use those and append the id3 blobs instead. taglib is much better at those, than the basic ffmpeg tag class i did, so it's not entirely obvious which way to go about this. as evidenced by your call to taglib on a dsf files - the replay gain info sits in a id3 tag :/

[DaveBlake]

Unfortunately the only way to learn about the current kodi design is the hard way, so please do dig some more, that is what I do. I don't have a solution in mind, my comments on your PR were more because the interdependancy seemed wrong, not because I know how it should be done.

What I see is that while CMusicInfoScanner uses the factory to choose how to scan tags to show on screen or load into the library etc., thus using FFmpeg for dfs, the audio/video player does not. When VideoPlayerCodec::Init wants to get replay gain at the moment it only uses TagLib, and probably needs to use a factory approach to get the tag data for this.

Also think about why the player scans the metadata at this point just for rgain. It has the other tag data such as artists, genre, etc. already in the CFileItem it is playing. Sometimes this includes gain, sometimes not. I think I have figured out why, but better for you to look at it yourself.

[DaveBlake]

the problem is that idiot software keeps littering dsf files with id3 tags. ffmpeg does the aiff tags, but lots of shit does not use those and append the id3 blobs instead. taglib is much better at those, than the basic ffmpeg tag class i did, so it's not entirely obvious which way to go about this. as evidenced by your call to taglib on a dsf files - the replay gain info sits in a id3 tag :/

Surely we just need to pick a utility to deal with metadata in dsf files and stick to it? His PR got the replay gain tag using FFmpeg, it didn't use TagLib to read the metadata as far as I can see.

[ironic_monkey]

sorry i completely misread the diff. somehow i read taglib in there.

but still, what i say is/was a real issue. your updates to the ffmpeg loader, makes the pain much less than it used to be, as it will
expose all tag data i believe, so probably sticking to that for dsf is preferrable.

what confuses me here is the need to read out rg in the codec. it should not be necessary and smells like an old workaround for a bug. it originated in 43e1077dadd0da0d5954768f9ece926c559d8371
as in the data should be attached to the passed item already.

[kyungrak]

I can confirm that. The metadata is read even without the call to FFmpeg, surely it is read from the DB. But since replaygain is not stored in the DB, without the call to ffmpeg it is never picked up.

Edit: oops, I was responding to DaveBlake comment.

[DaveBlake]

I can confirm that. The metadata is read even without the call to FFmpeg, surely it is read from the DB. But since replaygain is not stored in the DB, without the call to ffmpeg it is never picked up.

Yeap.
If the item is not in the db then the backgrond loader does fetch the tag data including replay gain, but when it is in the library if just populates the item CMusicUnfoTag from the db . This does not hold replay gain.

what confuses me here is the need to read out rg in the codec. it should not be necessary and smells like an old workaround for a bug. it originated in 43e1077dadd0da0d5954768f9ece926c559d8371
as in the data should be attached to the passed item already.

Not if it is a library item. Not my design, but that is the reason for what it does as far as I can tell.

[DaveBlake]

There is also the issue of cuesheets, both separate and embedded (for FLAC). The route to the item impacts what rgain has been read, hence the player checks again. One of the areas that makes my head hurt!

When stopping Cuesheet processing attempts (even when there are none) from slowing the song node 7% (completed work stuck in my repo, waiting for prior PRs to be reviewed/merged), I considered holding replay gain in the db, but decided against it.

[kyungrak]

One good side on keeping replaygain in the db would be to be able to check quickly which files have or don't have replaygain in your library.

But I'm not sure there are many people using replaygain.

[ironic_monkey]

right. but it still doesn't make sense that it sits in the codec. then it should sit in the decoder frontend.

[DaveBlake]

One good side on keeping replaygain in the db would be to be able to check quickly which files have or don't have replaygain in your library.

But is that ever likely useful as a smart playlist criteria? Only use I can think of is as a "what files did I forget to tag with gain?" check, and that is not really a media player task. There are so many other things to add that are wanted as filter criteria to aid navigation of large music collections and improve music choices, that replay gain as data seems to me just clutter.

right. but it still doesn't make sense that it sits in the codec. then it should sit in the decoder frontend.

I agree it doesn't make sense, then again I know nothing about the AE architecture so not in a rush to meddle.

[ironic_monkey]

actually this is completely outside AE. AudioDecoder.cpp in cores/paplayer is a frontend between the codec and the player. it takes care of buffers and such, and it makes sense to move this code there so it doesn't only apply with the one decoder. sure, it covers most things, but there are decoder add-ons and such..