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

Fade & replay gain #40

Closed
philippe44 opened this issue Dec 15, 2017 · 2 comments
Closed

Fade & replay gain #40

philippe44 opened this issue Dec 15, 2017 · 2 comments

Comments

@philippe44
Copy link

philippe44 commented Dec 15, 2017

I was seeing some issues with my modified squeezelite version for AirPlay bridge and fading + replaygain (maybe). I did the same correction you did in commit #724 a while ago, for the same reasons (compiler warning) but never reverted it. It seems to me that the original test is wrong

if (!ctx->output.fade == FADE_ACTIVE || !ctx->output.fade_mode == FADE_CROSSFADE)

This cannot be right. fade is an enum so !fade will never be FADE_ACTIVE (==2). this part of the expression is always false. It's ORed with !fade_mode which will only equal FADE_CROSSFADE (==1) if fade_mode is FADE_NONE. So replay gain is set to new one only when fade_mode it set to NONE, which is not desired for IN, OUT and INOUT. So I don't understand how the original version could work

Isn't what is trying to be done the following?

if (ctx->output.fase == FADE_INACTIVE || ctx->output.fade_mode != FADE_CROSSFADE)

In other words, change reply gain at track start when there is no fading or it is cross_fading? The current track shall be faded out with it's own (old) gain, the new track shall be faded in with its own (new) gain.
But with cross-fading, as said below in the code, the current_gain shall be retained to be applied on the old track piece while new gain is apply on the new track piece and the mix happens

 } else if (ctx->output.fade_mode == FADE_CROSSFADE) {		
      LOG_INFO("[%p]: crossfade complete", ctx);
  if (_buf_used(ctx->outputbuf) >= dur_f * BYTES_PER_FRAME) {
	_buf_inc_readp(ctx->outputbuf, dur_f * BYTES_PER_FRAME);
	LOG_INFO("[%p]: skipped crossfaded start", ctx);
 } else {
	LOG_WARN("[%p]: unable to skip crossfaded start", ctx);
}
ctx->output.fade = FADE_INACTIVE;
ctx->output.current_replay_gain = ctx->output.next_replay_gain;`

Why did you revert to the old test then? I did test with the "corrected test" and fade-in, out and cross work as expected with expected replay gains

@ralph-irving
Copy link
Owner

I reverted the change because the patch I was provided to attempt to fix the compiler warning completely broke crossfade.
After spending some time digesting your excellent description, I agree with your assessment of what the check should be so I've made the following change to output.c and crossfade still seems to be working as expected.

@@ -152,7 +152,7 @@
IF_DSD(
output.dop = output.next_dop;
)

  •                           if (!output.fade == FADE_ACTIVE || !output.fade_mode == FADE_CROSSFADE) {
    
  •                           if (output.fade == FADE_INACTIVE || output.fade_mode != FADE_CROSSFADE) 
    

{

Thank you for taking the time to report your fixes back to me. Change committed at 3028f38

@philippe44
Copy link
Author

philippe44 commented Dec 19, 2017

Thanks :-) What bothered me is that with the existing test, crossfade should not work when there is replaygain. I did explicity test it and the replaygain of the next track was applied to the current track when doing the crossfade, so with a big difference between the current and next, it was very audible. I'm surprised nobody ever complained, hence I was nervous my analysis was wrong. But I did checked the same 2 tracks with a big replay gain difference with old and new test and definitevly saw that new one works, old one does not ...

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

No branches or pull requests

2 participants