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

Proposal to increase A/V demux queues slightly #17646

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dagwieers
Copy link
Contributor

Description

This proposal is based on a change already implemented by @davilla for MrMC which also fixes known issues with specific HLS streams.

Whereas @davilla's changes for MrMC go a lot further than we need to fix the known issues increasing the SetMaxTimeSize of Audio and Video demux queues from a value of 8 seconds to 10 seconds is sufficient. @peak3d proposed 12 seconds for both.

This PR is the same changes as PR #17608 but against the master branch.

Motivation and Context

While these streams have issues, we are in no position to fix those streams. Other tools (ffplay, THEOplayer, Shakaplayer, OMXplayer) behave perfectly fine on these streams as well and with this small fix the streams behave well in Kodi as well.

For Matrix we are looking into a more extensive fix in the player, but while we do not expect large changes in the VideoPlayer in Leia to be accepted and we like to see this fixed in the next release of Leia we are proposing this here.

Relates to:

How Has This Been Tested?

This has been tested by various people over the past 2 years which have suffered from the existing streaming issues and has proven to overcome those.

And this is the default setMaxTimeSize setting for MrMC.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

This proposal is based on a change already implemented by @davilla for
MrMC which also fixes known issues with specific HLS streams.

Whereas @davilla's changes for MrMC go a lot further than we need to fix
the known issues increasing the SetMaxTimeSize of Audio *and* Video
demux queues from a value of 8 seconds to 10 seconds is sufficient.
@peak3d proposed 12 seconds for both.

While these streams have issues, we are in no position to fix those
streams. Other tools behave perfectly fine on these streams as well and
with this small fix the streams behave well in Kodi as well.

For Matrix we are looking into a more extensive fix in the player, but
while we do not expect large changes in the VideoPlayer in Leia to be
accepted and we like to see this fixed in the next release of Leia we
are proposing this here.

This PR to the master branch was required for backporting to Leia.
@dagwieers dagwieers changed the title Proposal to increase A/V emux queues slightly Proposal to increase A/V demux queues slightly Apr 9, 2020
@lrusak lrusak added this to the Matrix 19.0-alpha 1 milestone Apr 9, 2020
@FernetMenta
Copy link
Contributor

If you don't adjust calculations based on the buffer times, this will change behaviour, i.e. longer channel switching times for live tv.

@dagwieers
Copy link
Contributor Author

@FernetMenta The original value was acceptable 10 years ago. I would expect people's internet bandwidth and latency has only improved over that period, a multiple of the 25% increase we propose here.

@a1rwulf
Copy link
Member

a1rwulf commented Apr 10, 2020

@dagwieers I think what @FernetMenta wants to tell us, is that if we change those buffersizes we also need to change other stuff that's based on them being this size.

@FernetMenta
Copy link
Contributor

You always have to consider all use cases, not just your own. Receiving a realtime stream and having the buffer filled, i.e. 50% has nothing to do with bandwidth. If you increase the buffer by 25%, you have to wait 25% longer until the buffer is 50% full.

@dagwieers
Copy link
Contributor Author

dagwieers commented Apr 10, 2020

@FernetMenta If you are filling buffers when streaming, the time to fill your buffer is directly related to bandwidth and latency (as well as the size of the buffer).

If 10 years ago an 8-seconds buffer was good enough, I would expect a 10-seconds buffer today is filled faster than an 8-seconds buffer 10 years ago. I am sure your bandwidth and latency improved by more than 20% in the past 10 years.

Update: So yes, I didn't get the "live stream" part of the discussion :-)

@popcornmix
Copy link
Member

Not for a real-time (live) stream. To fill 10 seconds of buffer will take 10 seconds regardless of network bandwidth.

@a1rwulf
Copy link
Member

a1rwulf commented Apr 10, 2020

So we could fill 40% of a 10s buffer instead of 50% of a 8s buffer, right?

@FernetMenta
Copy link
Contributor

in order not to change behavior you need to change all calculations to absolute time rather than percentage

@davilla
Copy link
Contributor

davilla commented Apr 11, 2020

There is also the buffering rate limit clamp to deal with. We removed it long ago and fill buffers as fast as the network can handle. We would rather burst fill than fill too slow and it removes yet another dependency quirk that might or might not be causing issues.

@phunkyfish
Copy link
Contributor

@FernetMenta whereabouts should the calculations based on the buffer times be changed?

@DaveTBlake
Copy link
Member

Would prefer not to just bump this though milestones, but unclear on state.
As a "fix" it is still welcome in v19, but op says "For Matrix we are looking into a more extensive fix in the player,... we like to see this fixed in the next release of Leia we are proposing this here." so change was only intended as a workaround in v18 that never made it.

Then there is also the opaque comment from @FernetMenta

in order not to change behavior you need to change all calculations to absolute time rather than percentage

Not expecting answers by tomorrow so going to bump that milestone one more time, but can those that know about the player please clarify what needs to be done or if this should be closed.

@DaveTBlake DaveTBlake removed this from the Matrix 19.0-beta 1 milestone Nov 17, 2020
@DaveTBlake DaveTBlake added this to the Matrix 19.0-beta 2 milestone Nov 17, 2020
@keithah keithah removed the v19 Matrix label Dec 5, 2020
@keithah keithah removed this from the Matrix 19.0-beta 2 milestone Dec 5, 2020
@da-anda
Copy link
Member

da-anda commented Jun 8, 2022

@dagwieers is this PR still valid?

@sschamp
Copy link

sschamp commented Jun 17, 2022

I had to bump the values to 12.0
My Kodi build has been running for well over a year now and runs flawlessly with that value on an Nvidia Shield.

@phunkyfish
Copy link
Contributor

I had to bump the values to 12.0
My Kodi build has been running for well over a year now and runs flawlessly with that value on an Nvidia Shield.

Thanks for the info. Unless we can figure out where we need to change the calculations to absolute time instead of percentage then I don’t see how this PR could progress.

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Feb 4, 2023
@jenkins4kodi
Copy link
Contributor

@dagwieers this needs a rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Music Component: Video Rebase needed PR that does not apply/merge cleanly to current base branch Type: Fix non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet