Properly support vertical videos #1195
Conversation
|
|
There was a problem hiding this comment.
It will be good if we can resize the screen size dynamically according to the size of the video we are currently playing, since we can get into full screen with horizontal ads.
I tried to fix this by calling setFullScreenSize whenever video is onPlay, but looks like MediaSession.ElementMetadata is only updated onFullscreen, so maybe there will be no solution under current API and gecko should fix this somehow:
com.igalia.wolvic-20240122-213236.mp4
There was a problem hiding this comment.
Just noticed 3 things for resizing:
- Looks like we will still get back to horizontal playing if we resize in full screen and click on the preset scales.
- Also the minimum width in resize should be smaller here to fit the new need. now if we resize, we won't get back to the original one.
- We might want to fix the ratio when resizing for horizontal videos, or it will cause some edges to be cut off
Hmm that's precisely what this patch does, we listen for the media onfullscreen which is the event that carries the video size information. |
Really good points, I'll check them. |
I mean, the size of the video can still change even though you keep playing it in full-screen, so only checking the video size |
Ah I see what you mean now. Off the top of my head I don't remember any geckoview API that covers that situation but I'll double check anyway. |
|
Converted to draft in order not to land it accidentally |
So indeed we don't get additional notifications when the size of media changes after going fullscreen. I've asked for additional API upstream. |
There was a problem hiding this comment.
There are still some bugs for resizing in vertical mode:
- Default size is much bigger than 1x (still a little bit smaller than 2x)
- 2x now is almost the same size as 0.5x (the problem should lie in 0.5x)
- When you drag the border, you can't get as small as 1x.
com.igalia.wolvic-20240124-203818.mp4
com.igalia.wolvic-20240124-204647.mp4
|
Haven't checked them in detail but note that there are constrains we apply for horizontal and vertical sizes. In particular the max vertical size is smaller than the horizontal one. The thing about 0.5x seems a bug indeed |
But I don't think it's reasonable that, when you drag the border, you can't get as small as 1x. Maybe we shall make 0.5x to be greater than the smallest size when dragging. |
There was a problem hiding this comment.
Did some research, and I found that, actually even for horizontal ones, the default video size is always somehow around 1.5x when you first enter full-screen mode. Since this is an old behavior, this can be ignored:
Default size is much bigger than 1x (still a little bit smaller than 2x)
The rest can be fixed according to my suggestion in the change request below.
I think we will be ready to merge this PR after a rebase and integrating the following suggestion. Nice work @svillar 💪
app/src/common/shared/com/igalia/wolvic/ui/widgets/WindowWidget.java
Outdated
Show resolved
Hide resolved
svillar
force-pushed
the
vertical_video
branch
2 times, most recently
from
de09ec5 to
dad3120
Compare
The getSizeForScale() scales the size of a window based on a given aspect ratio. It was however assuming that the aspect ratio was always bigger than 1, i.e., horizontal layouts. That's a valid assumption for browser windows that have this kind of layout but it is not necessarily true for fullscreen windows as there is a lot of media with vertical layouts (like videos recorded with mobile phones). This PR refactors the code so that vertical layouts are properly computed respecting the maximum sizes that we've established.
Popular streaming services host more and more videos in vertical format usually recorded with mobile phones. Wolvic was not properly handling the case of watching them in fullscreen mode due to some misshandling of some web events. The confusion started because there are 2 different fullscreen events coming from Gecko. On the one hand we have the content fullscreen event that can be triggered by any HTML element that goes fullscreen. And on the other hand there is the media fullscreen event which is only triggered by media (video) elements. When media goes fullscreen we typically get first the content onfullscreen event and then the media onfullscreen event. But the code that performs the window resize was only listening to the content fullscreen event because that seemed enough. The problem is that by the time we get the *content* fullscreen event we don't usually have access to the media, and thus we were only able to do a "normal" window resize to fullscreen as we do with 2D movies for example. Then the *media* fullscreen event was emitted with the media size information but that was just ignored. The solution is to indeed listen to the media fullscreen event and perform the resize of the window there again so that we could respect the video aspect ratio and also the orientation.
When rescaling a window using presets, we used to do it based on the default window size. However it'd make much sense to do it based on the current window size. On example of that is watching vertical videos.
svillar
force-pushed
the
vertical_video
branch
from
dad3120 to
0804fbf
Compare
Popular streaming services host more and more videos in vertical format usually recorded with mobile phones. Wolvic was not properly handling the case of watching them in fullscreen mode. There were 2 reasons for that: