Skip to content
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

Subtitles are rendered outside of videowindow control #15902

Closed
1 of 7 tasks
scott967 opened this issue Apr 11, 2019 · 15 comments · Fixed by #20010
Closed
1 of 7 tasks

Subtitles are rendered outside of videowindow control #15902

scott967 opened this issue Apr 11, 2019 · 15 comments · Fixed by #20010
Assignees
Labels
Component: Video rendering Triage: Confirmed issue has been reproduced by a team member v18 Leia

Comments

@scott967
Copy link
Contributor

Bug report

Describe the bug

Here is a clear and concise description of what the problem is:

When a videowindow control is used with dimensions smaller than full screen, subtitles (tested, external srt sub) are rendered outside of the videowindow and are not scaled to videowindow dimensions.

Expected Behavior

Here is a clear and concise description of what was expected to happen:

The displayed subtitle is rendered using the subtitle settings and scaled/positioned within the dimensions of the videowindow control as can be observed in Kodi 17.

Actual Behavior

The subtitle is rendered full-size (at the display resolution/aspect). It appears to be positioned horizontally correctly, but vertically it is at the bottom of the screen, not within the videowindow control.

Possible Fix

To Reproduce

Steps to reproduce the behavior:

  1. Add video with external subtitle (Tested: episode of tv show tested: srt format)
  2. Start playback of video with subtitle enabled
  3. View playing video in a skin window with a videowindow control (tested: Custom window in estuary:
    https://pastebin.com/6Zsdc2cH

When settings/player/language subtitle position is set to "fixed", the subtitle is rendered in the proper x-position but at the bottom of the screen, not the bottom of the videowindow control as experienced in Kodi 17. In addition, the subtitles are not scaled to the videowindow control dimensions.

If the language settings are changed to subtitle position "bottom of video", the subtitles are rendered within the videowindow control, but are not scaled.

Tested in both full screen window and true full screen with DXVA/pixel shaders/software rendering. All other system display settings are default.

Debuglog

The debuglog can be found here:
https://pastebin.com/pKjDPUm6

Screenshots

Here are some links or screenshots to help explain the problem:

Sub position fixed:

Sub position bottom of video:

Kodi 17 Sub position fixed comparison:

Additional context or screenshots (if appropriate)

Here is some additional context or explanation that might help:

Your Environment

Used Operating system:

  • Android

  • iOS

  • Linux

  • OSX

  • Raspberry-Pi

  • Windows

  • Windows UWP

  • Operating system version/name: Win 7 SP1 x64

  • Kodi version: 18.2 RC1 190410

note: Once the issue is made we require you to update it with new information or Kodi versions should that be required.
Team Kodi will consider your problem report however, we will not make any promises the problem will be solved.

@xbmc-gh-bot xbmc-gh-bot bot added the Triage: Needed (managed by bot!) issue that was just created and needs someone looking at it label Apr 11, 2019
@Leatherface75
Copy link

I think this problem started after that commit for background for subtitles.
Seems to be ok for DVB subtitles for not for other types of subtitles.

@enen92
Copy link
Member

enen92 commented Apr 12, 2019

If that's the case I'll look into it and try to come up with a fix.

@enen92 enen92 self-assigned this Apr 12, 2019
@FernetMenta
Copy link
Contributor

this fixes position. scaling is a different story

--- a/xbmc/cores/VideoPlayer/VideoRenderers/OverlayRenderer.cpp
+++ b/xbmc/cores/VideoPlayer/VideoRenderers/OverlayRenderer.cpp
@@ -275,7 +275,7 @@ void CRenderer::Render(COverlay* o, float adjust_height)
       {
         RESOLUTION_INFO res = CServiceBroker::GetWinSystem()->GetGfxContext().GetResInfo(CServiceBroker::GetWinSystem()->GetGfxContext().GetVideoResolution());
         state.x += m_rv.x1 + m_rv.Width() * 0.5f;
-        state.y += m_rv.y1  + (res.iSubtitles - res.Overscan.top);
+        state.y += m_rv.y1  + (res.iSubtitles - res.Overscan.top) * (m_rv.y2 - m_rv.y1) / res.iHeight;
       }
       else
       {

@enen92
Copy link
Member

enen92 commented Apr 14, 2019

This got broken in #14371 due to the code removed in overlayrenderergui.cpp. Scalling is easily solvable by bringing back the old code. Thanks for helping with a patch. I got the impression that using res and overscan was wrong in favor of using viewrect, destrect, sourcerect, etc. But it looks like none take into account the full dimensions of the window but just the videocontrol

@FernetMenta
Copy link
Contributor

I got the impression that using res and overscan was wrong in favor of using viewrect, destrect, sourcerect,

Right, but here res is taken from gfx context, which does not know about the view,dest,source rect.

But it looks like none take into account the full dimensions of the window but just the videocontrol

Not quire right.
(m_rv.y2 - m_rv.y1) / res.iHeight
evaluates to 1 on fullsceen window.

@enen92 enen92 added Component: Video rendering Triage: Confirmed issue has been reproduced by a team member v18 Leia and removed Triage: Needed (managed by bot!) issue that was just created and needs someone looking at it labels Apr 14, 2019
@FernetMenta
Copy link
Contributor

@enen92 you are right, now I remember. The idea was getting rid of those calls to gfx context. In order to solve this, subtitle pos and scaling should be set like video rects in SetVideoRect.

@enen92
Copy link
Member

enen92 commented Apr 15, 2019

That was my idea since SetVideoRect is called before PrepareRender. Problem is that I take loads of time to do simple things in Kodi and haven't yet found the time to work on it. I'll try today and ping you for review if you are available. Thanks

@enen92
Copy link
Member

enen92 commented Apr 18, 2019

@FernetMenta a small question. Even if we treat the scale and the position like video rects the scalling operation will still require a call to gfx context (set transform) in overlayrendergui or am I wrong? Or should that be done before?

@FernetMenta
Copy link
Contributor

@enen92 text subtitles do not follow the schema of the other types and violate the design of rendering of video player. They delegate rendering to some gui component which expects this kind of transformation matrix. For gui this is done at the beginning of gui rendering but here we see an ugly hack that lacks any sense of a reasonable design.
The design of overlays in vp is as follows:

  1. have an object that holds the overlay information
  2. convert this info into a gl texture
  3. render the texture

I would implement 2) for text subtitles with a "convert" function (see other types).

@Leatherface75
Copy link

Any solution yet?

@enen92
Copy link
Member

enen92 commented May 12, 2019

@FernetMenta I had a look into it and it seems the COverlayRenderGUI.cpp is just legacy code, a GUI component completely tied to the player and with no control from the Overlayrenderer. As far as I could see there is no easy way to get the texture to render it later from the overlayrenderer. While at it, I had a look into libass which seems to provide all the available options we support right now for text overlays and might provide even more in the future (shadows, etc). What do you think of dropping this GUI component and relying only on libass for text overlay rendering in the player? Would this be something you would "approve"/support?

Seems like mpv does the same, creating events for each text overlay: https://github.com/mpv-player/mpv/blob/802f594a857c703ac88e946d14b69cd3b6eb6006/sub/osd_libass.c#L163

Best regards

@FernetMenta
Copy link
Contributor

What do you think of dropping this GUI component and relying only on libass for text overlay rendering in the player?

Sounds like an excellent plan to me.

@Leatherface75
Copy link

But i guess that's something for Kodi 19.
Would be nice with a fix for Leia too.

@enen92
Copy link
Member

enen92 commented May 12, 2019

@Leatherface75 unfortunately you either fix the scalling and break the subtitle background or leave it as it is. Another option is to disable the background if the video control is not in fullscreen. Either of the options sounds like a waste of effort, better fix it completely in v19.

@Leatherface75
Copy link

Leatherface75 commented May 12, 2019

Your idea with disable background works for me.
I am not using backgrounds anyway and right now subtitles is totally broken when not in fullscreen.
If you have to choose it's better with working subtitles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Video rendering Triage: Confirmed issue has been reproduced by a team member v18 Leia
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants