Support LayoutRes and rework YCbCr Matrix handling#594
Open
arch1t3cht wants to merge 23 commits into
Open
Conversation
arch1t3cht
force-pushed
the
layoutres
branch
3 times, most recently
from
c335253 to
d5f64b1
Compare
arch1t3cht
force-pushed
the
layoutres
branch
3 times, most recently
from
771d9a0 to
733e272
Compare
No longer needed after 372b9fe.
Initial support for LayoutRes. Allow editing the values in the properties and respect them in the visual tools. However, resolution resampling and the resolution mismatch dialogs still need to be adjusted.
Add a class agi::ycbcr::Header for all the color matrix/space/YCbCr Matrix header juggling, and move it outside of the ffms2 video provider. Respect all YCbCr Matrix values rather than only a 601 matrix, and stop erroring out on unknown video color spaces. The override logic in the ffms2 video provider follows the color mangling rules to the letter, in particular also enforcing a BT.601 matrix when the YCbCr Matrix header is unset. Following commits will then warn the user when this happens.
Unify the two YCbCrMatrix types as far as possible and improve the defaults for resampling the YCbCr Matrix: Only suggest to resample if the video's color space is known and the script's matrix is not None.
...and add an explicit "Leave the file as is" option. "Set to video resolution" apparently never worked in the first place and just also resampled the subtitles. Since changing PlayRes without adjusting any subtitles is never ever correct except when the input file is already broken in some way (in which case it can still be emulated by selecting "Leave the file as is" and then manually setting the PlayRes in the script properties), so just remove that option. Instead, add an explicit option to leave the file as is (which previously could only be achieved with "Cancel"), which should confuse users slightly less.
Following the previous commit, also remove the "Always Set" option in the preferences. Rename the relevant options to avoid any user settings changing due to the renumbering.
This is the last big part of the rework of the YCbCr Matrix handling. The general theme of the changes (and the incoming analogous additions of LayoutRes handling) is: - Do not (at least not by default) automatically change *any* fields of the file without notifying the user. - Select the "recommended" action (as far as we can tell with out limited information) as a default. - Show prompts for everything (which also has the benefit of teaching the user about these topics), but allow disabling them in the settings. Once Aegisub's website gets some love, there will hopefully be a more detailed guide on how to handle the YCbCr Matrix header there. The various different prompts add a fair bit of similar-looking boilerplate code (which will only get worse when additional prompts are added to handle LayoutRes), but I do believe that it is better to keep it this way instead of trying to add some common abstraction: This subject is enough of a headache as it is, and the current organization helps treat each case separately and reason better about which options should be available where.
And make the resolution box use a GridBagSizer in the process.
Now that opening a video file no longer automatically sets the YCbCr Matrix, this is no longer correct.
Now that Aegisub encourages setting LayoutRes (which will make subtitles scale sanely with aspect ratio differences), using the display resolution rather than the storage resolution will work without issues and result in saner coordinates, For example, text no longer needs to be horizontally scaled on anamorphic video to match the sample aspect ratio.
With LayoutRes being properly supported and handled now, resolution mismatches are no longer an issue most of the time. Users that want to be warned on mismatches can still enable the prompt in the settings. Similarly, default to "Leave resolution as is" rather than "Resample script" on the first invocation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These two seemingly different properties end up needing to be handled fairly similarly (ultimately because both of them specify properties of the video file the subtitles were authored for), which is why this ended up being one combined branch.
This is a big rework involving some tricky UX decisions so I will let people test this over at arch1t3cht/Aegisub for a while.
The main remaining thing to do is to properly support LayoutRes in the resolution resampler. The main difficulty here is figuring out what this is even supposed to mean, since the resolution resampler (mainly) resamples PlayRes, which is orthogonal to resampling LayoutRes. But the resolution resampler also offers some aspect ratio resampling, which is supposed to be LayoutRes's domain now.
I'll have to carefully think about all the cases involved here before tackling this, so I'll postpone it for now so that the remaining changes can be tested.