CineEncoder / cine-encoder

Cine Encoder is an application that allow to convert media files while preserving HDR metadata.
GNU General Public License v3.0
98 stars 12 forks source link

Migrate from QString to QStringLists for settings, UI elements for hard-burn of subtitles (font, background) #70

Closed philstopford closed 1 year ago

philstopford commented 1 year ago

This PR started with adding UI elements to configure hard-burning of subtitles. In the process of making this work (with some notes below), I discovered why the tool was failing with inputs containing spaces and went ahead and fixed that.

The space race:

The tool previously compiled a string representation of all arguments that were to be passed to ffmpeg. This required careful tracking of trailing spaces, etc. and eventually split(" ") was called to break the string into a list of arguments. Naturally, if there were needed-spaces (e.g. -i /my path/to myfile.srt), this would break. It was also very fragile if multiple spaces were presented (" ") because the act of splitting would cause arguments to have empty entries and the invocation of ffmpeg would then fail To fix this up, I went through and moved everything to QStringLists. This gives natural separation because you have one argument or value per list entry. This also makes it easy to clean up empty slots (RemoveAll(" ")) and makes it also trivial to compile a string representation using .join(" ").

I've left some string/split usage in place where it was dealing with hard-coded stuff (e.g. PRESETS), but that may be nice to migrate at some point.

In testing, this now reliably works with subtitles, including hard-burn, where the path contains spaces. Very exciting. This was only tested on Linux.

There are remaining issues:

I also plan to add support for defining the position of the subtitles based on a fixed set of choices. It will be the focus of the next iteration of this WIP.

philstopford commented 1 year ago

Subtitle location is now, I hope, supported. I've also refactored a lot of the encoder code because the function was very long and difficult to navigate. Each 'module' is now broken out into its own method to reduce the confusion factor.

This probably needs a good amount of testing to make sure I didn't screw something up.

philstopford commented 1 year ago

I should note that fonts with spaces also work with this. One minor thing that I'd like to understand/have help with is to get the main application font (e.g. Noto Sans in my case) assigned as the default subtitle font absent user change. At the moment, this isn't happening and I think I'm missing something obvious/silly.