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.

Sign up for GitHub

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

Jump to bottom

[videoplayer] allow custom headers to be sent to ffmpeg & inputstream class property #16847

Merged
merged 3 commits into from
Nov 14, 2019
Merged

[videoplayer] allow custom headers to be sent to ffmpeg & inputstream class property #16847

merged 3 commits into from
Nov 14, 2019

Conversation

phunkyfish
Copy link
Contributor

@phunkyfish phunkyfish commented Oct 30, 2019 •
edited
Loading

Description

Currently all unknown headers are not allowed to be passed to ffmpeg. This PR includes a blacklist to stop certain headers ever being sent to ffmpeg ad a whitelist of other ffmpeg headers. This allows all other custom headers to be passed.

Motivation and Context

How Has This Been Tested?

On OSX, verifying that headers get/don't get passed from teh log.

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 added Type: Improvement non-breaking change which improves existing functionality Component: Video v19 Matrix labels Oct 30, 2019
Rechi
Rechi reviewed Oct 30, 2019
View reviewed changes
xbmc/cores/VideoPlayer/DVDDemuxers/DVDDemuxFFmpeg.cpp Outdated
name == "reconnect_delay_max" || name == "icy" || name == "icy_metadata_headers" ||
name == "icy_metadata_packet")
{
CLog::Log(LOGDEBUG, "CDVDDemuxFFmpeg::GetFFMpegOptionsFromInput() adding custom header option '%s: %s'", it->first.c_str(), value.c_str());
Copy link
Member

Choose a reason for hiding this comment

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

The ColumnLimit in .clang-format is set to 100 which defines line length [...]

https://github.com/xbmc/xbmc/blob/master/docs/CODE_GUIDELINES.md#3-formatting

xbmc/cores/VideoPlayer/DVDDemuxers/DVDDemuxFFmpeg.cpp Outdated Show resolved Hide resolved
xbmc/cores/VideoPlayer/DVDDemuxers/DVDDemuxFFmpeg.cpp Outdated
else
{
CLog::Log(LOGDEBUG, "CDVDDemuxFFmpeg::GetFFMpegOptionsFromInput() ignoring header option '%s: %s'", it->first.c_str(), value.c_str());
CLog::Log(LOGDEBUG, "CDVDDemuxFFmpeg::GetFFMpegOptionsFromInput() adding user custom header option '%s: ***********'", it->first.c_str());
Copy link
Member

Choose a reason for hiding this comment

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

The ColumnLimit in .clang-format is set to 100 which defines line length [...]

https://github.com/xbmc/xbmc/blob/master/docs/CODE_GUIDELINES.md#3-formatting

@phunkyfish phunkyfish force-pushed the ffmpeg-custom-headers branch 4 times, most recently from 7b7dd3e to 63f21b2 Compare October 31, 2019 17:13
Rechi
Rechi reviewed Oct 31, 2019
View reviewed changes
xbmc/cores/VideoPlayer/DVDDemuxers/DVDDemuxFFmpeg.cpp Outdated
else
{
CLog::Log(LOGDEBUG, "CDVDDemuxFFmpeg::GetFFMpegOptionsFromInput() ignoring header option '%s: %s'", it->first.c_str(), value.c_str());
CLog::Log(LOGDEBUG, "CDVDDemuxFFmpeg::GetFFMpegOptionsFromInput() ignoring header option '%s'", it->first.c_str());
Copy link
Member

Choose a reason for hiding this comment

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

The ColumnLimit in .clang-format is set to 100 which defines line length [...]

https://github.com/xbmc/xbmc/blob/master/docs/CODE_GUIDELINES.md#3-formatting

This was referenced Nov 6, 2019
Closed
Closed
@phunkyfish
Copy link
Contributor Author

phunkyfish commented Nov 6, 2019 •
edited
Loading

@FernetMenta I added a commit here which would allow channels played from PVR addons such as iptvsimple to also pass ffmpeg headers (similar to using strm files).

Is it reasonable to assume that DVDSTREAM_TYPE_FFMPEG will function as well as DVDSTREAM_TYPE_FILE? Happy to remove it if there are more serious consequences to the change.

Note that streams with type DVDSTREAM_TYPE_PVRMANAGER (i.e. that handle inputstreams) are would not be covered by this change.

@FernetMenta
Copy link
Contributor

@phunkyfish your code targets pvr items that have set a dynPath. Since dynPath is not limited to internet streams only, this change is wrong.

@phunkyfish
Copy link
Contributor Author

phunkyfish commented Nov 7, 2019 •
edited
Loading

@FernetMenta, can you explain a little further for my understanding?

I'm trying to target pvr items that are internet streams, i.e. could require the ability to pass http headers. Is there another class of items with dynPath that I need to take care of?

Are there PVR items that would not set a dynPath? I think I only need to check path.

Or as always, some hints on the right way to go about this ;)

@FernetMenta
Copy link
Contributor

dynPath can be anything but you don't look at it. your current code assumes that dynPath for pvr items is always an internet stream url that can be handled by ffmpeg. those items with a dynPath that can be handled by ffmpeg should already be considered by the factory.
hard to say more without seeing an example url.

@phunkyfish
Copy link
Contributor Author

Ok, that makes sense. However the factory is not handling any of them currently. For instance http(s) would not be handled as it is.

Ideally there would be a whitelist of dynPath protocols that would be supported by ffmpeg. This should be easy enough to add. Thanks for the help ;)

@FernetMenta
Copy link
Contributor

For instance http(s) would not be handled as it is.

http(s) is not supposed to be handled by ffmpeg. passing those to inputstream ffmpeg would break a couple of functions like shoutcast redirect or file caching.

Ideally there would be a whitelist of dynPath protocols that would be supported by ffmpeg

already done by the factory

@phunkyfish
Copy link
Contributor Author

phunkyfish commented Nov 7, 2019 •
edited
Loading

Ok, but then what would be the best way to pass http headers if not using inputstream ffmpeg?

I guess a url option to request playback by ffmpeg would work.

@FernetMenta
Copy link
Contributor

why don't those headers find their why to CurlFile?

@phunkyfish
Copy link
Contributor Author

They do find there way to CurlFile. But they don’t find their way beyond that to FFmpeg such as for HLS and video fragments.

@FernetMenta
Copy link
Contributor

If CurlFile handles the http protocol and inputstream, ffmpeg does only demuxing. http headers are meaningless for a demuxer.
HLS is identified by the m3u8 extension and passed to inputstream ffmpeg.

you seem to have an odd or new case but you don't give much information on that.

@phunkyfish
Copy link
Contributor Author

phunkyfish commented Nov 8, 2019 •
edited
Loading

Sorry, never posted the reply. The reason is because the extension needs to be .m3u8 for it to choose inputstream fffmpeg. In my use case I'm serving the file from an API and it doesn't have this extension.

So I probably need a way to pass a hint to the factory so it knows the correct file type.

@FernetMenta
Copy link
Contributor

FernetMenta commented Nov 8, 2019 •
edited
Loading

You are facing the same issue like others before. I don't like the "solutions" for this in Kodi but see yourself:

For shoutcast streams, which are served over http(s) too, a new protocol specifier was introduced:
shout://blabla
This is just as little a protocol as pvr:// is and has the same weakness: there are cases where the url lacks this protocol specifier.
If CurlFile i.e. opens a shoutcast URL, it fires this ugly CRedirectException

Nowadays players get the entire fileitem insead just the url. IMO a much better solution is to set a property on the fileitem that can be examined by the factory. That's btw already done to select particular inputstream addons.

@phunkyfish
Copy link
Contributor Author

Perfect, I can use the latter to set a property via the PVR addon easily. That's makes the solution nice and clean.

@ksooo
Copy link
Member

ksooo commented Nov 8, 2019

Perfect, I can use the latter to set a property via the PVR addon easily.

This proerty should then get part of the PVR addon API and needs to be documented properly:

https://github.com/xbmc/xbmc/blob/master/xbmc/addons/kodi-addon-dev-kit/include/kodi/xbmc_pvr_types.h#L71 ff

@ksooo
Copy link
Member

ksooo commented Nov 8, 2019

Maybe even the existing "mimetype" property could be used here?

https://github.com/xbmc/xbmc/blob/master/xbmc/addons/kodi-addon-dev-kit/include/kodi/xbmc_pvr_types.h#L70

@phunkyfish
Copy link
Contributor Author

Which would you prefer? I added it as a separate property in latest commit. I made it general and not specific to m3u8, just setting a preference for inputstream ffmpeg.

@kib
Copy link
Member

kib commented Nov 11, 2019

Jenkins build this please

Rechi
Rechi previously requested changes Nov 12, 2019
View reviewed changes
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 code guidelines.

@phunkyfish
Copy link
Contributor Author

phunkyfish commented Nov 12, 2019 •
edited
Loading

@Rechi, done. It was hard to place the headers in order so added to a commit to reorder them.

EDIT, undid header ordering. The order that is there appears to be for a reason.

I did not change the lines with the long comments as breaking them still results in lines longer than 100 (plus they look really bad).

Let me know if there is anything else out of line but that should be everything.

@phunkyfish phunkyfish force-pushed the ffmpeg-custom-headers branch 3 times, most recently from c5001af to b97c630 Compare November 12, 2019 18:20
@phunkyfish phunkyfish changed the title [videoplayer] allow custom headers to be sent to ffmpeg after blockin… [videoplayer] allow custom headers to be sent to ffmpeg & inputstream class property Nov 12, 2019
@phunkyfish phunkyfish added the Type: Fix non-breaking change which fixes an issue label Nov 12, 2019
@phunkyfish
Copy link
Contributor Author

Managed to fix the header order. You happy now @Rechi ?

@phunkyfish
Copy link
Contributor Author

If there are no further objections I will merge this PR tomorrow.

@Rechi
Copy link
Member

Rechi commented Nov 13, 2019

The log lines still exceed the column limit of 100 characters.

@phunkyfish
Copy link
Contributor Author

Apologies @Rechi didn't include last fixup. Should be good now.

@Rechi
Copy link
Member

Rechi commented Nov 13, 2019 •
edited by phunkyfish
Loading

The include order of [videoplayer] refactoring - fix header order commit is wrong, so either drop that commit or fix it.

@phunkyfish
Copy link
Contributor Author

Can you explain how it’s wrong? I believed it to be correct as per the coding guidelines.

@Rechi
Copy link
Member

Rechi commented Nov 13, 2019

Look at the diff files in the CI build.

@phunkyfish
Copy link
Contributor Author

Thanks for that @Rechi, a handy thing to know for future reference ;)

Should be good now

@Rechi Rechi dismissed their stale review November 13, 2019 12:13

changed code now follows the current code guidelines

@phunkyfish phunkyfish added this to the Matrix 19.0-alpha 1 milestone Nov 14, 2019
@phunkyfish phunkyfish merged commit 60cd4e2 into xbmc:master Nov 14, 2019
@phunkyfish phunkyfish deleted the ffmpeg-custom-headers branch November 14, 2019 13:32
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jan 21, 2020
@phunkyfish @Maven85
Merge pull request xbmc#16847 from phunkyfish/ffmpeg-custom-headers
43d51cd 
[videoplayer] allow custom headers to be sent to ffmpeg & inputstream class property
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FernetMenta FernetMenta FernetMenta left review comments

@ksooo ksooo ksooo left review comments

@a1rwulf a1rwulf a1rwulf left review comments

@peak3d peak3d peak3d left review comments

@Rechi Rechi Rechi left review comments

Assignees
No one assigned
Labels
Component: Video Type: Fix non-breaking change which fixes an issue Type: Improvement non-breaking change which improves existing functionality v19 Matrix
Projects
None yet
Milestone
Matrix 19.0-alpha 1
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@phunkyfish @FernetMenta @ksooo @kib @Rechi @a1rwulf @peak3d