-
-
Notifications
You must be signed in to change notification settings - Fork 60
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
fix(mv): clapper: GIFV handling fixes #1075
Conversation
Tuba now tries to treat GIFV as GIFs and not videos. For this reason and consistency with how other clients/browser work, there should not be a MPRIS controls for GIF media.
Since Clapper patch version 0.6.1 this is fixed to be done automatically in ClapperGtk video, so no need to create another object instance here. Most distros have now updated to 0.6.1 version, Clapper in Tuba is not used by default and even with 0.6.0 the only dowside of removing it would be wrong audio pitch on non-1x speed, so not a major enough reason to do a version check just for that.
Tuba always set player to playing (both GtkVideo and Clapper) for any kind of video when ready, so autoplay here is redundant.
When going "stopped" -> "paused" player will already start caching/buffering until it has enough data to ba able to go into "playing" immediately. This causes a problem where multiple created video widgets for multiple GIFV media download all of videos at once while user views just on a one of them. Fix this misbehavior by not going into "paused" when we are "stopped" which means we are at initial state.
LGTM, thanks! I want to do some deeper testing with some extreme cases and I'll merge it afterwards
Unfortunately there's a lot at play :(, for example, even the attachment position matters and if there's: GIFV, GIFV, IMAGE, GIFV, then it would need at least 2 Clapper instances. Position matters sometimes, like in the case of documents where each attachment is a different page etc. Maybe we could use a queue if all the attachments are videos? (but that will complicate things a lot, so let's just leave as is for the near future 😅) |
Absolutely. Doing it like it is done currently was my intention, hence this PR.
In ideal scenario, there would be exactly 1 video player instance (since Tuba shows only 1 video/GIFV at a time) and media viewer is something that can show both videos and images (e.g. a GTK Stack with video and picture viewers that switches between these 2 child widgets when needed). If it could hide itself instead of destruction when unneeded/done that would be even better. There is a lot of work going on (between GTK and GStreamer) to prepare for video playback (not to mention decoders and separate memory pools of each pipeline, etc.). Afterwards, most of it does not have to be done again when video widget is recycled. This is true for both GtkVideo and Clapper that use GStreamer internally, so both would benefit. I just wanted to mention that. This is just a friendly TIP, for how this can be optimized (how can but its not a must, just optimization). I do not want nor plan to meddle with someone else's app too much. Just trying to give some friendly pointers/help and nothing more 😄 . |
Thank you for all the info! I value your opinion and knowledge on the matter and really appreciate it :) Feel free to share your thoughts whenever you feel like it! |
Seems to work fine (at least for me 😄) and does what its supposed to do (no MPRIS for GIFs and no buffering/download for remaining videos/gifs when only one is selected), so I am unmarking this from being a draft. |
LGTM, thanks! |
Some fixes mainly related to handling multiple GIFV in a single post. More info in individual commits descriptions.
Ideally, Tuba could be more efficient by creating a single video instance, putting multiple GIFV in Clapper Queue and just switching between them (maybe even with overlaid pre-made NextItemButton / PreviousItemButton) instead of creating multiple video instances and switching widgets. Unfortunately, this would be probably too much/hard to implement while retaining support for GtkVideo which is the default backend within a single source code file, so for now a few changes working around that to remain compatible 😄 .