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

Fix latency stream #1845

Closed
wants to merge 25 commits into from
Closed

Fix latency stream #1845

wants to merge 25 commits into from

Conversation

@lukh
Copy link
Contributor

lukh commented Nov 17, 2019

providing a fix for big latency when reading streams from web.
the fix comes from here:
https://discourse.mopidy.com/t/streaming-radio-station-pausing-buffering/1251/19

@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 17, 2019

Codecov Report

Merging #1845 into develop will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1845      +/-   ##
===========================================
+ Coverage     82.2%   82.22%   +0.01%     
===========================================
  Files           77       77              
  Lines         6334     6339       +5     
===========================================
+ Hits          5207     5212       +5     
  Misses        1127     1127
Impacted Files Coverage Δ
mopidy/audio/actor.py 59.72% <100%> (+0.46%) ⬆️

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 400e0ea...884b489. Read the comment docs.

@kingosticks

This comment has been minimized.

Copy link
Member

kingosticks commented Nov 17, 2019

This change affects all audio we get over http, including stuff we want buffered properly.

@lukh

This comment has been minimized.

Copy link
Contributor Author

lukh commented Nov 18, 2019

hello,
Can you please help me understand the situation here ?
Is there any better solution ?
For me, having a 5sec latency when streaming is a problem,
and I don't see the side effects of this PR ?
Is there any solution ? as a fix/add to mopidy or as a local fix for specific projects ?
Thank for your help,
Bests,

@adamcik

This comment has been minimized.

Copy link
Member

adamcik commented Nov 27, 2019

This fix would work great for things that are actually streaming, e.g. things you can't seek in. But say you have backend that is http based but lets you stream with HTTP ranges than you're breaking them with this.

As such we need a way for backends to give a hint to audio system about if this is live or not. Currently we have two main ideas for this:

  1. Creating a new Ref type stream - which backends would use for live content like radio streams
  2. Changing
    def change_track(self, track):
    (or something thereabouts) and the audio API to let Backend Playback providers indicate that live should be used, or to trigger the download flag in GStreamer.

Basically this needs to be something the backends opt in to, not something on by default. Does this make more sense?

@adamcik

This comment has been minimized.

Copy link
Member

adamcik commented Nov 27, 2019

#1608 is essentially the "same" problem, just a different GStreamer flag.

@lukh

This comment has been minimized.

Copy link
Contributor Author

lukh commented Nov 28, 2019

Ok thank you for your answer ! I get it now (at least, I think so...)
I added a new flag in Track, live_stream, that is set or not by the backend.

So Lib providers are free to set "live_stream", and the PlaybackProvider.change_track has the info (via track object) and passes it to the audio actor.

The audio actor has a flag as well, and I kept the implementation but with the flag check. (maybe I should use gstreamer flags but I don"t see how download flag work with streams ??) (from #1608)

I didn't get how to use the Ref objects, because there is no way to recover the Ref object from change track ?

@lukh

This comment has been minimized.

Copy link
Contributor Author

lukh commented Dec 4, 2019

Hello, @adamcik, What do you think of this implementation ? Is that what you meant ?

Would you consider merging ? And would you consider merging for the 3.0.0 release if we reach the quality required ?

Thank you !

Copy link
Member

adamcik left a comment

Indeed this is along the lines of what I was thinking, main thing perhaps @jodal or @kingosticks could also chime in on is naming and having the state in the model vs asking the playback provider? Even though these are probably bikesheds.

I can't think of any strong reasons for one over the other, so if nobody else has objections I would have no problem with going with this as is.

mopidy/audio/actor.py Outdated Show resolved Hide resolved
mopidy/backend.py Outdated Show resolved Hide resolved
mopidy/models/__init__.py Outdated Show resolved Hide resolved
@adamcik
adamcik approved these changes Dec 5, 2019
mopidy/audio/actor.py Show resolved Hide resolved
mopidy/backend.py Outdated Show resolved Hide resolved
lukh added 5 commits Dec 6, 2019
@kingosticks

This comment has been minimized.

Copy link
Member

kingosticks commented Dec 7, 2019

We don't want to test with real streams. The tests should run quickly. Is there not a way to mock what you want?

@lukh

This comment has been minimized.

Copy link
Contributor Author

lukh commented Dec 8, 2019

Yeah it looks a bad idea. I wanted to test the new section in _on_source_setup, inside
if self._live_stream and hasattr(source.props, "is_live"):
It is why the codecov decreased I think: if i use the uris given in base test, the is_live property is false, therefor, I can't test this part.
I am afraid I don't know how to mock up this part, because the _on_source_setup is called by Gst 's playbin, on a signal after a set_uri.
I will try to make a test for _on_source_setup only, but i think it should start with set_uri.

@lukh

This comment has been minimized.

Copy link
Contributor Author

lukh commented Dec 8, 2019

This is all I can think of for now, what do you think ?

@jodal
jodal approved these changes Dec 8, 2019
Copy link
Member

jodal left a comment

Looks good to me. Feel free to add a changelog entry in the audio section of v3.0.0 in docs/changelog.rst.

mopidy/backend.py Outdated Show resolved Hide resolved
mopidy/audio/actor.py Outdated Show resolved Hide resolved
mopidy/backend.py Outdated Show resolved Hide resolved
mopidy/backend.py Outdated Show resolved Hide resolved
tests/audio/test_actor.py Outdated Show resolved Hide resolved
@jodal jodal added this to the v3.0 milestone Dec 8, 2019
lukh and others added 8 commits Dec 9, 2019
Co-Authored-By: Stein Magnus Jodal <stein.magnus@jodal.no>
Co-Authored-By: Stein Magnus Jodal <stein.magnus@jodal.no>
Co-Authored-By: Stein Magnus Jodal <stein.magnus@jodal.no>
Co-Authored-By: Stein Magnus Jodal <stein.magnus@jodal.no>
Co-Authored-By: Stein Magnus Jodal <stein.magnus@jodal.no>
@jodal jodal closed this in d3c1824 Dec 9, 2019
@jodal

This comment has been minimized.

Copy link
Member

jodal commented Dec 9, 2019

Thanks! I've squashed and merged this in commit d3c1824.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.