bitmovin-player-ui icon indicating copy to clipboard operation
bitmovin-player-ui copied to clipboard

Improper subtitle vertical sizing/positioning

Open squarebracket opened this issue 6 years ago • 9 comments

The commits merged in from #30 introduced some problems with captions overlapping each other vertically in some conditions. For example, scaling down the window vertically:

screenshot from 2017-10-16 12-52-08

Additionally, applying a black background causes lots of problems even when text is spaced out enough, since the text boxes are so close to one another: screenshot from 2017-10-16 12-59-05 screenshot from 2017-10-16 12-58-42

The subtitles should really be spaced out enough to allow for styling, and the vertical size of the video should be taken into account when sizing the subtitles.

squarebracket avatar Oct 16 '17 17:10 squarebracket

CEA-608 subtitle positioning was generally designed for a 4:3 display so it's rather difficult to get it render correctly in arbitrary aspect ratios. If we decrease the font size (font height), then the text width is too narrow.

Do you need the positioning, or would it be an option for you if you could turn off the positioning and have the subtitles rendered as in previous UI versions?

protyposis avatar Oct 16 '17 17:10 protyposis

Positioning would obviously be preferable since there is important information contained in the positioning, such as who is speaking.

I'm not really sure why you think this has to do with the aspect ratio? It clearly happens for 4:3 feeds: screenshot from 2017-10-16 13-37-38

Unless you mean it's difficult to calculate the size for subtitles for arbitrary <video> sizes. Though since the sizing works properly horizontally, I feel like it wouldn't be too hard to extend it to work vertically too -- you just use the smaller dimension as the anchor for the calculation.

squarebracket avatar Oct 16 '17 17:10 squarebracket

Yes the current implementation is definitely not ideal... I'll look into improvements, we'll find a solution.

protyposis avatar Oct 16 '17 18:10 protyposis

So, I think the overlapping text issue is about to be solved.

Old

cea-old2 cea-old2_bg

New

cea-new2 cea-new2_bg

What do you think? That's about as good as it will get. As you can see you can add a background color behind the text, but it's still not possible to pad the background (that will still lead to overlapping backgrounds). Adjusting the text size to accomodate the padding screws up the CEA-608 character grid so you'll have to disable that for CEA-608 captions via CSS. You can still keep the padding for non-CEA subtitles.

protyposis avatar Oct 17 '17 18:10 protyposis

Did a checkout to 8826a96, looks like now the positioning is off: screenshot from 2017-10-17 17-41-43

squarebracket avatar Oct 17 '17 21:10 squarebracket

roll-up is also causing problems: screenshot from 2017-10-17 17-51-28

squarebracket avatar Oct 17 '17 21:10 squarebracket

Works correctly for me. Please make sure you don't have any interfering styles, especially since this PR changes the default style so it won't work correctly with an old CSS.

image

protyposis avatar Oct 17 '17 22:10 protyposis

Positions were undefined in some cases, causing positioning issues. It is yet to be determined if that is a player or stream issue, but I anyway added a fix into the UI to always display them correctly.

image

Edit: also works for weird player aspect ratios: image

protyposis avatar Oct 17 '17 22:10 protyposis

Positions were undefined in some cases

It's probably the CEA608 parser.

Please make sure you don't have any interfering styles

The only CSS I have:

.bmpui-ui-skin-modern .bmpui-ui-subtitle-overlay .bmpui-ui-subtitle-label {
    background-color: black;
    color: white;
}

I rebuilt using a72cf76. Still see the issues. I guess I'll have to take a look at the code.

squarebracket avatar Oct 18 '17 18:10 squarebracket