Replay gain is sometimes not applied to remote tracks

[SamInPgh]

The problem is that, in the case of streaming services such as Qobuz, track metadata (e.g. Replay gain) is cached for a limited amount of time (30 days for Qobuz) and, if a track whose metadata has been expired from the cache but has been imported into the LMS library is played, that metadata must be refreshed. This is done asynchronously when a call to the Qobuz protocol handler's getMetadataFor() routine is done. The reason that the correct RG seemed to be applied most of the time proved to be very difficult to track down but, thanks to logBackTrace(), it was determined to be due to some inefficient (if not faulty) code in the stream_s() subroutine in Squeezebox.pm that caused Slim::Player::ReplayGain->trackAlbumMatch() to be called once or twice each time a track (of any kind) is streamed in order to check whether smart fading should be applied during the transition to the next track. This was done for every track, even if the player's Crossfade option was set to "None". In the trackAlbumMatch() routine, if both tracks are remote, getMetadataFor() is called for both the current and previoius/next track to see if they are from the same album and if, therefore, fading should or shouldn't be applied. All this is totally unnecessary (and the results ignored) when the player's Crossfade option is "None". Eliminating these unnecessary call(s) to trackAlbumMatch() also eliminates the call(s) to getMetadataFor(), which means that the metadata is also not retrieved there (and expired cache data not refreshed) and so this must now be done in a more straightforward manner, and for ALL tracks. Therefore, an explicit call to getMetadataFor() is added to the _getNextTrack() routine for remote tracks in StreamingController.pm in order to ensure that any metadata required later in the logic path (e.g. Replay gain) will be in the associated plugin's cache.

[michaelherger]

But would getMetadataFor() still be called multiple times? As it's asynchronous there might potentially be no data on first call?

[SamInPgh]

But would getMetadataFor() still be called multiple times? As it's asynchronous there might potentially be no data on first call?

It could indeed be called multiple times. In the case of tracks that have expired from the cache, there will be no metadata returned on the first call. That is the reason for adding the call to the _getNextTrack() routine, which doesn't actually make use of the returned data, if any, but ensures that it will be available later in the logic path if needed --- for instance, when the Qobuz protocol handler's trackGain() routine is called during the _Stream() process, where the track's metadata is retrieved directly from the Qobuz cache. I'm not sure how the plugins for other streaming services that support replay gain handle the case of expired metadata, but theoretically it should help them too.

[SamInPgh]

Bonus! Now that I can be confident that the track's metadata will be available in the cache when the Qobuz plugin's protocol handler trackGain() routine is called, I can implement a true version of Smart Gain there by making use of ReplayGain.pm's trackAlbumMatch() routine, just as is done in Squeezebox.pm to support Smart Fade for remote tracks. I will also make use of the wantarray() Perl function to optimize Qobuz's getMetadataFor() routine, simply making sure that the cached data is present and avoiding the extra overhead of populating the metadata structure if no return argument is passed to it. Fun fact: I'm all about software efficiency, having spent the last fifteen years of my career working on enterprise level performance monitoring and management software. 😏

[michaelherger]

I'm not sure calling getMetadataFor() all the time is optimising things. Those implementations can be quite heavy. Could this call be made conditional to only be executed if replay gain information is really needed?

[SamInPgh]

I'm not sure calling getMetadataFor() all the time is optimising things. Those implementations can be quite heavy. Could this call be made conditional to only be executed if replay gain information is really needed?

I think you might be missing the point. While getMetadataFor() can indeed incur some amount of overhead depending on the specific implementation, it is already called multiple times for a remote song from various places along the code path --- not just when Replay Gain is in use. Don't forget that this PR eliminates the two unnecessary calls to getMetadataFor() that were done for every remote track in the trackAlbumMatch() routine when evaluating Smart Fade, the net result of the change thus being a reduction in the number of these calls. Doing the initial call from the _getNextTrack() routine simply ensures that the metadata will, in fact, be available from the cache for all subsequent calls. Without this change, the first call to getMetadataFor() for a track that has been expired from the cache will return missing or invalid data if it refreshes the cache asynchronously, as it should. I have also provided a way for plugin developers to avoid the overhead of populating the metadata for return to the caller by making this initial call without a returnargument, which they can detect via the wantarray() function and then simply return after ensuring that the cache is present, or initiating an asynchronous refresh if it is not. I believe that this change will bring an overall improvement in both efficiency and consistency, while also preventing errors caused by expired cache data, such as the one that led to its creation. (I apologize for my wordiness. I'm better at speaking conversationally 😏).

[michaelherger]

Ok, let's give this a try. Thanks!

[michaelherger]

I have also provided a way for plugin developers to avoid the overhead of populating the metadata for return to the caller by making this initial call without a returnargument, which they can detect via the wantarray() function and then simply return after ensuring that the cache is present, or initiating an asynchronous refresh if it is not.

I haven't grasped this one yet. How would that work? Where would we ever expect getMetadataFor() to return an array?

[SamInPgh]

I have also provided a way for plugin developers to avoid the overhead of populating the metadata for return to the caller by making this initial call without a returnargument, which they can detect via the wantarray() function and then simply return after ensuring that the cache is present, or initiating an asynchronous refresh if it is not.

I haven't grasped this one yet. How would that work? Where would we ever expect getMetadataFor() to return an array?

We wouldn't. The wantarray() function simply allows the called subroutine to take different action based on the caller's context. I plan to make use of it in the Qobuz plugin's getMetadataFor() routine to refresh the cache and return without incurring the further overhead of populating the metadata. I suspect that there are other plugins that might also be able to benefit from this to some extent, the Qobuz routine being a prime candidate due to its unusually high overhead. Here's a good explanation of the function:

https://perldoc.perl.org/functions/wantarray

[SamInPgh]

Ok, let's give this a try. Thanks!

Thank YOU! 👍

[michaelherger]

We wouldn't. The wantarray() function simply allows the called subroutine to take different action based on the caller's context. I plan to make use of it in the Qobuz plugin's getMetadataFor() routine to refresh the cache and return without incurring the further overhead of populating the metadata. I suspect that there are other plugins that might also be able to benefit from this to some extent, the Qobuz routine being a prime candidate due to its unusually high overhead. Here's a good explanation of the function:

I know wantarray. But I don't understand how this is helping you/us? We never expect an array when calling getMetadataFor(), do we? Therefore that's somewhat useless, isn't it?

[SamInPgh]

I know wantarray. But I don't understand how this is helping you/us? We never expect an array when calling getMetadataFor(), do we? Therefore that's somewhat useless, isn't it?

The function is misnamed, as Perldoc points out:

---

wantarray

Returns true if the context of the currently executing subroutine or eval is looking for a list value. Returns false if the context is looking for a scalar. Returns the undefined value if the context is looking for no value (void context).

This function should have been named wantlist() instead.

---

Here is the line I am adding to getMetadataFor() in the Qobuz plugin (after refreshing the cache, if needed):

return unless defined wantarray;

[michaelherger]

Oh... it's not just falsy, it's also differentiating between defined falsy (0?) and undefined. Wow...