Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: store tracklist as tag comment on recording #13761

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

acolombier
Copy link
Member

@acolombier acolombier commented Oct 13, 2024

Closes #12855

Implemented for MP3 and Wave and tested with both.

ffprobe example with MP3

$ ffprobe ../2024-10-13_20h38m19s.mp3 
ffprobe [...]
   [...]
[mp3 @ 0x556661ba7b80] Skipping 339 bytes of junk at 813.
Input #0, mp3, from '/home/antoine/Music/Mixxx/Recordings/2024-10-13_20h38m19s.mp3':
  Metadata:
    encoder         : LAME 64bits version 3.100 (http://lame.sf.net)
    title           : Foo
    artist          : Bar
    album           : Baz
    comment         : 00:00:00: Twenty One Pilots - The Craving (single version)
                    : 00:00:09: Carnage - Psy Or Die
                    : 00:00:19: bbno$ - it boy
                    : 00:00:38: Armin van Buuren feat. Trevor Guthrie - This Is What It Feels Like
                    : 00:00:47: Armin van Buuren - Larger Than Life
                    : 00:01:06: Freestylers - Cracks - Flux Pavilion Remix
  Duration: 00:01:17.14, start: 0.023021, bitrate: 128 kb/s
  Stream #0:0: Audio: mp3, 48000 Hz, stereo, fltp, 128 kb/s
    Metadata:
      encoder         : LAME3.100

Note on MP3

After a long day of learning MP3 spec, it would appears that our implementation was somewhat flawed. My understanding is that the Xing frame that contains various parameter about the MP3 frames is expect in the beginning of the file, however we currently overwrite the first frames with it, leading to data loss. Since a frame is < 100ms, this is not something you can easily hear.

Second point is, we currently only write IDv1 (automatically supported by lame) but this limits the comment to 30 bytes. In order to lift this limit (and reach the hard limit of 2**32), we need to write ID3v2, but those are expected as header (unlike ID3v1 which can be appended in the footer when flushing the file.

In an ideal world, we would want to shift the file content to fit the ID3 header + the Xing frame. Because this basically requires to tear down the whole EncoderCallback mecanism, this instead adds a static header of 10K, which will be filled with the header, leading to a hard limit for comment (limit which depends of the artist, title and album size, as well the Xing frame size.

Appreciate this is far from optimal, but hopefully we can find an acceptable trade-off

@acolombier acolombier force-pushed the feat/store-tracklist-as-comment branch 2 times, most recently from 64079ce to 5108a46 Compare October 13, 2024 19:40
@acolombier acolombier force-pushed the feat/store-tracklist-as-comment branch from 5108a46 to acc29fb Compare October 13, 2024 19:45
@acolombier acolombier marked this pull request as ready for review October 13, 2024 22:04
Comment on lines +159 to +182
void Encoder::addToTracklist(const QString& artist,
const QString& title,
std::chrono::seconds timecode) {
auto recordedDuration = timecode.count();
m_trackList.append(
QStringLiteral("%1: %2 - %3")
.arg(QString("%1:%2:%3")
.arg(recordedDuration / (60 * 60),
2,
'f',
0,
'0') // hours
.arg((recordedDuration / 60) % 60,
2,
'f',
0,
'0') // minutes
.arg(recordedDuration % 60, 2, 'f', 0, '0'),
artist.trimmed().isEmpty()
? QObject::tr("(Unknown Artist)")
: artist,
title.trimmed().isEmpty()
? QObject::tr("(Unknown Title)")
: title));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already have logic for generating a tracklist in cuesheet format. IMO it would make more sense to simply write that in the comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cuesheet have a specific, non-human friendly format.

The point of this formatting is to generate something like

00:11:22: An artist - A title

Since the formatting logic is meant to be unrelated (a change to CUE shouldn't impact metadata and vice versa), I thought it was alright to have a little bit of code duplication (that is for the timecode only)

@Swiftb0y
Copy link
Member

considering the complications and tradeoffs there are with writing large metadata, why not write this tracklist as a separate file like the cuesheet? Is there that much value to having it part of the file metadata?

@acolombier
Copy link
Member Author

acolombier commented Oct 14, 2024

considering the complications and tradeoffs there are with writing large metadata, why not write this tracklist as a separate file like the cuesheet? Is there that much value to having it part of the file metadata?

Because that's not the feature 😃

We already support the ability to generate a cue file, but there was a requested feature to have the ability to store the track. I think that's a fair request as the cue file isn't embedded.
I sometime copy recording of set I have made on my phone, and having a way to check the tracklist as comment metadata would be a great value

@Swiftb0y
Copy link
Member

fully agree, I just find it weird that its a different format from the cuesheet, so I figured these were two different features.

@Holzhaus
Copy link
Member

Holzhaus commented Oct 14, 2024

Didn't check the code, but I was wondering: Is this only for MP3/ID3?

For FLAC I think there is the CUESHEET tag block which is specifically for this purpose and might have proper support by audio players.

@Holzhaus
Copy link
Member

@acolombier
Copy link
Member Author

acolombier commented Oct 14, 2024

I was wondering: Is this only for MP3/ID3?

Yes - only for MP3/ID3 and Wave so far. Since the FLAC encoder inherits the Wave one, it would also support the COMMENT field to be filled up.

https://www.xiph.org/flac/format.html#metadata_block_cuesheet

Nice, I'm wondering how widely is this supported? It feels like some custom logic that could be added in the recordermanager to duplicate the sheets in the FLAC file before flushing. It sounds a bit hacky, but I don't think other format would support cuesheet within their tag's spec?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Recording] Add tracklist to MP3 comments field
3 participants