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(Attachment.Box): stream attachments by default #925

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rmader
Copy link
Contributor

@rmader rmader commented Apr 25, 2024

The bug preventing this before has long been fixed in Gst 1.22.1.


Closes #924

The bug preventing this before has long been fixed in Gst 1.22.1.
@GeopJr
Copy link
Owner

GeopJr commented Apr 25, 2024

Thanks!

Streaming worked but by using Files as a workaround. What I wanted to do was use inputstreams from libsoup. I pushed a commit that does that, but it won't work until it's fixed from GTK's side. They should be cacheable (this is just an initial implementation, I'd probably avoid creating yet another session for it and just use the images one)

If you try to play a video, you'll be met with a bunch of criticals and exit:

ERROR:../modules/media/gtkgstmediafile.c:290:gtk_gst_media_file_open: code should not be reached
Bail out! ERROR:../modules/media/gtkgstmediafile.c:290:gtk_gst_media_file_open: code should not be reached
Application exited

@rmader
Copy link
Contributor Author

rmader commented Apr 25, 2024

Gave it a quick try with https://gitlab.gnome.org/rmader/gtk/-/commits/issue4062 (from https://gitlab.gnome.org/GNOME/gtk/-/issues/4062#note_2093829), but that'll need a bit more debugging.

That being said, with just the first commit things work quite well already for me (tested with flatpaks with gst 1.24 from https://github.com/rmader/Tuba/tree/flatpak on laptops and phones). I'm missing the background here, but why do you prefer downloading over that kind of streaming / why inputstreams needed?

@GeopJr
Copy link
Owner

GeopJr commented Apr 25, 2024

When adding PeerTube support, which mostly allows streaming, I discovered two major issues that forced me to disable it:

  1. On long videos, sometimes the audio was not synced and almost every single one would stop updating its image after the ~30 minute mark while audio would still play - even after destroying the video widget
  2. They weren't seekable, its length was unknown

Not really related to the media viewer, but when drafting animated emojis (which was also blocked), a MediaFile was needed and (due to that bug + an unfortunate issue with Vala) I had to also use GFiles. I hit the file-max limit pretty quick and killed tuba.

Looking into the future, even if the PeerTube issues didn't occur due to using GFiles, I believe having control over the requests is important:

  1. ActivityPub software can deny requests if there's no user agent or one they don't like. GoToSocial for example does (or used to) deny all requests made without a user agent to their API
  2. Supposedly someone makes a 'secure' AP server, that requires a token or a POST request to get assets, it wouldn't be possible with just GFiles (unless im wrong)

@rmader
Copy link
Contributor Author

rmader commented Apr 26, 2024

I gave this a try on top of https://gitlab.gnome.org/GNOME/gtk/-/merge_requests/7186 - sometimes it works and plays videos fine, but usually it crashes with:

libsoup:ERROR:../libsoup/cache/soup-cache-input-stream.c:154:soup_cache_input_stream_write_next_buffer: assertion failed: (priv->output_stream && !g_output_stream_is_closed (priv->output_stream))
Bail out! libsoup:ERROR:../libsoup/cache/soup-cache-input-stream.c:154:soup_cache_input_stream_write_next_buffer: assertion failed: (priv->output_stream && !g_output_stream_is_closed (priv->output_stream))
Aborted (core dumped)

or without any error at all. It sounds like some threading issue in the MR here to me, but we should probably figure it out first before merging the GTK MR.

@rmader
Copy link
Contributor Author

rmader commented Apr 26, 2024

P.S.: thanks for the explanation, that makes a lot of sense!

@GeopJr
Copy link
Owner

GeopJr commented Apr 26, 2024

For the sake of not blocking it from the GTK side, I wrote a small reproducer that uses GFile but with inputstream https://github.com/GeopJr/Tuba/tree/experiment/reproducer-inputstream-video

the flatpak manifest uses your GTK branch

Unfortunately, I see it's just as unreliable, the video plays but the image is lagging behind. Seeking a bit can sometimes break it completely. Maybe streaming a local file is better for getting it merged on GTK

I experimented with libsoup threading but I faced other problems (plus libsoup is not entirely thread-safe) 🙃

I'll deal with these!

@rmader
Copy link
Contributor Author

rmader commented Apr 27, 2024

For the sake of not blocking it from the GTK side, I wrote a small reproducer that uses GFile but with inputstream
...
Unfortunately, I see it's just as unreliable, the video plays but the image is lagging behind. Seeking a bit can sometimes break it completely. Maybe streaming a local file is better for getting it merged on GTK

Nice, thanks a lot! So IIUC that means there's some issue in either libsoup, gtk or gstreamer not playing nicely yet - and that's not Tubas fault. So I'd say let's first try to fix that - we have the whole cycle before GTK 4.16 / Gnome 47 get released :)

@rmader
Copy link
Contributor Author

rmader commented Apr 30, 2024

I experimented with libsoup threading but I faced other problems (plus libsoup is not entirely thread-safe) 🙃

Just wanted to drop that I wonder if using the Gstreamer souphttpsrc would make sense - and if you tried that before?

@GeopJr
Copy link
Owner

GeopJr commented May 1, 2024

I haven't, but I'll experiment with it!

FWIW, I did start the initial experiment with Clapper on #931, and was pleasantly surprised with how well it worked

Screencast.from.2024-04-30.05-31-40.webm

(ignoring the papercuts) streaming just worked, it was seekable, it was aware of its length etc. I do recognize however, that clapper being relatively new as a library (instead of an app) and requiring(?) some gstreamer patches for now, it cannot be the default (that's also why it falls back to GtkVideo if meson cannot find it). I'll dive a bit deeper at some point to see how they deal with streaming

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.

[Bug]: Frequent crashes when playing high-res videos
2 participants