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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

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

Remove unneeded pkgconfig(xcb).

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
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.

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

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 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?

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.