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

Small fixes of issues with old devices support, brightness, etc #4272

Merged
merged 18 commits into from Sep 27, 2020

Conversation

avently
Copy link
Contributor

@avently avently commented Sep 11, 2020

What is it?

  • Bug fix (user facing)

Description of the changes in your PR

Some very small issues were fixed by this PR.

Fixes the following issue(s)

closes #4271
closes #4253
closes #4245
closes #4230
closes #4277
closes #4143
closes #4294
closes #4292
closes #4322

Testing apk

find the testing apk somewhere in this page because it's changes often

Agreement

@MD77MD
Copy link

MD77MD commented Sep 11, 2020

brightness levels work ok now👌

@opusforlife2
Copy link
Collaborator

@avently The seekbar jumps up a pixel in this as well. To be clear, it's not just the seekbar that moves. The two timers and the fullscreen button also move with it. The whole bottom setup also shrinks by a pixel on either side, I just noticed.

@avently
Copy link
Contributor Author

avently commented Sep 12, 2020

@opusforlife2 it's probably happens because in the app status bar in landscape mode and have one size, then you move out of the app, the status bar in portrait and it's larger, when you move Into the app again, status bar in landscape again. So the whole view will be repositioned on every such situation. And there are more elements like navigation bar make a difference.

Anyway:

  • this is not caused by this PR
  • I will never do anything with it, even will not try.

@opusforlife2
Copy link
Collaborator

this is not caused by this PR

Neve said it was.

I will never do anything with it, even will not try.

It's not worth fixing anyway. I just noticed it when I was comparing padding between blackbox's PR and the latest unified debug, so I asked.

@avently
Copy link
Contributor Author

avently commented Sep 12, 2020

@opusforlife2

Neve said it was.

I never said that you said:)

I said it just for future reviewers of the PR. So it means that this PR is fine.

It's not worth fixing anyway.

Actually I thought you'll open an issue but you agree with me. Bravo! It was unexpected :)

@MD77MD
Copy link

MD77MD commented Sep 12, 2020

🤣🤣🤣 both of you guys are just hilarious especially avently

@TobiGr TobiGr added bug Issue is related to a bug player Issues related to any player (main, popup and background) labels Sep 12, 2020
@avently
Copy link
Contributor Author

avently commented Sep 14, 2020

Added a commit related to Android 11 #4277. There is still a problem with overlapping thin navigation bar but I didn't fix it because it is already fixed but unreleased in #4255

@avently
Copy link
Contributor Author

avently commented Sep 15, 2020

I have something interesting today. After I initially made this https://github.com/TeamNewPipe/NewPipe/blob/dev/app/src/main/java/org/schabi/newpipe/player/VideoPlayerImpl.java#L1492
I thought it's too complex thing to maintain but I didn't find another way. Make once right and you never need to touch this black box. But from time-to-time we see new issues related to this code. After I saw how much needed to be done in order to make my code working here #4255 I couldn't sit back and relax and started to search an alternative.

So I found a new (old) way of how player positioning should work. It doesn't need to have all these manual calculations. It just works (almost identical to 0.19.8).

I checked in portrait/landscape, vertical/non-verical videos, with/without cutout, on 4.4 (emulator), 9, 11 (emulator), on Android TV (only a little bit), in multiWindow mode. Maybe some bugs still present on real devices, can't say.

It's not a small fix so it shouldn't be in this PR but since I started to work in that branch I left it as is. I can create a new PR with just this one commit if you want but this commit uses the changes from this PR anyway.

As I said, it works like in 0.19.8 but I don't like how shadow covers the player. Shadow should be reimagined too but not from me or not now 100%. For non-cutout the shadow works ok, but for cutouts it isn't as good as I want it to be.

Let me know if some bugs are present.

@opusforlife2 @TobiGr @Stypox @blackbox87 @wb9688 @B0pol
@ZeroPucks

positioning.zip

Edit: updated version with a perfect shadow:
position-and-shadow.zip

@opusforlife2
Copy link
Collaborator

Sorry, it's not exactly clear what you've changed from a user perspective. Can you clarify what to test?

@Stypox
Copy link
Member

Stypox commented Sep 15, 2020

@avently with that positioning APK on Huawei P9 lite android 7.0, the controls go behind the system ui (hardware buttons and top dropdown menu indicator) instead of being constrained by them

@avently
Copy link
Contributor Author

avently commented Sep 15, 2020

@opusforlife2 we have a video view that should be located under status bar and navigation bar in landscape, under status bar in multiwindow mode, away of statusbar in portrait.
We have controls (top and bottom menu buttons/position slider, fullscreen button that located inside the player view). And the controls should be accessible in any orientation, device size, device type, with or without cutout, etc. So the controls shouldn't be overlapped by other visual elements.

So if you see some button/control is inaccessible or located at a wrong position in the player view it means you find a bug.

@avently
Copy link
Contributor Author

avently commented Sep 15, 2020

@Stypox but this device doesn't have hardware buttons, only virtual buttons.

Can you show a screenshot?

@avently
Copy link
Contributor Author

avently commented Sep 15, 2020

@Stypox I installed Android 7.0 emulator and everything works fine, how to reproduce the issue you reported?

@opusforlife2
Copy link
Collaborator

Alright. I have a simple display. Hardware buttons and no cutouts/notches. Here's my testing comparing base unified and positioning apks:

  • Positioning: when you tap to show player controls and rotate to landscape, you can see that after the status bar hides itself, the shadow doesn't extend to cover that area, leaving a weird bright rectangle until the controls fade out.
  • Unified: the shadow correctly extends to that area.

This happens with auto-rotate both off and on.

That's it! That's the only bug I found.

People with cutouts/notches/navigation bars may have other issues, though, so I suggest trying this experiment in a separate Issue, and if it works out, a PR later.

@Stypox
Copy link
Member

Stypox commented Sep 15, 2020

but this device doesn't have hardware buttons, only virtual buttons.

Yeah, sorry, I meant those buttons. Anyway, I can't reproduce. Either I tested the wrong app (though I am pretty sure I tapped on "Open" after having installed the apk you provided), or who knows. I didn't do anything strange to create the situation I described above, I just tapped on a random video in trending and tapped on the fullscreen button.
@avently

@avently
Copy link
Contributor Author

avently commented Sep 15, 2020

@opusforlife2

when you tap to show player controls and rotate to landscape, you can see that after the status bar hides itself, the shadow doesn't extend to cover that area, leaving a weird bright rectangle until the controls fade out.

but I don't like how shadow covers the player. Shadow should be reimagined too but not from me or not now 100%

Fiiine. Made a perfect shadow. Shadow of all shadows!
If no problem will be found, the PR can be merged.

@Stypox

Anyway, I can't reproduce

or who knows.

For me with every configuration and on any devices I test the app works perfectly.
In this apk:

  • perfect shadow. No one has this shadow but you.

position-and-shadow.zip

@MD77MD
Copy link

MD77MD commented Sep 26, 2020

ok so @avently I tried this apk #4272 (comment) and got the same result as #4272 (comment) ... although the video would go back playing normal if you return the aspect ratio to fit... moreover, normal landscape videos don't suffer from this problem, on the other hand, probably because all three aspect modes have no effect what's so ever... perhaps we can test this if there is a landscape video with a weird aspect ratio.

 also want to listen @MD77MD feedback on these two issues.

do you mean by the second issue the brightness issue?

p.s: testing device.. galaxy s4 - android 4.4

@avently
Copy link
Contributor Author

avently commented Sep 26, 2020

do you mean by the second issue the brightness issue?

This one:
#4272 (comment)

Also let me know how to reproduce the brightness issue.

And thank you for the testing. Looks like the only thing I can do is to disable ZOOM mode for KitKat devices... so this mode will act like FIT mode.

@MD77MD
Copy link

MD77MD commented Sep 26, 2020

I don't know about that. Could you describe the steps I need to do in order to reproduce the issue?

@avently I have been trying for the past week to catch the exact steps to reproduce this bug but failed. however this is the closest I got:

  1. open a video and set brightness lets say 40 or 70% ... close or pause the video
  2. switch to another app (maybe do some stuff)
  3. go back to newPipe
  4. open any video in landscape mode... the video will start playing using the same last brightness you used (everything is fine until now)
  5. the bug: if you would try increase the brightness the screen brightness would suddenly drop and the slider would starts to increase from 0% even if the last value was greater.

expected behavior: when changing brightness... it would start from the same exact point used on the last video.

@avently
Copy link
Contributor Author

avently commented Sep 26, 2020

@MD77MD

the bug: if you would try increase the brightness the screen brightness would suddenly drop and the slider would starts to increase from 0% even if the last value was greater.

This can happen before this PR and I fixed such situation here. Are you sure you see such behavior in final2.zip?

- the app will not rotate the screen to portrait after video completes, it will just exit from fullscreen mode
- ability to rotate the orientation via fullscreen button from landscape to portrait when device has locked orientation in landscape
- ability to enter/exit to/from fullscreen on tablets with unlocked global orientation in portrait mode
@avently
Copy link
Contributor Author

avently commented Sep 27, 2020

In this apk:

final3.zip

More recent version below

@MD77MD
Copy link

MD77MD commented Sep 27, 2020

tried final3 #4272 (comment) on android 4.4 and does solve those mentioned problems.

… on vertical video to portrait after clicking on fullscreen button
@avently
Copy link
Contributor Author

avently commented Sep 27, 2020

Same as #4272 (comment) but with some changes:

  • made refactoring and included one suggestion to rotate the screen when in landscape & fullscreen on vertical video. So now the app will be rotated to portrait in this case after clicking on fullscreen button.

final4.zip

@opusforlife2
Copy link
Collaborator

I'm reporting a small visual bug here which is present in the unified debug, since it probably has a small fix as well:

Play a playlist or some other queue, and open the queue from the main player. Swipe down in the middle. The queue will rapidly vibrate up and down.

Copy link
Member

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Thank you for your ongoing efforts ❤️

@avently
Copy link
Contributor Author

avently commented Sep 27, 2020

@opusforlife2

The queue will rapidly vibrate up and down.

Vibrate? What do you mean? Show a screenrecord, nothing unexpected happens for me

@opusforlife2
Copy link
Collaborator

Give me a minute.

@opusforlife2
Copy link
Collaborator

Wow. Just... wow. The video was large so I scaled it down using ffmpeg from 1080 to 720. Somehow the size went from 43 MiB to under 2 MiB. O.o

ScreenRecord.mp4.gz

@TobiGr
Copy link
Member

TobiGr commented Sep 27, 2020

I was able to reproduce this, too. only happens, when the status bar is hidden. fir some reason, on a pixel 3a XL simulator with Android 11, the status and navigation bar are always displayed (but without content) when playing a video:
Screenshot_1601207142

@avently
Copy link
Contributor Author

avently commented Sep 27, 2020

@opusforlife2

ScreenRecord.mp4.gz

Thanks, reproduced.

fir some reason, on a pixel 3a XL simulator with Android 11, the status and navigation bar are always displayed (but without content) when playing a video

I use Pixel 3 emulator with Android 11 and no such problem. Are you sure you use the latest code or apk? Or do I need to do something special to reproduce?

@TobiGr
Copy link
Member

TobiGr commented Sep 27, 2020

I use Pixel 3 emulator with Android 11 and no such problem. Are you sure you use the latest code or apk? Or do I need to do something special to reproduce?

Never mind. Some noob tested the wrong version...

@avently
Copy link
Contributor Author

avently commented Sep 27, 2020

@opusforlife2

ScreenRecord.mp4.gz

fixed
final5.zip

@opusforlife2
Copy link
Collaborator

fixed
final5.zip

Confirmed!

Copy link
Member

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Thank you, once again!

@TobiGr TobiGr merged commit 541eb70 into TeamNewPipe:dev Sep 27, 2020
Unified Player regressions and bugs automation moved this from In Progress to Done Sep 27, 2020
This was referenced Sep 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment