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

Disable media tunneling by default on known unsupported devices #10122

Merged
merged 5 commits into from
Jul 31, 2023

Conversation

TobiGr
Copy link
Member

@TobiGr TobiGr commented May 26, 2023

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

In #8875 a setting to disable media tunneling was added. However, the info on devices which do not support media tunneling was removed. That info is re-added and used to disable media tunneling on those devices by default.

Disabling media tunneling seems to also fix the full screen enter/exit bug on Android TVs (#9023)

New models added:
  • Sony Bravia VH1 (BRAVIA_VH1)
  • Sony Bravia VH2 (BRAVIA_VH2), e.g. model A90J
  • Sony Bravia Android TV platform 2 (BRAVIA_ATV2)
  • Sony Bravia Android TV platform 3 4K (BRAVIA_ATV3_4K)
  • Phillips 4K (O)LED TV (PH7M_EU_5596)
  • Panasonic 4KTV-JUP (TX_50JXW834)
  • Bouygtel4K (Bouygues Telecom Bbox 4K) (HMB9213NW)
Other changes:
  • Added info to ExoPlayer settings explaining that media tunneling was disabled automatically.
Media tunneling setting screenshot grafik

Fixes the following issue(s)

ToDo

  • Rework applying device detection results and restoring that info correctly from a backup.

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. You can find more info and a video demonstration on this wiki page.

Due diligence

@TobiGr TobiGr added bug Issue is related to a bug player Issues related to any player (main, popup and background) labels May 26, 2023
@TobiGr
Copy link
Member Author

TobiGr commented Jun 5, 2023

I wanted to wait on feedback from #8829 and #10112 to include more devices, however there is no response.

@TobiGr TobiGr marked this pull request as ready for review June 5, 2023 11:47
@peat80
Copy link

peat80 commented Jun 12, 2023

BRAVIA_VH2 should be added to this list.

@TobiGr
Copy link
Member Author

TobiGr commented Jun 12, 2023

@peat80 which android version is it running?

@peat80
Copy link

peat80 commented Jun 12, 2023

I believe the tv runs android tv 10.

@TobiGr
Copy link
Member Author

TobiGr commented Jun 12, 2023

@peat80 Please test the APK (see PR description). I hope that everything works as expected. If it does not, please go to the settings > debug and use on of the options which crash the app and show the error screen. The crash info does contain information on your Android version. If it is Android 10 / SDK level 29, go into the video & audio settings > ExoPlayer settings and check whether the option "Disable media tunneling" is enabled. If it is not enabled, enable it and try to play a video.

@peat80
Copy link

peat80 commented Jun 12, 2023

Back home now. Can confirm tv indeed runs on android 10. The downloded artifact apk has media tunneling disabled per default now and works. 😀👌

@peat80
Copy link

peat80 commented Jun 13, 2023

BRAVIA_VH1 should probably also be added.

There was a closed PR for that:

#9425

@TobiGr TobiGr mentioned this pull request Jul 4, 2023
6 tasks
@TobiGr TobiGr added the Android TV Issue is related to Android TV label Jul 8, 2023
@TobiGr TobiGr mentioned this pull request Jul 8, 2023
5 tasks
@TobiGr TobiGr marked this pull request as draft July 8, 2023 23:04
@TobiGr TobiGr marked this pull request as ready for review July 16, 2023 10:58
@pihug12
Copy link

pihug12 commented Jul 17, 2023

I don't suffer the "no video / black screen" problem, but disabling "media tunneling" with this APK solves the fullscreen problem for me.

  • Model: Bouygtel4K (Bouygues Telecom Bbox 4K)
  • Android TV 9 (build "Californie")
  • Device ID: HMB9213NW

I don't know if the device should be added to this list or not?

@TobiGr
Copy link
Member Author

TobiGr commented Jul 18, 2023

@pihug12 I disabled it in case there are other people using the same device.

Copy link
Member

@AudricV AudricV left a comment

Choose a reason for hiding this comment

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

Your changes look mostly good to me, however, I think we should use a complete different approach instead of DB migrations, by using an app preference containing with which version of the app the media tunneling check has been ran and run the check with each NewPipe update.

With this value, we can check if the version matches the current one installed + use some logic with the two preferences storing whether the device has been whitelisted automatically and media tunneling has been enabled by user, in order to know if we need to disable automatically media tunneling or not.

This approach should make maintenance less hard than doing a lot of database migrations every time we add a new device.

Could you also format better and improve your method Javadocs? For instance, you are using line returns to separate titles and descriptions, but they are ignored, so you should also use paragraph elements in this case.

@Ang-le
Copy link

Ang-le commented Jul 26, 2023

Device ID: BRAVIA_ATV2
I just tried it and it worked for me in full screen Disable media tunneling.

Thank you

Revert removing the Utils related to media tunneling.
Store in preferences whether media tunneling was disabled automatically.
Show info in ExoPlayer settings if media tunneling was disabled autmatically.
Sony BRAVIA_VH1, BRAVIA_VH2, BRAVIA_ATV2, BRAVIA_ATV3_4K
Phillips 4K (O)LED TV (PH7M_EU_5596)
Panasonic 4KTV-JUP (TX_50JXW834)
Bouygtel4K (HMB9213NW)
@TobiGr
Copy link
Member Author

TobiGr commented Jul 29, 2023

I added two new devices: BRAVIA_ATV2 and BRAVIA_ATV3_4K as requested by @Ang-le and @ROBYER1.
@AudricV I'll address your review during the next week. Thank you for clarifying what you meant by using preferences.

@AudricV AudricV dismissed their stale review July 31, 2023 20:11

Outdated review

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

I think a simple list preference with "Tunneling on, tunneling off, automatic" would have been simpler, since with the current code once you manually disable tunneling there is no way to go back to automatic mode. I think we can postpone this to a small PR after the release.

@sonarcloud
Copy link

sonarcloud bot commented Jul 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@TobiGr TobiGr merged commit 3c91ec3 into dev Jul 31, 2023
@TobiGr TobiGr deleted the fix/media-tunneling branch July 31, 2023 21:30
This was referenced Jul 31, 2023
@disconnect5852

This comment was marked as spam.

@TobiGr

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android TV Issue is related to Android TV bug Issue is related to a bug player Issues related to any player (main, popup and background)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Crash when going fullscreen (especially Android TVs) Android TV - No video/black screen
8 participants