Add an ExoPlayer settings page #8875
Conversation
AudricV
added
feature request
|
Kudos, SonarCloud Quality Gate passed! |
|
I'd recommend that, when using multiple sentences for a preference summary, that each sentence end with a period. E.g. "These changes require a player restart to take effect." Same with the ExoPlayer settings. |
This behavior is used in the majority of settings description with multiple settings, so I try to kept the same style. |
I just checked all the settings, and that does seem to be the case. That's kinda weird IMO, but alright. 👍 |
AudricV
force-pushed
the
exoplayer-settings
branch
from
0b6ec1f
to
1d4227c
Compare
AudricV
force-pushed
the
exoplayer-settings
branch
from
1d4227c
to
b699465
Compare
AudricV
force-pushed
the
exoplayer-settings
branch
from
b699465
to
748f77f
Compare
|
Kudos, SonarCloud Quality Gate passed! |
|
I agree that having the ability to disable tunneling from settings in the release build is a good thing, but I also believe a tunneling blacklist should be maintained for those devices that don't support it properly. Otherwise you're just going to end up with frustrated users searching forums and whatnot trying to find out why the app is unusable or constantly crashing for them. Even though that switch was available in the debug build, it wasn't once mentioned on the bug report I was following, and not once did I consider it a potential fix. I had to actually go through the whole debugging process to find it. Maybe a knowledgeable dev would have found that much sooner, but I'm just a lowly sys admin; it took some time. Anyway, give users of the application a nice experience from the get go, without the need for having to hunt down why it won't work for them in the first place. For those with issues that do manage to find the switch, perhaps also adding the internal device name & API version to the about page would help aid in maintaining the blacklist in future? Rather than someone reporting that "disabling tunneling works for me on a Zephir TS43UHD-2", they could instead report that it works for them on a cvt_mt5886_eu_1g with API version 24 for example. Anyway, just my 2c. |
@glbyers The thing is, media tunneling was created for Android TVs in the first place. So bypassing it on TVs makes no sense at all. o_O
That's the idea. Since all these TVs were working fine before 0.24.0, this is a regression, and a fix for this should fix full screen on all TVs, not specific models. You finding out that media tunneling causes this has been a huge help, so thank you for debugging. ^_^ |
There are a significant number of issues both open and closed in the exoplayer github related to media tunneling. I've seen some of the devs suggest that it's a risky thing to enable by default as it has a high number of device specific problems. I get how valuable it is, and sweet if it works, especially at 4k resolutions. But it's kinda reliant on every device manufacturer that claims to support it to implement it properly. Even with my TV, it's not that the codecs don't support media tunneling, it's more that the audio codec doesn't support changing of the surface after it has been started with tunneling enabled. I'm not at all arguing against a more general fix. I'm not sure an obscure option is necessarily a general fix though, especially if it defaults to on. It should still default to off for devices it's known not to work with in my mind. Perhaps it needs some clarity so it's obvious what it does, not what it's called. "Hardware decoding" is instantly more recognisable than '"media tunneling" IMO. FWIW, as I said in the issue. It wasn't working properly prior to 0.24 either. At least for my TV. It's just that having to relaunch it between every video was still a usable solution. Anyway, you know what they say. Opinions are like something or other. Everyone has one. |
@glbyers Oh, this PR is not the intended fix. It just happens to contain the workaround you found already. We've asked users in that thread to test it so that we can be sure that everyone's problem was the same and is indeed solved this way. Don't worry. We'll try to look for a proper fix that doesn't need users to mess around with decoding settings. |
ghost
mentioned this pull request
|
Hey this fixes the issue for me on android tv, it is slow as though (probably due to not debugging apk), can this be merged as a quick fix for TV users. Its hard to debug issues on TV compared to phone and I dont think the issue has to do with media tunneling but rather refactoring the player. Thanks. |
| && Build.DEVICE.equals("RealtekATV"); | ||
| // Philips QM16XE | ||
| private static final boolean QM16XE_U = Build.VERSION.SDK_INT == 23 | ||
| && Build.DEVICE.equals("QM16XE_U"); |
There was a problem hiding this comment.
Mmmh, why are you removing this list completely? We know all these TVs don't work, so by default on such devices tunneling should be disabled. Then the user can go into settings and enable it, but at least it should be off by default.
There was a problem hiding this comment.
Someone needs to write a methid that sets a default value depeding on the device. Otherwise, NewPipe is going to stop working for some users and they certainly not search for a solution in the settings.
| // enable media tunneling | ||
| if (DEBUG && PreferenceManager.getDefaultSharedPreferences(context) | ||
| // Disable media tunneling if requested by the user from ExoPlayer settings | ||
| if (!PreferenceManager.getDefaultSharedPreferences(context) | ||
| .getBoolean(context.getString(R.string.disable_media_tunneling_key), false)) { |
There was a problem hiding this comment.
So I would make sure the preference does not get auto-created, and then use !DeviceUtils.shouldSupportMediaTunneling() instead of false as the value to use when the preference is not set.
app/src/main/java/org/schabi/newpipe/player/helper/OldMediaCodecSelector.java
Outdated
Show resolved
Hide resolved
Why? That's a standard player setting, not specific to Exoplayer-related concerns. |
|
Well, it's a setting that changes how exoplayer seeks, so it's more a tecnichal setting. But yeah, it's arguable whether that setting should be under the exoplayer menu or not. |
|
My understanding of this PR was that it would contain esoteric settings that most users shouldn't be touching. Seek interval is a regular customisable setting. |
Done. |
AudricV
force-pushed
the
exoplayer-settings
branch
from
748f77f
to
e1b2852
Compare
|
Kudos, SonarCloud Quality Gate passed! |
…tting into it This fragment has been added into SettingsResourceRegistry, to allow searches in its options. It has been placed at the place of the previous playback load interval size setting (so in Video and Audio settings).
This option could help to avoid decoder initialization issues, which falls back to lower-priority decoders if decoder initialization fails. This may result in poor playback performance than when using primary decoders. It is disabled by default, but can be enabled in ExoPlayer settings.
…ing available on release builds Media tunneling may be not supported by more devices than the ones we whitelisted before. As a matter of fact, the list of devices on which media tunneling is disabled could be not maintainable in the future, especially if the list of devices grows more and more. A preferable solution is to allow users to configure this setting themselves, allowing them to not wait for their device(s) to be whitelisted in a future NewPipe update. This solution has been applied in this commit and works on every build type. The corresponding preference in the debug settings has been of course removed and the code used to prevent media tunneling activation on specific devices has been removed.
- Remove redundant player restart requirement note, as it is written on the ExoPlayer settings description page; - Add precision about the setting effect/limitation, as it only applies on progressive contents/media sources and not on every content/media source; - Remove translations of this description, to ensure that they will be updated by translators.
…derer setOutputSurface workaround As some devices not present in ExoPlayer's list may not implement MediaCodec.setOutputSurface(Surface) properly, this workaround could be useful on these devices. It forces ExoPlayer to fall back on releasing and re-instantiating video codec instances, which is always used on Android 5 and lower due to addition of this method in Android 6. To do so, a CustomMediaCodecVideoRenderer, based on ExoPlayer's MediaVideoCodecRenderer which always return true for the codecNeedsSetOutputSurfaceWorkaround method has been added, which is used in CustomRenderersFactory, a class based on DefaultRenderersFactory which always returns our CustomMediaCodecVideoRenderer as the video renderers. CustomRenderersFactory replaces DefaultRenderersFactory in the player, in the case this setting is enabled.
Stypox
force-pushed
the
exoplayer-settings
branch
from
e1b2852
to
787758a
Compare
|
Kudos, SonarCloud Quality Gate passed! |
What is it?
Description of the changes in your PR
This PR adds an ExoPlayer settings page to NewPipe's settings, at the current place of the playback load interval size setting (
Video and audiosettings page).This page contains three settings:
More settings could be added in the future, such as ones to change buffer settings.
These settings require a player restart to take effect (this information has been written on the link of this page, in
Video and audiosettings).Before/After Screenshots/Screen Record
Fixes the following issue(s)
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.
Due diligence