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: use Clapper enhancers #1308

Merged
merged 9 commits into from
Feb 5, 2025
Merged

Conversation

GeopJr
Copy link
Owner

@GeopJr GeopJr commented Jan 24, 2025

Clapper 0.8 is out, so let's do #1171 again. Should probably rewrite the whole preview card logic.

fix: #1178
fix: #938
fix: #749
fix: #662
fix: #924

@Rafostar
Copy link
Contributor

Thanks for still working on this (its been a while) 👍

@GeopJr
Copy link
Owner Author

GeopJr commented Jan 25, 2025

I'm considering making it an optional flatpak plugin for now for two reasons:

  1. size; going off by just the CI here, it went from 0.8 to 5 MB. I assume it will be more on flathub / when packaged right. That's not 'a lot' but it's still 5x the current one
  2. updating; it will be easier to update ytdl outside of tuba, especially since youtube seems to break it often

(fwiw, size is not a clapper problem but mostly ytdl, ffmpeg etc I assume)

what do you think?

@Rafostar
Copy link
Contributor

what do you think?

Well, Tuba is a special case where video playback from sites like YT is not its major feature. It just can make-do without (it can just pop a browser). So having it as a plugin in Tuba makes sense.

Ideally, the easiest to maintain/include for everyone would be having it packaged in a Flatpak extension shared among the apps. This way, would give only one place to maintain it. Currently there are 4 apps (including Clapper itself) that I know of which would like to use it. But then, the questions arise: Why wasn't yt-dlp ever packaged into extension if many apps use it? If we are packaging enhancers, why not package whole Clapper into a Flatpak extension?

I dunno what are the requirements for such an extension to be accepted in Flathub. But if single place to package/maintain it existed, that would certainly make things easier. Currently, we are just adding them to manifests of each app separately.

fwiw, size is not a clapper problem but mostly ytdl, ffmpeg etc I assume

ffmpeg is already an extension, so it shouldn't increase size here. Outside of yt-dlp and enhancers itself, I think libpeas contributes to the size notably. For some reason its not in GNOME runtime (even through it appears to be a GNOME-tech).

@GeopJr
Copy link
Owner Author

GeopJr commented Jan 25, 2025

Why wasn't yt-dlp ever packaged into extension if many apps use it? If we are packaging enhancers, why not package whole Clapper into a Flatpak extension?

We can ask on the flathub/pak matrix rooms for advice I guess. Packaging clapper (lib) as an extension with enhancers and whatnot sounds good to me. ytdlp specifically needs constant updating and can render apps unusable if left unmaintained (e.g. Pipeline would probably stop working).

just want to see the size difference
@GeopJr
Copy link
Owner Author

GeopJr commented Jan 27, 2025

Unless I'm missing something, dynamically loading libraries on runtime in vala is beyond dreadful... (I'd have to use https://valadoc.org/gmodule-2.0/GLib.Module.html). Might have to bundle clapper anyway. I'm testing now a build without enhancers to see if the size difference is trivial, then I could make a tuba plugin with just the enhancers

@Rafostar
Copy link
Contributor

Rafostar commented Jan 27, 2025

Having at least enhancers as an extension would be nice start. Size aside, the more concerning problem is maintenance effort (compatibility and updates to yt-dlp) that would have to be done in each app. And they could be opt-in this way, since they are already dynamically loaded (within Clapper with libpeas).

@Rafostar
Copy link
Contributor

Um, I think you might have accidentally removed stuff from not the manifest used by CI.

just want to see the size difference
@GeopJr
Copy link
Owner Author

GeopJr commented Jan 27, 2025

🤦🤦🤦 thanks!

@GeopJr
Copy link
Owner Author

GeopJr commented Jan 27, 2025

816KB => 1.01MB

Sounds good to me.

the more concerning problem is maintenance effort (compatibility and updates to yt-dlp) that would have to be done in each app

I did some thinking on this and... can it be actually done? Flathub being strict on version matching, might render the whole 'update outside of apps' effort meaningless.

I mean, if Tuba depends on version 1.0.0 of the enhancers extension and then ytdlp breaks and you fix it, won't you have to publish 1.0.1? And won't apps need to bump the version manually?

Unless flathub allows us to be loose on patches so apps can depend on version 1.0 and patches get applied automatically

@Rafostar
Copy link
Contributor

Rafostar commented Jan 28, 2025

I mean, if Tuba depends on version 1.0.0 of the enhancers extension and then ytdlp breaks and you fix it, won't you have to publish 1.0.1? And won't apps need to bump the version manually?

I have never done a Flatpak extension, but... I guess, no?

AFAIU (from the little reserach I did), application just names the branch name as version (usually "stable") and patch downloads of updated versions take place separately from app that uses it. Like "ffmpeg-full" extension used here.

@Rafostar
Copy link
Contributor

size; going off by just the CI here, it went from 0.8 to 5 MB. I assume it will be more on flathub / when packaged right. That's not 'a lot' but it's still 5x the current one
updating; it will be easier to update ytdl outside of tuba, especially since youtube seems to break it often

I did some thinking on this and... can it be actually done?

It can be done (and I made it), but its up to Flathub maintainers whether they accept it, see: flathub/flathub#6106

I have also tried it locally besides Clapper itself with a modified Tuba manifest that mounts enhancers as an extension. So yeah, it works.

@Rafostar
Copy link
Contributor

Rafostar commented Feb 5, 2025

Approved as a bundle. See updated Clapper Flathub README for how to use this extension: flathub/com.github.rafostar.Clapper

Note that it will probably take a few hours from now before this will be accessible after merge.

@GeopJr
Copy link
Owner Author

GeopJr commented Feb 5, 2025

Woah! I didn't expect it to actually get approved, nice, thanks!

@GeopJr
Copy link
Owner Author

GeopJr commented Feb 5, 2025

Tested with and without, everything seems to work as expected! No idea how to make GNOME Builder install the extensions, but outside of it it worked fine.

I'll remove the CLAPPER_0_8 stuff since it's out now and can require it directly.

@Rafostar
Copy link
Contributor

Rafostar commented Feb 5, 2025

One thing that still bugs me, is why we need to build libpeas ourselves? 🤔

It seems to be in 47 runtime as far as I see: https://gitlab.gnome.org/GNOME/gnome-build-meta/-/blob/gnome-47/elements/core-deps/libpeas.bst?ref_type=heads

@Rafostar
Copy link
Contributor

Rafostar commented Feb 5, 2025

I mean, last time I checked build was failing without libpeas, but maybe I was testing this before 47? (I do not remember)

@GeopJr
Copy link
Owner Author

GeopJr commented Feb 5, 2025

gnome-build-meta is not just flatpak runtimes, only a handful of things are in them. The actual reason it's not included is:

We don't want to support them for various reasons (e.g. vte is not needed by most apps, libpeas has questionable stability and unclear relevance for flatpak apps, freerdp cannot really be considered part of the GNOME platform). Apps should just bundle these.

https://gitlab.gnome.org/GNOME/gnome-build-meta/-/issues/425#note_1271294

🤷

@Rafostar
Copy link
Contributor

Rafostar commented Feb 5, 2025

Ah, good find. Then having it be build before Clapper for apps that want enhancers will be necessary. Nothing more we can do about it.

@GeopJr
Copy link
Owner Author

GeopJr commented Feb 5, 2025

Merging! With that, Clapper is now the default player (if available). Thanks!

TODO: Make the audio visualizer's controls match Clapper's

@GeopJr GeopJr marked this pull request as ready for review February 5, 2025 18:14
@Rafostar
Copy link
Contributor

Rafostar commented Feb 5, 2025

TODO: Make Clapper be able to be used as audio player/visualizer 😉

@GeopJr GeopJr merged commit 0a2d9e5 into main Feb 5, 2025
5 checks passed
@GeopJr GeopJr deleted the feat/mediaviewer/make-clapper-the-default branch February 5, 2025 18:16
@Rafostar
Copy link
Contributor

Rafostar commented Feb 5, 2025

Make the audio visualizer's controls match Clapper

Alternatively, remember that Clapper offers individual widgets that bottom seek bar consists of. You have the freedom to design and do your own controls instead of using pre-made "simplecontrols" widget if you want.

So you can also customize Clapper to match your app (probably entirely from UI file). Well, I guess compat with GtkVideo that Tuba media viewer is designed/made around for might make it slightly harder/inconvenient.

@GeopJr
Copy link
Owner Author

GeopJr commented Feb 5, 2025

I should have asked for a review from you when I was implementing it but forgot 😩 The audio visualizer requires gstreamer so I had to write everything, including controls. I made them match the GTK ones as Gtk.Video was the default.

https://github.com/GeopJr/Tuba/tree/main/src/Widgets/AudioPlayer

The main thing I needed was levels (https://github.com/GeopJr/Tuba/blob/main/src/Widgets/AudioPlayer/Stream.vala#L120) so I could draw the visualizer based on them https://github.com/GeopJr/Tuba/blob/main/src/Widgets/AudioPlayer/Visualizer.vala

@GeopJr
Copy link
Owner Author

GeopJr commented Feb 5, 2025

419518356d8ad151.mp4

@Rafostar
Copy link
Contributor

Rafostar commented Feb 5, 2025

The audio visualizer requires gstreamer so I had to write everything, including controls. I made them match the GTK ones as Gtk.Video was the default.

ClapperGtkAudio is on my TODO list, since it will allow to reuse the same buttons that can be overlaid on ClapperGtkVideo, but there are lots of things/issues to do, so its not a top priority currently. Whatever you did is "fine" as you get it faster/already this way and it would make Tuba depend on Clapper for yet another thing otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants