search

GeopJr / Tuba

Browse the Fediverse
https://tuba.geopjr.dev/
GNU General Public License v3.0
506 stars 55 forks source link

feat(MediaViewer): enable GraphicsOffload for videos #909

Closed rmader closed 2 months ago

rmader commented 2 months ago

In order to allow pass-through of video content to Wayland compositors, reducing copies on the GPU and thus improving battery life and performance.

This change should generally be safe to do, however given the higher chances of triggering driver bugs good testing is due.

An easy way to test this is to look at intel_gpu_top/radeontop/ similar tools - in many cases this should be able to reduce GPU work considerably, especially for high-res content.

Note that for now it only affect hardware-decoded content - this will hopefully change in the future.

Draft because:

rmader commented 2 months ago

@GeopJr: just to ask in advance: would you be open to make the video top-bar auto-hide together with other video controls in fullscreen mode? I think that would be good both from a design and a performance perspective :)

On touch devices all controls would probably need to toggle on/off on tap.

P.S.: it might be a chance to give the video mode a small design overhaul - did you already have ideas for that? I hope to talk to some members of the design team about that soon, they might have good ideas as well.

GeopJr commented 2 months ago

Yeah, I wouldn't mind removing the headerbar on fullscreen altogether and instead have a floating 'toggle fullscreen' button (visible on mouse movement/click and such)!

In general, I'll follow anything the design team suggests. In the past, Tobias on #299 suggested making the state overlay of GtkVideo clickable (done upstream) and adding padding around the media controls (ignored upstream, done here). On #397, dropping the zoom buttons was mentioned but no further discussion took place

As I mentioned briefly on #299, it might be time for a libadwaita video player or controls so other apps like Fractal can also benefit from it - rather than designing and implementing one just for Tuba

rmader commented 2 months ago

With further changes in the pipe - like https://github.com/GeopJr/Tuba/pull/913 - I think this is good to go. Tested on a few devices and haven't seen any regression - while there are some pretty nice performance improvements in various cases.

FTR: I think this is also good for a point release. In case some people see regressions, we'd most likely have to fix them on the driver side. Tuba is one of the first apps to (hopefully :P) adopt this new feature, but fortunately would be much less badly affected than e.g. a video player. I.e. if people using specific hardware would see buggy output when watching videos, that wouldn't break the main features of the app.

GeopJr commented 2 months ago

Thanks for leading this!

Btw, I don't know if it makes a difference, but FWIW, Tuba does not use ngl, it uses gl (due to some performance regressions). It won't override GSK_RENDERER if manually set however

Feel free to experiment on Tuba if needed in the future again! It's still 'alpha' software so I don't mind if it's used as a testing ground

rmader commented 2 months ago

Oh, the Flatpak build seems to fail because it's still using GTK 4.13.9, but this needs 4.14 - do you happen to know where to fix that?

Regarding ngl and gl - fortunately the offloading is still supported with the old renderer, allowing it to work on devices without GLES 3.0 support. There are not many of them any more, but some mobile devices like the Librem5 are among them - and that benefits a lot from the offloading, more than most more powerful devices.

Feel free to experiment on Tuba if needed in the future again! It's still 'alpha' software so I don't mind if it's used as a testing ground

Thanks a lot for the app, the help and the quick responses! I use Tuba as my main Mastodon client now and am really happy with it already - and will surely come back from time to time with issues or ideas.

GeopJr commented 2 months ago

do you happen to know where to fix that?

I think 46 does use 4.14 but 46beta doesn't (and the CI should switch to 46 now that it's released I guess). I'll do it don't worry about it. (The runtimes are maintained on https://gitlab.gnome.org/GNOME/gnome-build-meta)

rmader commented 2 months ago

Just wanted to play with the flatpak and stumbled over the 46beta entry there - with that, I suppose the CI should succeed now.

rmader commented 2 months ago

Yay, it worked.

GeopJr commented 2 months ago

LGTM, merging! Hopefully the repo change won't create issues for nightly users