-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add move functionality #12
Conversation
Thank you, the idea is sound but it's based on some assumption that are not true, for example your solution will only work when all subpicture are close to the same border and are outside the new screen area, in my experience subtitle may be stationed near all corner of the screen, on the middle and not always the subpic is outside the screen area, for example on a 1920x1080->1920x800 conversion the subtitle may already be on the x800 section, and in that case we won't need to move them. What you are looking for is something similar to what you have implemented but based on a different working principle. What do you think?
Could you send me the original subtitle file and the crop parameter you are applying? In theory the crop functionality was implemented to AVOID such a behavior from the video player |
|
||
if (object->objectCroppedFlag == 0x40) { | ||
object->objCropHorPos += clampedDeltaX; | ||
object->objCropVerPos += clampedDeltaY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never seen objCropHorPos and objCropVerPos used in the wild, always being filled with a fixed value, I think you should check if those field are at a default and only change them otherwise, personally I'd also output a warning when changing those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that the objectCroppedFlag
being set to enabled (0x40
) indicated that these aren't set to default values (0
?) and should be respected.
In that case, they are some absolute coordinates which should be translated like the subpic itself to keep their relative positions and thus leave the crop unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experience that flag is set when the subpic is forced and if you check in ReadPCS and WritePCS the handling of those fields is commented out, i can share you this file which was sent to me to debug this flag behavior, if I remember correctly I never found those field populated, but I may be wrong
testcropped.zip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I missed that these fields aren't even read/written currently. Then it's better to leave this to a different PR, perhaps.
I will print a warning instead.
A visual example to illustrate what my issue is and why the current
|
I've analyzed the behavior of the player, those are scaling the internal subtitle resolution to the video resolution, VLC does an uneven scaling while other seems to do a proportional scaling, hence the stretched vertically subtitle.
For that i'll need to experiment, it is indeed possible that my idea does not handle all the case, i think your idea of "moving" everything towards the center may be the best one, but, again, i'll need to thinker with it a bit to understand how to best handle the baselines issue.
Yeah, your implementation is doing exactly what you're asking and will work fine when: 1) you are not cropping the video 2) the subtitle position subpic only on one side on the screen; in all other case it may not do what one expect. In any case i don't see any issue in merging it, thank you for your contribution and my apologies for those wall of text :) |
Now I get how the My workflow is different (which is probably why I made wrong assumptions), I just mux it with the original uncropped 1920x1080 video again and set MKV cropping metadata. Then some time later I let ffmpeg automatically overlay video and subtitle, burn it into the video and then apply the crop.
Yeah, it's not really intelligent but rather quite "low level".
No worries, it was fun and I learned something about how players handle PGS subs :) |
Nope, the stretching is done by the player to match the subtitle resolution to the video resolution, the subtitle picture are maintained unchanged by this application, as you correctly stated at the end, only moved according to the crop/move parameters.
I may borrow that one ;)
That's the main reason why I've implemented the crop functionality, before that i used BDSup2Sub but it can introduce some artifact as it decodes the image to png, scale it and then reconvert it to sup, as sup are limited to 256 color palettes a lot of quantization error were introduced, from invisible to extremely noticeable, and in some cases even breaking the images |
Added a new
move
mode for shifting the position of all windows and their composition objects by the specified delta amount, clamped to keep the window rect fully contained within the screen rect.Should work with crop flag enabled as well, but not tested.
Motivation for adding this mode was the issue that e.g. moving subtitles out of the lower letterbox area with the current
crop
mode results in all images having their bottom edge aligned with the bottom edge of the crop rect, which doesn't necessarily keep the relative position of subtitle lines.This means that baselines of consecutive subtitle lines aren't always aligned (e.g. if one line has a letter with a descender — like lowercase g — while the next line doesn't), which negatively effects reading experience.
This new
move
mode avoids this issue by keeping the relative positions of images the same (unless it hits a screen edge).(I also observed that the current
crop
mode also changes the scaling of the images in most players in addition to the position which is not acceptable in my case)