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

Store the ReplayGain tag values as strings #864

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

robUx4
Copy link
Contributor

@robUx4 robUx4 commented Oct 24, 2024

This is how it's stored in Vorbis [1][2] and how
mkvmerge passes the value from one to the other [3]. There doesn't seem to be any player reading it from Matroska.

[1] https://code.videolan.org/videolan/vlc/-/blob/f1a90394435471e75fee6f13383079a3d0272ec4/modules/demux/xiph_metadata.c#L472
[2] https://github.com/FFmpeg/FFmpeg/blob/153a6dc8faafc4de263a493484ffc1dc2b5b26b2/libavformat/replaygain.c
[3] https://gitlab.com/mbunkus/mkvtoolnix/-/blob/7a32d31e7029b1ca4f323eb0f7fb975ec979b089/src/common/tags/vorbis.cpp#L78

I would have preferred a binary version of a float value to avoid parsing errors. But for backward compatibility it's probably better to keep it like this. Unless mkvmerge ends up turning Vorbis replay gain into a binary format ? I don't know the code enough to tell for sure, cc @mbunkus.

@robUx4 robUx4 added bug spec_tags Tags Matroska spec document target labels Oct 24, 2024
@robUx4 robUx4 requested a review from mbunkus October 24, 2024 09:26
@robUx4
Copy link
Contributor Author

robUx4 commented Oct 24, 2024

That leaves MCDI as the only (official) binary tag. It may change with #831 but we may just use strings as for ReplayGain.

@robUx4
Copy link
Contributor Author

robUx4 commented Oct 24, 2024

I tried muxing a .ogg file with REPLAYGAIN_GAIN and REPLAYGAIN_PEAK and mkvmerge did turn them into string (or kept them as-is).

image

@robUx4
Copy link
Contributor Author

robUx4 commented Oct 24, 2024

But the "dB" unit is there in the gain and the peak level is not in dB.

@robUx4
Copy link
Contributor Author

robUx4 commented Oct 24, 2024

The original files has those strings:

replaygain_album_true_peak=1.117522
replaygain_track_true_peak=1.117522
replaygain_album_gain=-7.31 dB
replaygain_album_peak=1.072099
replaygain_track_gain=-7.31 dB
replaygain_track_peak=1.072099

@robUx4 robUx4 force-pushed the replaygain-strings branch 3 times, most recently from 1c9b932 to 5d1791c Compare October 24, 2024 13:11
@robUx4 robUx4 force-pushed the replaygain-strings branch from 5d1791c to b508626 Compare October 24, 2024 13:12
@robUx4
Copy link
Contributor Author

robUx4 commented Oct 24, 2024

BTW, given it influences the playback (it should, in an ideal world) I think it could be elements of the TrackEntry\Audio. We would need to put the "album" and "track" values. But if the file is a whole album with tracks split as chapters, we can't define individual tracks from there...

@robUx4
Copy link
Contributor Author

robUx4 commented Oct 25, 2024

Merging this as this matches what exists in the wild.

@robUx4 robUx4 merged commit e4c8f7e into ietf-wg-cellar:master Oct 25, 2024
2 checks passed
@robUx4 robUx4 deleted the replaygain-strings branch October 25, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug spec_tags Tags Matroska spec document target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant