Make playbin buffer-size and buffer-duration configurable in mopidy/audio section #1950

dingo35 · 2020-12-24T10:40:34Z

See forum discussion https://discourse.mopidy.com/t/buffer-size-and-buffer-duration-configurable-where-to-post-my-patch/4591

…re/reduce-latency

codecov · 2021-01-03T14:23:41Z

Codecov Report

Merging #1950 (481daf0) into develop (7037d58) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1950      +/-   ##
===========================================
+ Coverage    76.97%   77.01%   +0.03%     
===========================================
  Files           55       55              
  Lines         4713     4721       +8     
===========================================
+ Hits          3628     3636       +8     
  Misses        1085     1085

Impacted Files	Coverage Δ
mopidy/audio/actor.py	`61.38% <100.00%> (+0.70%)`	⬆️

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 7037d58...481daf0. Read the comment docs.

dingo35 · 2021-01-11T09:08:49Z

Bump.

djmattyg007 · 2021-01-11T11:48:57Z

docs/config.rst

+ A value of 0 seems to work fine, then the minimum buffer size seems to be
+ determined by min_buffer_duration.


I think this sentence is a bit confusing, especially since the default value isn't zero.

docs/config.rst

djmattyg007 · 2021-01-11T11:50:29Z

docs/config.rst

+ Sets the minimum buffer duration of the GStreamer playbin. If you experience
+ buffering/stuttering, it may help to increase this. If you experience latency
+ when changing tracks, lower this value.
+ A value of 0 seems can work fine, although we recommend a minimum value of


You should change this:

A value of 0 seems can work fine

To something like:

A value of 0 can work fine

docs/config.rst

djmattyg007 · 2021-01-11T11:53:13Z

mopidy/config/__init__.py

+_audio_schema["min_buffer_size"] = Integer(optional=True, minimum=0)
+_audio_schema["min_buffer_duration"] = Integer(optional=True, minimum=0)


Given our code doesn't make any attempt to modify these values dynamically, I think it's probably better if the existing convention is followed by removing min_ from the start of the setting names.

No mopidy doesnt modify these values; what is meant is that this buffer is to be filled fully to the value specified, before playing starts. So the larger this value, the larger the latency. The buffer itself might be larger, or not, only GStreamer devs would know, but that is IMHO not relevant here....

I agree these names should change and the min_ prefix removed.

docs/config.rst

kingosticks · 2021-01-11T12:34:59Z

mopidy/config/default.conf

@@ -16,6 +16,8 @@ mixer = software
 mixer_volume =
 output = autoaudiosink
 buffer_time =
+min_buffer_size =


Set the default values here

This is part of https://github.com/mopidy/mopidy/pull/1950/files#r555993368

kingosticks · 2021-01-11T12:35:41Z

mopidy/config/__init__.py

@@ -60,6 +60,8 @@
 _audio_schema["output"] = String()
 _audio_schema["visualizer"] = Deprecated()
 _audio_schema["buffer_time"] = Integer(optional=True, minimum=1)
+_audio_schema["min_buffer_size"] = Integer(optional=True, minimum=0)


Make these new configs non-optional, they don't work the same as buffer_time (which is truly optional because we want to fallback to GST's default).

This is part of https://github.com/mopidy/mopidy/pull/1950/files#r555993368

docs/config.rst

kingosticks · 2021-01-11T13:29:13Z

docs/config.rst

+
+ Expects an integer 0 or higher.
+
+ Sets the minimum buffer size of the GStreamer playbin. If you experience


Do you have a GStreamer documentation reference specifying these properties are minimums? As I understand it, these values directly map onto the max-size-bytes and max-size-time properties of the uridecode's internal queue2 element. These are the maximum amounts the queue will buffer before it blocks the source. FYI, the high/low watermark settings of the underlying queue are not adjustable on a playbin.

Co-authored-by: Nick Steel <[email protected]>

kingosticks · 2021-01-11T14:19:12Z

I've made suggestions to be more explicit about what these configs adjust, rather than hide Gstreamer-specific things which would normally be the correct approach. I am in two minds but I think this makes sense here since adjusting the watermarks is actually what we want to do but that is sadly not possible.

kingosticks · 2021-01-11T14:20:22Z

FYI, you can use the "Add suggestion to batch" button to avoid a bunch of separate commits and wasted CI runtime.

kingosticks · 2021-01-11T19:24:32Z

So if we could just expose these config options as they are and avoid renaming them. If you wanted to suggest any of them be zero I think you would need to clearer about what happens but then you are just documenting parts of GStreamer that even they don't document clearly. I'd still prefer wording that made it clear that fiddling with these configs is fiddling with GST-specific parameters and unless you know what you are doing you should leave them well alone.

And in regards to the original forum post. The fix for removing all latency when switching radio streams is to specify the radio streams to GStreamer as "live" streams. Doing so disables GStreamer's "stream buffering", which is the only solution to avoid the x seconds of clock time it takes to buffer x seconds worth of audio from certain HTTP streams. Unfortunately it's really not obvious when you should set live mode for a random given HTTP URI. We could revisit the idea of setting it for all HTTP streams that appear to be unseekable, but that is a different PR.

djmattyg007 · 2021-01-11T22:02:46Z

@dingo35 to confirm, do these settings affect regular song-based playback, or just playback of livestreams?

kingosticks · 2021-01-11T22:10:57Z

Everything, except Spotify.

dingo35 · 2021-01-12T15:17:28Z

And in regards to the original forum post. The fix for removing all latency when switching radio streams is to specify the radio streams to GStreamer as "live" streams. Doing so disables GStreamer's "stream buffering", which is the only solution to avoid the x seconds of clock time it takes to buffer x seconds worth of audio from certain HTTP streams. Unfortunately it's really not obvious when you should set live mode for a random given HTTP URI. We could revisit the idea of setting it for all HTTP streams that appear to be unseekable, but that is a different PR.

I understand that you would expect that, but I don't think that it's true:

Setting "is_live" (by calling set_live(True)) does not change anything measurable in latency when playing live streams; that is probably the reason that it is always set False and is AFAIK never set to True anywhere in the Mopidy code;
It makes sense that this doesn't change anything in latency. You need buffering because you don't want the playing to be interrupted because data is not there. You have two kinds of data sources, the ones you can read from start to end, either local or remote, which I will call "files", and the ones that are sent out continuously ("non seekable"), which I will call "streams". Note that by that definition streams are always live. I can't think of streams that are not live, but that might be my limitation of imagination.

Now for playing streams you cannot read ahead (as you can with files), so you will always need a bigger buffer (or the same) (as opposed to files.
So if there are any buffers you can disable when playing streams, you could disable those buffers anyway for playing files.

Now I must be wrong, because both the GStreamer docs and the Mopidy comments all say the opposite. Could you please tell me where my thinking is going wrong?

kingosticks · 2021-01-12T17:04:39Z

Setting "is_live" (by calling set_live(True)) does not change anything measurable in latency when playing live streams; that is probably the reason that it is always set False and is AFAIK never set to True anywhere in the Mopidy code;

No. It just doesn't make any sense in the majority of cases. It is up to each backend to decide where it wants to use it. The latest version of Mopidy-Tunein uses live mode for some streams. If you are interested you can install it from pypi with pip (it's not available from APT yet).

dingo35 · 2021-01-12T18:00:38Z

That surely is interesting, because I use the TuneIn plugin; it's hard to measure latency, because tune-in adds latency too, so it is hard to keep conditions comparable. I will try to do some profiling of the code to see whether there is actual improvement of latency.

But as you mentioned earlier, that is OT to this PR. Now I am losing track of all those repositories, I have accepted all your changes, so if you need me to do anything please let me know (but you'll have to spell it out, as much as I like the mopidy code I've come to hate your github/circleci/action/workflow environment....)

docs/config.rst

mopidy/audio/actor.py

tests/audio/test_actor.py

Co-authored-by: Nick Steel <[email protected]>

dingo35 · 2021-01-14T13:21:23Z

Well the "Add suggestion to batch" option is greyed out, so I had to commit all this stuff separately.

Seriously, all this button pressing and patch switching for 15 lines of code? And then a mailbox overflowing with all workflow shit...
"Sometimes a machine is more a handicap than a help."

kingosticks · 2021-01-14T13:59:56Z

I don't know why it's greyed out, I only recently noticed it even existed. I added those "suggestions" in attempt to to make it easier/quicker. An alternative would be to make the changes in your editor and push that new commit to this branch - zero buttons pressed. I guess it's a personal preference what you would prefer. I hope you can appreciate we are all just trying to get stuff done here.

schuenep · 2021-08-15T18:06:33Z

@dingo35 @kingosticks Hi, any updates on this PR? I started only today to use mopidy inside Phoniebox. I too noticed the long delay before starting a live radio stream. It really is not so nice user-experience wise.
Nevertheless I am very thankful for the existence and maintenance of this project :)

dingo35 added 8 commits December 23, 2020 11:29

audio/actor.py: make buffer-size and buffer-duration configurable

ada3b81

audio/actor.py: fix flake8 messages

f4f9471

audio/actor.py: fix black message

26baf18

rename config names to be more consistent with other mopidy config names

3e833f0

actor.py: remove debug message

55eb2c9

test_actor.py: update tests

aa40aea

actor.py: improve comment to kick circleci into gear

08684d1

Merge branch 'develop' of https://github.com/mopidy/mopidy into featu…

824af8b

…re/reduce-latency

djmattyg007 reviewed Jan 11, 2021

View reviewed changes

docs/config.rst Outdated Show resolved Hide resolved

djmattyg007 reviewed Jan 11, 2021

View reviewed changes

docs/config.rst Outdated Show resolved Hide resolved

djmattyg007 reviewed Jan 11, 2021

View reviewed changes

kingosticks requested changes Jan 11, 2021

View reviewed changes

dingo35 and others added 7 commits January 11, 2021 15:14

Update docs/config.rst

0b71553

Co-authored-by: Nick Steel <[email protected]>

Update docs/config.rst

4a28fd0

Co-authored-by: Nick Steel <[email protected]>

Update docs/config.rst

46ef557

Co-authored-by: Nick Steel <[email protected]>

Update docs/config.rst

8a1b4ba

Co-authored-by: Nick Steel <[email protected]>

Update docs/config.rst

a357e26

Co-authored-by: Nick Steel <[email protected]>

Update docs/config.rst

92eefe3

Co-authored-by: Nick Steel <[email protected]>

Update docs/config.rst

481daf0

Co-authored-by: Nick Steel <[email protected]>

kingosticks added A-audio Area: Audio layer C-enhancement Category: A PR with an enhancement or an issue with an enhancement proposal labels Jan 11, 2021

kingosticks added this to the Next feature release milestone Jan 11, 2021

kingosticks reviewed Jan 12, 2021

View reviewed changes

docs/config.rst Outdated Show resolved Hide resolved

kingosticks reviewed Jan 12, 2021

View reviewed changes

docs/config.rst Outdated Show resolved Hide resolved

kingosticks reviewed Jan 12, 2021

View reviewed changes

mopidy/audio/actor.py Outdated Show resolved Hide resolved

kingosticks reviewed Jan 12, 2021

View reviewed changes

tests/audio/test_actor.py Outdated Show resolved Hide resolved

dingo35 and others added 4 commits January 14, 2021 14:19

Update tests/audio/test_actor.py

4cebd6d

Co-authored-by: Nick Steel <[email protected]>

Update docs/config.rst

7735e1b

Co-authored-by: Nick Steel <[email protected]>

Update mopidy/audio/actor.py

9c33886

Co-authored-by: Nick Steel <[email protected]>

Update docs/config.rst

0e0538b

Co-authored-by: Nick Steel <[email protected]>

jodal modified the milestones: v3.4.0, Next feature release Oct 29, 2023

jodal deleted the branch mopidy:main March 1, 2024 23:15

jodal closed this Mar 1, 2024

jodal reopened this Mar 1, 2024

jodal changed the base branch from develop to main March 1, 2024 23:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make playbin buffer-size and buffer-duration configurable in mopidy/audio section #1950

Make playbin buffer-size and buffer-duration configurable in mopidy/audio section #1950

dingo35 commented Dec 24, 2020

codecov bot commented Jan 3, 2021 •

edited

dingo35 commented Jan 11, 2021

djmattyg007 Jan 11, 2021

djmattyg007 Jan 11, 2021

djmattyg007 Jan 11, 2021

dingo35 Jan 11, 2021

kingosticks Jan 12, 2021 •

edited

kingosticks Jan 11, 2021

kingosticks Jan 12, 2021

kingosticks Jan 11, 2021

kingosticks Jan 12, 2021

kingosticks Jan 11, 2021

kingosticks commented Jan 11, 2021

kingosticks commented Jan 11, 2021

kingosticks commented Jan 11, 2021

djmattyg007 commented Jan 11, 2021

kingosticks commented Jan 11, 2021

dingo35 commented Jan 12, 2021

kingosticks commented Jan 12, 2021

dingo35 commented Jan 12, 2021

dingo35 commented Jan 14, 2021 •

edited

kingosticks commented Jan 14, 2021

schuenep commented Aug 15, 2021

		A value of 0 seems to work fine, then the minimum buffer size seems to be
		determined by min_buffer_duration.

		_audio_schema["min_buffer_size"] = Integer(optional=True, minimum=0)
		_audio_schema["min_buffer_duration"] = Integer(optional=True, minimum=0)


		Expects an integer 0 or higher.

		Sets the minimum buffer size of the GStreamer playbin. If you experience

Make playbin buffer-size and buffer-duration configurable in mopidy/audio section #1950

Are you sure you want to change the base?

Make playbin buffer-size and buffer-duration configurable in mopidy/audio section #1950

Conversation

dingo35 commented Dec 24, 2020

codecov bot commented Jan 3, 2021 • edited

Codecov Report

dingo35 commented Jan 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingosticks Jan 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingosticks commented Jan 11, 2021

kingosticks commented Jan 11, 2021

kingosticks commented Jan 11, 2021

djmattyg007 commented Jan 11, 2021

kingosticks commented Jan 11, 2021

dingo35 commented Jan 12, 2021

kingosticks commented Jan 12, 2021

dingo35 commented Jan 12, 2021

dingo35 commented Jan 14, 2021 • edited

kingosticks commented Jan 14, 2021

schuenep commented Aug 15, 2021

codecov bot commented Jan 3, 2021 •

edited

kingosticks Jan 12, 2021 •

edited

dingo35 commented Jan 14, 2021 •

edited