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

WIP: Stop sniffing mimetype on playlists and don't use playlist.once anymore #48

Closed

Conversation

hairmare
Copy link
Member

@hairmare hairmare commented May 1, 2020

This replaces the mime type sniffing that we are doing when loading a playlist with a hardcoded mime type. We can always assume that the generated m3u files have a text/plain mime type. With this assumption liquidsoap won't log Wrong mime type application/octet-stream for playlist! if the file is empty.

We need to switch to not using playlist.once anymore since it is deprecated nowadays: savonet/liquidsoap#1015. I think we don't need to use reload_mode="once" but can stick to reload_mode=watch since the prio file is handled in a way that watching it leads to the behaviour we want.

  • deactivate mime type sniffing
  • refactor so we don't use playlist.once anymore
  • test if this works with all the liquidsoap versions we are supporting (currently 1.3.2 with the possibilty to upgrade to 1.3.7 which has already been built, 1.4 support depends on us rolling out CentOS 8)

@spameier
Copy link
Member

spameier commented May 1, 2020

Maybe we should use application/x-mpegURL.
I just noticed the following lines in the logs:

2020/05/01 17:05:43 [classics:3] Playlist treated as format application/x-mpegURL

Not entirely sure though. 😕

@hairmare hairmare force-pushed the fix/assume-text-mime-type branch from 300210a to 82e42dd Compare May 1, 2020 20:53
@hairmare
Copy link
Member Author

hairmare commented May 1, 2020

Maybe we should use application/x-mpegURL.

I changed it to audio/x-mpegurl after referencing the docs.

@hairmare hairmare marked this pull request as draft June 3, 2020 07:16
@hairmare hairmare marked this pull request as ready for review June 3, 2020 16:32
@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2020

Codecov Report

Merging #48 into master will decrease coverage by 0.88%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
- Coverage   78.90%   78.01%   -0.89%     
==========================================
  Files           1        1              
  Lines         602      605       +3     
  Branches      141      141              
==========================================
- Hits          475      472       -3     
- Misses        110      114       +4     
- Partials       17       19       +2     
Impacted Files Coverage Δ
klangbecken.py 78.01% <0.00%> (-0.89%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b82a16...4925c9b. Read the comment docs.

Copy link
Member

@smlz smlz left a comment

Choose a reason for hiding this comment

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

What is the reason for removing .once for the prio playlist?

The rest looks good.

@@ -110,24 +110,24 @@ def check_next_func(r) =
end

# Priority playlist for 'play next' feature
prio = playlist.once(id="prio", reload_mode="watch", path.concat(DATA_DIR, "prio.m3u"))
Copy link
Member

Choose a reason for hiding this comment

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

You should leave the .once, to double ensure that the prio playlist never loops.

Copy link
Member Author

@hairmare hairmare Aug 21, 2020

Choose a reason for hiding this comment

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

playlist.once is deprecated in new versions of liquidsoap and it is recommended to replace it with reload_mode=once. Since we are already using reload_mode=watch and the prio.m3u file gets changed when it is used anyway I think that keeping the reload_mode as is makes sense. See my initial comment for a link to the liquidsoap PR that deprecates playlist.once.

Copy link
Member

Choose a reason for hiding this comment

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

My bad. Thanks for the hint.

I think making sure that we never ever end up in a endless loop would be wise, especially when I'm looking at my hacky prio playlist code. I would suggest, that we add two more options to the playlist call: mode="normal" and loop=false. Better be safe than sorry.

This might also allow us to implement a priority queue with more than one entry in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added mode=normal but didn't add loop=false since loop only seems to be implemented in dev according to the docs.

@smlz
Copy link
Member

smlz commented Jan 13, 2021

I think we can close this one. Right?

@hairmare
Copy link
Member Author

hairmare commented Apr 5, 2021

I think we can close this one. Right?

At a glance, I don't think so... IMO we should still circumvent mimetype sniffing and get rid of playlist.once.

@smlz
Copy link
Member

smlz commented Apr 13, 2021

playlist.once is gone in the new code.

As far as mime type sniffing goes, I tried this out some time ago with strange results. Manually setting the mime type makes check_next always fail, and thus sends the player into an endless loop. 😰

@hairmare hairmare closed this Apr 13, 2021
@hairmare hairmare deleted the fix/assume-text-mime-type branch April 13, 2021 20:46
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.

None yet

4 participants