Rafostar / clapper

Level up your video experience with a modern and user-friendly media player.
https://rafostar.github.io/clapper/
GNU General Public License v3.0
795 stars 37 forks source link

Use custom widget for video paintable #269

Open Rafostar opened 2 years ago

Rafostar commented 2 years ago

Right now we use stock GtkPicture where our sink displays video using custom GdkPaintable. Unfortunately current implementation requires paintable to hold a weak ref on its parent widget, so we can measure it and apply black borders for every frame. This also prevents putting the same paintable within other widgets.

Ideally we should replace GtkPicture with something similar, but able to handle appending black borders on its own within snapshot, so our paintable can become more universal (and avoid a weak ref).

Rafostar commented 2 years ago

One thing that concerns me here through, is if GStreamer plugin loader will not encounter problems with having both GTK3 and GTK4 in single process if this is done. Right now this is avoided (partially via hack). But this would need to be carefully tested, as conflicting with GTK3 plugin that is used by multiple of other apps would not be plausible.

Rafostar commented 2 years ago

Worst case, making widget loaded as GModule, should work around it.

vixalien commented 1 year ago

appending black borders

Is this really necessary? In the best case scenario, clappersink should just provide a paintable. The app developer can put it inside a GtkPicture and add background-color: black to the video.

Rafostar commented 1 year ago

Is this really necessary?

Yes, we need widget cause we do much more with it. For e.g. GstNavigation interface and calculate widget mouse coords into video coords. This alows for e.g. navigation in DVD menus with mouse or touch on touchscreen (yes, I know we miss option to easily play DVD/BD discs, its not implemented yet but navigation already is).

and add background-color: black to the video

This is very inefficient. With such approach GPU would have to draw up to 2x the amount of pixels your screen has per frame. So viewing on 4k screen would require to draw whole screen background texture + video frame texture centered on to of it per frame. This is a huge hit on low-end devices (tested on Atom z3795 where current approach almost doubles max possible FPS we can play).

vixalien commented 1 year ago

what about providing 2 different sinks then, one that provides just a paintable and the one called clapperpicturesink or something that provides a Gtk.Picture (for all those optimizations) and maybe clappercustomsink that provides a custom widget.

I'm saying this because injecting a Gtk.Picture can be very limiting in real apps. I feel like it could be fine with just a paintable and maybe some functions to tell the width and height of where the paintable is painted.

Rafostar commented 1 year ago

I'm saying this because injecting a Gtk.Picture can be very limiting in real apps.

What would be your use case where you would prefer paintable alone instead of already placed within usable widget?

vixalien commented 1 year ago

There are many such use-cases, here's some I already envisioned:

  1. When you simply don't want to use Gtk.Picture. For example, one might be willing to use Gtk.Video to get automatic & native video controls or Gtk.Image to render a paintable in a constrained area. I might even want to use the paintable in a custom widget that manages paintables manually
  2. When you want to show the paintable at many places in the app
  3. When you don't want another window to pop out (when you don't use the provided Gtk.Picture)
Rafostar commented 1 year ago

Initially clappersink was made for the needs of this player cause GStreamer did not have a gtk4 compatible sink. But I do not mind people using it in their own projects if they want to.

For what Clapper needs we need custom widget implementation cause we do more with it and need size that paintable alone does not have. Once I have custom widget as this issue stands for, I will try to think of something to provide a way to duplicate video content (maybe by just giving paintable prop) as Hanabi (live wallpaper extension) also would like such feature.

vixalien commented 1 year ago

yes that's very understandable that clappersink was made for clapper and hence must first cater to it's needs first.

what about providing 2 different sinks then, one that provides just a paintable and the one called clapperpicturesink or something that provides a Gtk.Picture (for all those optimizations) and maybe clappercustomsink that provides a custom widget

on the other hand, did you consider this idea? this way people who just want a paintable can get the paintable, and projects that want a fully fledged widget (like clapper) can get a widget instead and directly place it in the UI.

Rafostar commented 1 year ago

Since single sink uses both paintable and a widget it is placed within, I would rather just try to provide both if needed from it instead for simplicity of maintaining one universal sink. But if it proves to be hard to achieve, I will consider splitting it/subclassing into two.

vixalien commented 1 year ago

thanks for understanding. and thanks for clapper

Rafostar commented 1 year ago

Note to self: implement overlays size negotiation for https://github.com/Rafostar/clapper/issues/321 when writing custom widget (another thing we need to have single widget in addition to possible multiple paintables).

When widget is unused, leave subtitles at video resolution. Otherwise if someone is using both widget and paintables, paintable will have overlays scaled to current widget size, which I guess should be fine as placing paintable elsewhere in addition to widget shouldn't be suprising if paintable picture reflects that of widget. Can also consider boolean prop to disable auto scaling if needed.

To not break compatibility with gst-launch-1.0, we should probably just not spawn a window if someone accessed paintable prop at least once (since unlike widget, we cannot tell if it was placed in app widget hierarchy AFAIK).

Alternatively, do as suggested above and subclass into one sink that provides paintable, but has less features and one more complete but limited to single widget where paintable is placed.