mahkoh / spotblock

A minimal spotify ad blocker
GNU General Public License v3.0
35 stars 1 forks source link

spotblock mutes music videos #7

Closed donbex closed 5 years ago

donbex commented 5 years ago

Some playlists, e.g. this one include music videos. However, spotblock erroneously detects them as ads and mutes them. This is probably because they have an empty artist metadata field, e.g.

$ playerctl metadata
spotify mpris:trackid             spotify:episode:61u1uwI2nJehLtEJlvEMX9
spotify mpris:length              227750000
spotify mpris:artUrl              
spotify xesam:album               Hot Country: The Performances
spotify xesam:albumArtist         
spotify xesam:artist              
spotify xesam:autoRating          0.0
spotify xesam:discNumber          0
spotify xesam:title               Morgan Wallen - Whiskey Glasses (Live)
spotify xesam:trackNumber         0
spotify xesam:url                 https://open.spotify.com/episode/61u1uwI2nJehLtEJlvEMX9
mgavin commented 5 years ago

I'm just having a quick look-see through the code, and the only place I see that determines something as an ad is: https://github.com/mahkoh/spotblock/blob/9a46e40434470d3cef2e16577baebab91c2b0d3e/src/daemon.cc#L218 looking for "spotify:ad" in mpris:trackid ;

I tried stopping spotblock, and got the same effect as the muted video. I opened up the volume controls and saw a chromium process running that was muted. After unmuting it, I got sound.

Your issue may be related to: https://github.com/mahkoh/spotblock#spotblock-mutes-chromium Instead of spotblock targetting the music video as an advertisement.

donbex commented 5 years ago

That's the same I saw when I looked through the code myself. I thought I might have been missing a piece of logic in a different function.

You're probably right about the nature of the issue, but then I don't know how to fix it. I am running spotify through the wrapper script from the readme, and indeed pactl list sink-inputs shows that the property is set correctly. Also, separate instances of Chromium are unmuted, unlike what happens when running spotify without the wrapper script.

However, the readme also says

spotblock mutes some spotify ads which run inside of chromium processes

so doesn't this mean that the issue is somewhat related to the ad-detection logic?

mgavin commented 5 years ago

Perhaps. Just to verify, you've stopped the spotblock service, restarted spotify, and played the videos with success?

Maybe it has to do with pulseaudio, dbus, spotify, chromium... That's just off the top of my head. More investigation would be needed into debugging this issue. I would like to see if after playing a video, spotblock sent a dbus message to mute the stream, but I'm not sure how to get dbus logs.

May investigate later... for now ¯\(°_o)/¯

donbex commented 5 years ago

So, it took me a while, but I found the culprit: spotblock checks that it's dealing with the right sink-input by looking at its name. However, the videos play through a sink-input called "Playback" instead of "Spotify. Here's a (very) quick and dirty patch:

diff --git a/src/daemon.cc b/src/daemon.cc
index ac7b434..c426341 100644
--- a/src/daemon.cc
+++ b/src/daemon.cc
@@ -185,7 +185,7 @@ void Daemon::set_stream_muted(uint32_t idx, bool muted) {
 }

 static bool is_real_spotify_stream(const char *c) {
-    return c && strcmp(c, "Spotify") == 0;
+    return c && (strcmp(c, "Spotify") == 0 || strcmp(c, "Playback") == 0);
 }

 void Daemon::handle_new_spotify_stream(const pa_sink_input_info *i) {

However, I think it would be better to check against a different sink-input property. Here are the property lists of both sink-inputs, minus application.process.user, application.process.host, and _application.process.machineid:

media.role = "music"
media.name = "Spotify"
application.name = "Spotify"
native-protocol.peer = "UNIX socket client"
native-protocol.version = "32"
application.process.id = "5504"
application.process.binary = "spotify"
window.x11.display = ":0"
application.language = "en_US.UTF-8"
application.process.session_id = "1"
application.icon_name = "spotify-client"
module-stream-restore.id = "sink-input-by-media-role:music"
application.icon_name = "chromium-browser"
media.name = "Playback"
application.name = "Chromium"
native-protocol.peer = "UNIX socket client"
native-protocol.version = "32"
module-stream-restore.id = "spotify"
application.process.id = "7423"
application.process.binary = "spotify"
window.x11.display = ":0"
application.language = "en_US.UTF-8"
application.process.session_id = "1"

All in all, I think it would be best to check against application.process.binary, since here module-stream-restore.id is the same only because I am using the wrapper script (otherwise its value would be "sink-input-by-application-name:Chromium"). Incidentally, I think this might also solve the muting issue mentioned in the readme without resorting to a wrapper script.

donbex commented 5 years ago

I don't really know any C++ and this is probably quite ugly, style-wise, but here's another patch to check against check against application.process.binary. After a quick test, it seems to work as it should.

diff --git a/src/daemon.cc b/src/daemon.cc
index ac7b434..b274471 100644
--- a/src/daemon.cc
+++ b/src/daemon.cc
@@ -184,12 +184,14 @@ void Daemon::set_stream_muted(uint32_t idx, bool muted) {
     this->pa_ctx.set_sink_input_mute(idx, muted, nullptr, nullptr);
 }

-static bool is_real_spotify_stream(const char *c) {
-    return c && strcmp(c, "Spotify") == 0;
+static bool is_real_spotify_stream(const pa_sink_input_info *i) {
+    const char *prop = "application.process.binary";
+    return pa_proplist_contains(i->proplist, prop) &&
+           strcmp(pa_proplist_gets(i->proplist, prop), "spotify") == 0;
 }

 void Daemon::handle_new_spotify_stream(const pa_sink_input_info *i) {
-    if (!is_real_spotify_stream(i->name)) {
+    if (!is_real_spotify_stream(i)) {
         if (!i->mute) {
             this->set_stream_muted(i->index, true);
         }

I can send a pull request if you'd like, but I thought it best to submit it for review first.

mgavin commented 5 years ago

My only thought is with conforming to the norm in the source to check the character string specifically instead of sending the properties, but I'm sure it doesn't matter. Just a preference for function functionality.

In addition, to check dbus messages coming from spotblock, you can use dbus-monitor "sender=org.spotblock"

Nice work!

mahkoh commented 5 years ago

Some playlists, e.g. this one include music videos.

It doesn't play any videos for me. I've only seen videos on the mobile client. I'm using 1.1.5.153.gf614956d.

The reason for blocking these streams unconditionally is that it's not possible to detect video ads.

donbex commented 5 years ago

It doesn't play any videos for me. I've only seen videos on the mobile client. I'm using 1.1.5.153.gf614956d.

It plays videos for me, at least those in the playlist I linked. The audio is output by a separate Chromium process, with its own PulseAudio sink-input.

The reason for blocking these streams unconditionally is that it's not possible to detect video ads.

Can you give me an example of ad that isn't detected otherwise? All the ads that I've seen so far which played audio had been properly identified with "spotify:ad" in their mpris:trackid, even those which opened a video "pop-up" within spotify.

mahkoh commented 5 years ago

Let's see if this works.