This currently only works with the youtube-dl extension but would technically work with all custom downloader extensions, correct? If so, wouldn't it be better to use a generic name such as videoplayer_custom_downloader_support?

But I think it would be better to fix file_type() in model.py instead of adding this hack. There could be a hook for internal and custom downloaders to return a file type instead of forcing it to video for youtube and vimeo. It should first check self.mime_type for audio or video, otherwise call the hook and let it decide. The internal youtube code and youtube-dl extension could initially return 'video', effectively doing what this PR does, but without needing the config variable. In the future, they could determine the file type based on format ID, allowing for audio formats.

The existence of custom_downloader is used as a filter to determine which URLs can be fed to the video player for streaming, but the downloader itself is not used. To emphasize, if a custom_downloader for an URL exists and this config var is set, the video player is expected to be able to stream the URL directly.

Currently the only custom_downloader is in the youtube-dl extension, but if there would be more (and the set of URLs they support disjoint), then this would break. It's a hack for sure.

Getting downloader extensions to return the file type would be good, but in the case youtube-dl, getting a definitive answer requires connecting to the URL and parsing the response. The file_type() function is called all over the code via play_or_download(), so at least the result should be cached, if not stored to the DB already when parsing the feed.

Hi @elelay, would you be ok with merging the first two commits from this PR (i.e. remove the legacy audio and video player config vars), if I separate them to a new PR?

The new preferences dialog in the 1.1-adaptive branch currently uses the player.{audio, video} variables, which do not work if the legacy support is masking the 'player' variable (see #969).

I like the new consistent naming, but the last hunk in second patch is questionable. If the setting is None or empty, won't player be set to None or empty?

Merged first 2 commits, thanks.
@auouymous can you point at the line of the issue with player None or empty?

@elelay I commented on the merged commit (2497dd9).

I like the new consistent naming, but the last hunk in second patch is questionable. If the setting is None or empty, won't player be set to None or empty?

The config object does have a default value ('default') for the player vars, so they should never be set to None. If a user sets the player to an empty string, then it's the users problem. I see these changes were already merged, but OTOH adding the check back would be also ok for me.

The config object does have a default value ('default') for the player vars, so they should never be set to None. If a user sets the player to an empty string, then it's the users problem. I see these changes were already merged, but OTOH adding the check back would be also ok for me.

I am understanding correctly: that there is a problem if player is an empty string or null in the config instead of the object with 2 keys video and audio?
This is a problem only if the user comes now with gPodder settings from 2012 (before the introduction of player as an object) and they have an invalid (None or empty string instead of 'default') value for config.player.

I've tried gPodder before @tpikonen 2 commits with "player": "" and "player": null. Both break gPodder.

  File "/home/elelay/devel/gPodder/src/gpodder/gtkui/app.py", line 249, in on_itemPreferences_activate
    gPodderPreferences(self.window.gPodder,
  File "/home/elelay/devel/gPodder/src/gpodder/gtkui/interface/common.py", line 36, in __init__
    GtkBuilderWidget.__init__(self, gpodder.ui_folders, gpodder.textdomain, parent, **kwargs)
  File "/home/elelay/devel/gPodder/src/gpodder/gtkui/base.py", line 71, in __init__
    self.new()
  File "/home/elelay/devel/gPodder/src/gpodder/gtkui/desktop/preferences.py", line 199, in new
    index = self.audio_player_model.get_index(self._config.player)
  File "/home/elelay/devel/gPodder/src/gpodder/config.py", line 405, in __getattr__
    return getattr(self.__json_config, name)
  File "/home/elelay/devel/gPodder/src/gpodder/jsonconfig.py", line 197, in __getattr__
    value = self._lookup(name)
  File "/home/elelay/devel/gPodder/src/gpodder/jsonconfig.py", line 181, in _lookup
    return reduce(lambda d, k: d[k], name.split('.'), self._data)
  File "/home/elelay/devel/gPodder/src/gpodder/jsonconfig.py", line 181, in <lambda>
    return reduce(lambda d, k: d[k], name.split('.'), self._data)
TypeError: 'NoneType' object is not subscriptable

and

Traceback (most recent call last):
  File "/home/elelay/devel/gPodder/src/gpodder/gtkui/app.py", line 249, in on_itemPreferences_activate
    gPodderPreferences(self.window.gPodder,
  File "/home/elelay/devel/gPodder/src/gpodder/gtkui/interface/common.py", line 36, in __init__
    GtkBuilderWidget.__init__(self, gpodder.ui_folders, gpodder.textdomain, parent, **kwargs)
  File "/home/elelay/devel/gPodder/src/gpodder/gtkui/base.py", line 71, in __init__
    self.new()
  File "/home/elelay/devel/gPodder/src/gpodder/gtkui/desktop/preferences.py", line 199, in new
    index = self.audio_player_model.get_index(self._config.player)
  File "/home/elelay/devel/gPodder/src/gpodder/config.py", line 405, in __getattr__
    return getattr(self.__json_config, name)
  File "/home/elelay/devel/gPodder/src/gpodder/jsonconfig.py", line 197, in __getattr__
    value = self._lookup(name)
  File "/home/elelay/devel/gPodder/src/gpodder/jsonconfig.py", line 181, in _lookup
    return reduce(lambda d, k: d[k], name.split('.'), self._data)
  File "/home/elelay/devel/gPodder/src/gpodder/jsonconfig.py", line 181, in <lambda>
    return reduce(lambda d, k: d[k], name.split('.'), self._data)
TypeError: string indices must be integers

Also tried with "player": "/usr/bin/vlc" and it also breaks. Legacy keys only worked when used from the code, not directly in the settings AFAIU

Please ignore my previous comment. I wasn't reading the code properly...
I've added the check back, just in case.

Commit e2bd98c fixes the issue.

Rebased to master, which now has the legacy config variable changes.

#963 (comment) is still a better solution to this problem.

Another rewrite of the CustomDownload (youtube-dl) streaming support. We assign the episodes with CustomDL URLs an unique mime_type, which is then used to detect these episodes when deciding if streaming is possible.

Generic youtube-dl streaming needs support from the player, so the config variable remains necessary.

What is needed to test this patch?

Why are youtube and vimeo episodes set to application/x-gpodder-videoplugin?

Audio content supported by youtube-dl will be sent to the video player when this is enabled, and that doesn't work when the video player has no UI other than a video window. It would be better if the file type was known and the correct player used. But which players actually support this feature?

The new code in delete_from_disk() should have a linefeed above it.

The following check happens three times and the code might be easier to read if it was encapsulated inside an episode.is_customdl_video() call. Do you know if python uses equality or identity for string comparisons? If equality, it would be faster to check the config boolean before comparing the mime_type.

episode.mime_type == 'application/x-gpodder-customdl' and config.player.videoplayer_customdl_support

There are youtube-dl plugins for at least vlc and mpv. These plugins make the players stream youtube-dl compatible URLs directly, which is what is needed to use this patch. You'll also need a feed with suitable link URLs, like the Bandcamp one mentioned in #949.

The 'application/x-gpodder-videoplugin' is there mostly because of completeness, I wanted to have the mime_type set to something sensible (other than 'application/octet-strem') for all the cases. It's not necessary for the functionality here.

I don't really have a solution for the audio/video player problem, other than a per feed manual setting. I use mpv also for audio streams and it's not so bad with the on-screen contoller.

You're right about the linefeed and the cumbersome check, I'll fix them soon.

Rebased and updated per @auouymous comments.

You'll also need a feed with suitable link URLs

Twitch support is also possible with https://twitchrss.appspot.com/vod/{channel} feeds and supported formats (#1032).

The 'application/x-gpodder-videoplugin' is there mostly because of completeness,

It should be 'application/x-gpodder-customdl' for youtube and vimeo, because manage_downloads uses youtube-dl to download, and it should use it to stream as well. It also means youtube-dl can be used to stream when the built-in support is broken. But pointless, see below.

I don't really have a solution for the audio/video player problem, other than a per feed manual setting. I use mpv also for audio streams and it's not so bad with the on-screen contoller.

I tested with mpv and it also has no UI when playing audio episodes.

--

Another issue is that only the URL is passed to the player, and formats configured inside gpodder are ignored. Requiring users to find a supported player, configure formats externally and then enable a hidden option in gpodder is too hacky.

A better option would be to ask youtube-dl for the URL (the top-level url field in query result) and pass that to the player. This URL is for the chosen format and does not need youtube-dl support in the player. It can also play ondemand or live stream content. get_playback_url in model.py already does this for youtube, why can't it do the same for youtube-dl?

And while querying this URL, we can check the top-level width or height fields for a value. No value indicates an audio mime-type and the URL can be passed to the audio player. This "mime-type" would only be an audio/video flag, like PodcastEpisode file_type(). There is no need to set the mime-type when adding new episodes (without an enclosure), because it could change if the user changes formats.

There could also be a "Stream with Youtube-DL" option for those not using manage_downloads.

Downloads with youtube-dl should also check the width or height field and change to an audio mime-type, which fixes audio-only youtube formats. I'll submit a PR for this after the 3.10.20 release.

We could eventually get rid of the built-in youtube support (excluding the channel feed support) and let youtube-dl do all downloading and streaming. No more manange_downloads setting and no more chasing the youtube API. And if Mac/Windows bundles could update youtube-dl locally... :)

--

#949 added episodes with a youtube-dl supported URL. It would be better if gpodder added all episodes even if they lack media. It would be a better user experience if all episodes appeared instead the current method of skipping over them and returning an empty feed after subscribing. An error is already thrown when attempting to download an episode that doesn't have an URL, so no issues for episodes without one. The Twitch feed above has an URL and an HTML page will be downloaded, but gPodder could detect a non-audio/video content-type and fail instead of downloading. @elelay Do you disagree with this part?

A better option would be to ask youtube-dl for the URL (the top-level url field in query result) and pass that to the player. This URL is for the chosen format and does not need youtube-dl support in the player. It can also play ondemand or live stream content. get_playback_url in model.py already does this for youtube, why can't it do the same for youtube-dl?

This would probably work for the majority of users and feeds, but there are streams with multiple softsubs (and perhaps audio tracks) which need to be passed to the player somehow. Passing the per format stream URL from youtube-dl to the player also makes the title of the stream in the player to be the (looong) URL instead of the actual title. The Lua plugins for mpv and vlc exist for a reason.

I think the best we could do in this case is to have format strings for the extra info (title, subtitle URLs). These could then be configured by the user in the player command, and we could provide defaults for the common players. But this would not be zero configuration either.

The default should be to pass the stream/download URL to the player for broadest support. There could also be an option to pass the youtube-dl URL instead of the queried URL. It could still query (maybe) the URL to select the correct player, but that could be disabled with another setting to force a specific player since the audio player might not support youtube-dl URLs.

It wouldn't be difficult for gpodder to fill in format strings (already handled in util.format_desktop_command) to pass a title to the player. Automatically setting them for common players might be harder because existing users will never see the change unless they switch to another player and then switch back to update the setting.

Rebased to master and adapted to the current code in PodcastEpisode class and main.py.

I still don't agree this is the right way to handle this problem.

This patch did modify the only chunk of the new play_or_download() code that hasn't yet been fixed. All of the variables, including open_instead_of_play, are set true once and never revert back to false. Your patch overrides open_instead_of_play and forces it to false if custom streamable, which creates different toolbar button labels depending on the order of episodes in a selection. I need to add a method in PodcastEpisode to get the open_instead_of_play state and a comment in play_or_download() to not touch any of the variables because that is how the action code handles them. But the whole mixed open/play/preview/stream selection handling was skipped over until I figure out how best to do it.