Page MenuHomeSolus

subtitlecomposer: disable libmpv backend
ClosedPublic

Authored by aleksvor on Nov 22 2020, 6:12 PM.

Details

Summary

mpv backend relies on qthelper.hpp header, which was removed in mpv 0.33.0. While it's possible to provide this header in source tree of Subtitle Composer (that's what mpv recommends downstreams to do), I think it's not necessary:

  1. GStreamer backend is still functional and on par with mpv backend.
  2. Upstream Subtitle Composer don't even support these backends anymore - they have removed them in favor of unified backend based on FFmpeg and OpenGL.

Depends on D10020.

Test Plan

Loaded MKV container with English subtitles. Checked that Gstreamer player backend can playback the video, render original subtitles and my translated subtitles.

Diff Detail

Repository
R4928 subtitlecomposer
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

aleksvor created this revision.Nov 22 2020, 6:12 PM
aleksvor requested review of this revision.Nov 22 2020, 6:12 PM
aleksvor edited the summary of this revision. (Show Details)Nov 23 2020, 10:14 AM
aleksvor edited the summary of this revision. (Show Details)
livingsilver94 added inline comments.
package.yml
30

IMHO here it's better to remove specific files. At first glance, it seems you're removing the 64x64 icons of subtitlecomposer instead of stuff related to mpv.

aleksvor updated this revision to Diff 24071.Nov 23 2020, 1:02 PM

Made icon deletions more easy to read and clarified them in comments - thanks @livingsilver94 for suggestion.

aleksvor updated this revision to Diff 24072.Nov 23 2020, 1:22 PM

Remove unneeded pkgconfig(xcb).

JoshStrobl requested changes to this revision.Nov 23 2020, 3:00 PM
JoshStrobl added a subscriber: JoshStrobl.
JoshStrobl added inline comments.
package.yml
34

Why are you removing the apps icons only to remove the entire folder and its subcontents? Why not just remove the 64x64 folder to begin with?

This revision now requires changes to proceed.Nov 23 2020, 3:00 PM
aleksvor added inline comments.Nov 23 2020, 3:08 PM
package.yml
34

Just for readability - so other packagers can easily understand which files exactly are being removed. I can return it to removing the whole folder recursively.

aleksvor updated this revision to Diff 24077.Nov 23 2020, 3:19 PM

Remove folder with unneeded icons with one command instead of two.

JoshStrobl requested changes to this revision.Nov 23 2020, 4:30 PM
JoshStrobl added inline comments.
package.yml
34

Yea I'd prefer we go with the more succinct removing. The comment above the rm's is self-explanatory.

This revision now requires changes to proceed.Nov 23 2020, 4:30 PM
aleksvor requested review of this revision.Nov 23 2020, 4:46 PM
aleksvor added inline comments.
package.yml
34

Not sure why you marked as "Requested changes" - I've already replaced 2 removal commands with one 'rm -rf' on entire folder?

JoshStrobl accepted this revision.Nov 23 2020, 5:30 PM

Accidental.

This revision is now accepted and ready to land.Nov 23 2020, 5:30 PM
This revision was automatically updated to reflect the committed changes.