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

Simulation of skipping ads #214

Closed
wants to merge 5 commits into from

Conversation

thanasistrisp
Copy link
Contributor

@thanasistrisp thanasistrisp commented Apr 16, 2023

After issue #197, I tried to implement skipping ads by going forward to 29.9th second (it is constant always) using playerctl. It seems to work well.
However, there are some issues that should be fixed (if @Mastermindzh could help):

  • The command is based on shell using fork technique. This should use MPRIS API (else playerctl should be installed explicitly to system) but I do not know exactly how to call it: I do not see the position command to API.
  • Playerctl command should be called with --player option. If I try to run through shell the command playerctl --player=tidal-hifi position 29.9, I get the error Could not execute command: Could not get track id to set position. There something goes wrong. Track id is not set at all: it does nothing yet.
  • For better user experience, it would be better to also mute music these milliseconds of skipping as sometimes (not always) at the beginning, the ad maybe heard for a little (~ <1 second).
  • Hide upgrade banner and other annoying connection messages. Network issues are kept untouched.

Copy link
Owner

@Mastermindzh Mastermindzh left a comment

Choose a reason for hiding this comment

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

Please put it behind a setting and extract the function 😄

src/preload.js Outdated Show resolved Hide resolved
@Mastermindzh
Copy link
Owner

Playerctl is not an issue i.m.o, mpris doesn't offer the control you need IIRC.
I don't know the answer to the second answer, I'm not sure I understand it yet either. Will have to look more closely but I don't have much time.

- add new button in settings with requirement mpris service
- remove mutedArtists as not needed anymore
- split adblock code to new function
- add muted functionality to new function
- reduce interval time for song changes to 400ms from 1000ms
@thanasistrisp
Copy link
Contributor Author

thanasistrisp commented Apr 18, 2023

Playerctl is not an issue i.m.o, mpris doesn't offer the control you need IIRC. I don't know the answer to the second answer, I'm not sure I understand it yet either. Will have to look more closely but I don't have much time.

Yes, I do not disagree. The problem is not the command line option, but the issue that playerctl command is called without --player=tidal-hifi option, which could cause issues if another player supported by mpris service is played simultaneously.
To make it work, I need to set "mpris:trackid" option, but I do not fully understand what exactly is this here. If you have time, take a look. That's fixed and also fixed other issues like the ability to use more playerctl functions like shuffle, repeat (but not yet position and absolute volume, not just mute). However, position now needs to move control to playerctl tidal-hifi player and I do not know if it is possible by using div elements or some other technique.

@thanasistrisp
Copy link
Contributor Author

Also, I removed muted Artists option as it is not needed anymore. It just adds workload to the loop without reason, and it causes conflicts with ad blocking. Now it is ok, less than one second, to skip the ad. I also set the interval time to 400ms so that options get updated more quickly.

@thanasistrisp
Copy link
Contributor Author

Ready for merge for now. Sometime later, we will have to look for playerctl player option I said above to have more precise results.

do it after load and at preinjection as it had problems, like tidal server errors
@Mastermindzh
Copy link
Owner

@thanasistrisp,

I am attending Kubecon right now so this merge will have to wait until next week.
My first impressions of your changes are pretty good but I want to test & evaluate properly when I am not bogged down by Kubecon.

Thanks for the changes!

@thanasistrisp
Copy link
Contributor Author

Closed for real ad blocking in #218.

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.

2 participants