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

Full replay gain support for track metadata, file tags and library #766

Merged
merged 21 commits into from Nov 7, 2015
Merged

Full replay gain support for track metadata, file tags and library #766

merged 21 commits into from Nov 7, 2015

Conversation

uklotzde
Copy link
Contributor

Sorry, I couldn't resist ;) Another PR for master.

This PR completes the replay gain support for track metadata (including file tags) and the library (database) by properly reading/writing the peak amplitude of a track.

The peak amplitude is currently not computed by the RG1 analyser, but could be added easily. The RG2 analyser seems to perform this analysis on the fly.

// 1.0/2.0 specification.
// http://wiki.hydrogenaud.io/index.php?title=ReplayGain_1.0_specification
// http://wiki.hydrogenaud.io/index.php?title=ReplayGain_2.0_specification
static CSAMPLE parsePeak(QString strPeak, bool* pValid = 0);
Copy link
Member

Choose a reason for hiding this comment

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

An additional hint that strPeak = "1.0" represents the digital full scale could be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also thought about adding a static assert, because we implicitly assume
here that CSAMPLE_PEAK = 1.0

On 11/03/2015 08:15 AM, Daniel Schürmann wrote:

In src/util/replaygain.h
#766 (comment):

@@ -56,6 +69,18 @@ class ReplayGain {
m_peak = CSAMPLE_PEAK;
}

An additional hint that strPeak = "1.0" represents the digital full scale
could be useful.


Reply to this email directly or view it on GitHub
https://github.com/mixxxdj/mixxx/pull/766/files#r43721107.

@daschuer
Copy link
Member

daschuer commented Nov 3, 2015

Thank you for the PR. I have only commented some naming nits.

@uklotzde
Copy link
Contributor Author

uklotzde commented Nov 4, 2015

Added column (read-only) for replay gain in table views.

@daschuer
Copy link
Member

daschuer commented Nov 6, 2015

This PR is growing. When will it be ready for merge?
I have already tested and reviewed it until 10b31ed and I can say LGTM up to that.
So if you mind, you can do separate PR to merge this now.

I will now read though the remaining commits.

@uklotzde
Copy link
Contributor Author

uklotzde commented Nov 6, 2015

I don't plan any further additions.

The GUI extensions are just the icing on the cake that I need to test your
RG2 / EBU R 128 stuff ;)

On 11/06/2015 10:43 PM, Daniel Schürmann wrote:

This PR is growing. When will it be ready for merge?
I have already tested and reviewed it until 10b31ed
10b31ed
and I can say LGTM up to that.
So if you mind, you can do separate PR to merge this now.

I will now read though the remaining commits.


Reply to this email directly or view it on GitHub
#766 (comment).

@daschuer
Copy link
Member

daschuer commented Nov 6, 2015

I don't know why, but now there is something broken.
The track info dialog is empty for every track.
The history does not track files.
This also happens in current master... strange.

@uklotzde
Copy link
Contributor Author

uklotzde commented Nov 6, 2015

I've tested it only with a fresh Mixxx library/config. I will test again
with an existing Mixxx library/config.

On 11/06/2015 11:25 PM, Daniel Schürmann wrote:

I don't know why, but now there is something broken.
The track info dialog is empty for every track.
The history does not track files.
This also happens in current master... strange.


Reply to this email directly or view it on GitHub
#766 (comment).

@uklotzde
Copy link
Contributor Author

uklotzde commented Nov 7, 2015

My tests with both an empty and an existing Mixxx config were successful.
Also the history view works as expected.

Did you try a clean rebuild? Sometimes SCons does not reliably switch
between different branches.

@daschuer
Copy link
Member

daschuer commented Nov 7, 2015

Strange! It seems I had messed up my working directory. Now the issue is like that:
Plain recent master: no issue
Merged with this branch scons -c and rm -R lin64_build: no track info content.
I will now debug why.

@daschuer
Copy link
Member

daschuer commented Nov 7, 2015

My error: [ no such column: replaygain_peak ]

The library predict that is was already at 25, but that was wrong. After changing current the schema version back to 24, it works as desired.

LGTM

daschuer added a commit that referenced this pull request Nov 7, 2015
Full replay gain support for track metadata, file tags and library
@daschuer daschuer merged commit 5ded6a8 into mixxxdj:master Nov 7, 2015
@uklotzde
Copy link
Contributor Author

uklotzde commented Nov 7, 2015

Great! Thank you for reviewing and merging, Daniel.

You can now rebase your PR "Replaygain 2.0" on master for a fresh start. Since no one has actually reviewed the code this is safer and clearer than merging.

@uklotzde uklotzde deleted the ReplayGainPeak branch November 7, 2015 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants