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

feat(MediaViewer): enable GraphicsOffload for videos #909

Merged
merged 2 commits into from
Apr 18, 2024

Conversation

rmader
Copy link
Contributor

@rmader rmader commented Apr 14, 2024

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:

  • More testing is needed
  • Some possible design changes to e.g. hide the top bar in fullscreen mode after a timeout would increase the effect even more. This might be worth to investigate before merging. Doing this with the GTK inspector allows me to get zero-copy playback, which especially on mobile and low-end devices is really good.

@rmader
Copy link
Contributor Author

rmader commented Apr 16, 2024

@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
Copy link
Owner

GeopJr commented Apr 16, 2024

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 rmader marked this pull request as ready for review April 17, 2024 21:01
@rmader
Copy link
Contributor Author

rmader commented Apr 17, 2024

With further changes in the pipe - like #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
Copy link
Owner

GeopJr commented Apr 17, 2024

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
Copy link
Contributor Author

rmader commented Apr 17, 2024

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
Copy link
Owner

GeopJr commented Apr 17, 2024

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
Copy link
Contributor Author

rmader commented Apr 18, 2024

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

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.
@rmader
Copy link
Contributor Author

rmader commented Apr 18, 2024

Yay, it worked.

@GeopJr
Copy link
Owner

GeopJr commented Apr 18, 2024

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

@GeopJr GeopJr merged commit 9f9f16f into GeopJr:main Apr 18, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants