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

[url] skip parsing username:password@ for udp/rtp streams #17222

Merged
merged 4 commits into from
Mar 7, 2020

Conversation

phunkyfish
Copy link
Contributor

@phunkyfish phunkyfish commented Jan 19, 2020

Description

Username and password are not valid for UDP/RTP streams. This can cause issues where the URL is in the form rtp://sourceip@multicastip.

Fixes: #17220

Motivation and Context

Fix udp/rtp streams in the format rtp://sourceip@multicastip

How Has This Been Tested?

Requires user testing. I don't have a stream of this type.

Screenshots (if appropriate):

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

@phunkyfish phunkyfish changed the title [url] skip parsing username:passwor@ for udp/rtp streams [url] skip parsing username:password@ for udp/rtp streams Jan 19, 2020
Copy link
Member

@Rechi Rechi left a comment

Choose a reason for hiding this comment

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

Please follow the current code guidelines.

@phunkyfish
Copy link
Contributor Author

Fixed @Rechi

Copy link
Member

@Rechi Rechi left a comment

Choose a reason for hiding this comment

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

Please follow the current code guidelines.

@phunkyfish
Copy link
Contributor Author

It's really fixed now.

@phunkyfish
Copy link
Contributor Author

phunkyfish commented Jan 20, 2020

After some user testing on windows we get an error with what is enabled in the ffmpeg build:

20-01-20 20:30:40.851 T:41920  NOTICE: Creating Demuxer
2020-01-20 20:30:40.851 T:41920   DEBUG: ffmpeg[0x281d99ca380X]: [udp] 'circular_buffer_size' option was set but it is not supported on this build (pthread support is required)
2020-01-20 20:30:40.853 T:23612   DEBUG: Previous line repeats 1 times.
2020-01-20 20:30:40.853 T:23612   DEBUG: Loading settings for pvr://channels/tv/All%20channels/pvr.iptvsimple_425868680.pvr

Full log: https://paste.kodi.tv/vewologahu.kodi

@phunkyfish
Copy link
Contributor Author

As it’s lacking pthread the error should be windows only.

@phunkyfish
Copy link
Contributor Author

phunkyfish commented Jan 21, 2020

@Paxxi would there be any side effects if we build ffmpeg for windows using pthreads instead w32threads that you know of?

So with something like --disable-w32threads --enable-pthreads or it's working equivalent?

@Paxxi
Copy link
Member

Paxxi commented Jan 21, 2020

I don't really know.

Based on the given information I would assume pthreads don't work on windows or there wouldn't really be a reason to have a w32-threads option.

Maybe it's a simple patch for ffmpeg where it checks for thread support? I would look into that first, if it's more than a bad if check then it might be worth trying with pthreads.

@phunkyfish
Copy link
Contributor Author

phunkyfish commented Jan 21, 2020

Good point. Will look into it.

May also be solved by a later version of ffmpeg, another option.

@phunkyfish
Copy link
Contributor Author

phunkyfish commented Jan 21, 2020

Ok, so ffmpeg requires the URL in a different format if source IPs are used: http://ffmpeg.org/ffmpeg-protocols.html#rtp

So rtp://sourceip@multicastip needs to become rtp://@multicastip?sources=sourceip. Which will mean an additional change in DVDDemuxFFmpeg so support this.

May also mean the error above is red herring.

@phunkyfish
Copy link
Contributor Author

Also, after some reading there is a marginal difference between w32threads and pthreads, hence why most use w32. We can always try it later on if we eventually need to.

@phunkyfish phunkyfish force-pushed the rtp-source-ip-fix branch 5 times, most recently from 828e571 to 147efcf Compare January 25, 2020 16:53
@phunkyfish phunkyfish added the WIP PR that is still being worked on label Jan 27, 2020
@phunkyfish phunkyfish force-pushed the rtp-source-ip-fix branch 4 times, most recently from 474cd9a to 56f075e Compare February 29, 2020 22:45
@FernetMenta
Copy link
Contributor

I seriously doubt that changing ffmpeg to pthreads is an appropriate solution for the issue you are trying to solve. Try to explain why this should require pthreads. Makes no sense at all. Why don't you elaborate on the root cause. Have you asked ffmpeg developers already?

Changing ffmpeg to pthreads introduces a high risk of breakage. Taking this risk for something that should be fixed elsewhere is a bad idea.

@phunkyfish
Copy link
Contributor Author

UDP streams use pthreads for their circular_buffer logic. The included patch adds missing logic in w32pthreads to allow this (hopefully) to function, but just for UDP.

Have not asked ffmpeg devs yet. I have submitted a patch that fixes an RTP bug, but I fear this currently lacks a maintainer.

Is the ffmpeg patch in the PR not how you would go about this?

@FernetMenta
Copy link
Contributor

Your commit messages are misleading. I thought you were disabling w32threads in favour of pthreads for the mingw build. This would have been a major change.

What you actually do is making udp use the pthreads compat layer on windows platform. That's a completely different story. Sorry for thy noise. More precise commit messages welcome :)

@phunkyfish
Copy link
Contributor Author

:) in hindsight it is a bit ambiguous!

@phunkyfish phunkyfish removed the WIP PR that is still being worked on label Mar 2, 2020
@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Mar 5, 2020
@jenkins4kodi jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Mar 5, 2020
@phunkyfish
Copy link
Contributor Author

All user testing on this has passed so will merge this tomorrow.

Due to the patching required I don’t think I will backport this to Leia.

@phunkyfish phunkyfish added this to the Matrix 19.0-alpha 1 milestone Mar 7, 2020
@phunkyfish phunkyfish merged commit de0d034 into xbmc:master Mar 7, 2020
@phunkyfish phunkyfish deleted the rtp-source-ip-fix branch March 7, 2020 09:43
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Mar 10, 2020
[url] skip parsing username:password@ for udp/rtp streams
Maven85 pushed a commit to Maven85/kodi that referenced this pull request May 5, 2020
[url] skip parsing username:password@ for udp/rtp streams
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 3, 2020
[url] skip parsing username:password@ for udp/rtp streams
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
[url] skip parsing username:password@ for udp/rtp streams
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
[url] skip parsing username:password@ for udp/rtp streams
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020
[url] skip parsing username:password@ for udp/rtp streams
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
[url] skip parsing username:password@ for udp/rtp streams
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 7, 2020
[url] skip parsing username:password@ for udp/rtp streams
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: PVR Component: Video Type: Fix non-breaking change which fixes an issue v19 Matrix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RTP streams that use an interface IP incorrectly interpreted as username
5 participants