Salastil/ncmpcpp
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix crash when background-fetching lyrics for streams (#561)

Browse Source 
When `fetch_lyrics_for_current_song_in_background` is set to `yes`, and
an HTTP(S) stream is loaded with a long URL, ncmpcpp crashes due to the
length of the filename generated to store lyrics. This commit makes the
background lyrics fetcher ignore such streams, preventing the crash.
Addresses issue #380.
This commit is contained in:
Alex Balgavy
2024-07-24 17:57:38 +02:00
committed by GitHub
parent e87c104dd6
commit f76dd085b2
2 changed files with 28 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
src/screens/lyrics.cpp
View File

@@ -361,6 +361,25 @@ void Lyrics::toggleFetcher()
Statusbar::print("Using all lyrics fetchers"); Statusbar::print("Using all lyrics fetchers");
} }
/* For HTTP(S) streams, fetchInBackground makes ncmpcpp crash: the lyrics_file
* gets set to the stream URL, which is too long, hence
* boost::filesystem::exists() fails.
* Possible solutions:
* - truncate the URL and use that as a filename. Problem: resulting filename
* might not be unique.
* - generate filenames in a different way for streams. Problem: what is a good
* method for this? Perhaps hashing -- but then the lyrics filenames are not
* informative.
* - skip fetching lyrics for streams with URLs that are too long. Problem:
* this leads to inconsistent behavior since lyrics will be fetched for some
* streams but not others.
* - avoid fetching lyrics for streams altogether.
*
* We choose the last solution, and ignore streams when fetching lyrics. This
* is because fetching lyrics for a stream may not be accurate (streams do not
* always provide the necessary metadata to look up lyrics reliably).
* Furthermore, fetching lyrics for streams is not necessarily desirable.
*/
void Lyrics::fetchInBackground(const MPD::Song &s, bool notify_) void Lyrics::fetchInBackground(const MPD::Song &s, bool notify_)
{ {
auto consumer_impl = [this] { auto consumer_impl = [this] {
@@ -377,7 +396,10 @@ void Lyrics::fetchInBackground(const MPD::Song &s, bool notify_)
break; break;
} }
lyrics_file = lyricsFilename(consumer->songs.front().song()); lyrics_file = lyricsFilename(consumer->songs.front().song());
if (!boost::filesystem::exists(lyrics_file))
// For long filenames (e.g. http(s) stream URLs), boost::filesystem::exists() fails.
// This if condition is fine, because evaluation order is guaranteed.
if (!consumer->songs.front().song().isStream() && !boost::filesystem::exists(lyrics_file))
{ {
cs = consumer->songs.front(); cs = consumer->songs.front();
if (cs.notify()) if (cs.notify())
@@ -389,7 +411,7 @@ void Lyrics::fetchInBackground(const MPD::Song &s, bool notify_)
} }
consumer->songs.pop(); consumer->songs.pop();
} }
if (!cs.song().empty()) if (!cs.song().empty() && !cs.song().isStream())
{ {
auto lyrics = downloadLyrics(cs.song(), nullptr, nullptr, m_fetcher); auto lyrics = downloadLyrics(cs.song(), nullptr, nullptr, m_fetcher);
if (lyrics) if (lyrics)

6
src/song.cpp
View File

@@ -293,7 +293,9 @@ bool Song::isFromDatabase() const
bool Song::isStream() const bool Song::isStream() const
{ {
assert(m_song); assert(m_song);
return !strncmp(mpd_song_get_uri(m_song.get()), "http://", 7); const char *song_uri = mpd_song_get_uri(m_song.get());
// Stream schemas: http, https
return !strncmp(song_uri, "http://", 7) || !strncmp(song_uri, "https://", 8);
} }
bool Song::empty() const bool Song::empty() const
@@ -308,7 +310,7 @@ std::string Song::ShowTime(unsigned length)
int minutes = length/60; int minutes = length/60;
length -= minutes*60; length -= minutes*60;
int seconds = length; int seconds = length;
std::string result; std::string result;
if (hours > 0) if (hours > 0)
result = (boost::format("%d:%02d:%02d") % hours % minutes % seconds).str(); result = (boost::format("%d:%02d:%02d") % hours % minutes % seconds).str();