[Subtitles] Avoid using res and overscan (use rectview instead) #14371

enen92 · 2018-08-28T17:50:36Z

Description

Overscan was not being taken into account when drawing the subtitle background causing the background to deviate from the rendered subs.

Motivation and Context

Fix behaviour. Some old tv models force us to use overscan to fix the screen positioning. If a user in those conditions defines a non-opaque background for subtitles, the end result is a mess.

How Has This Been Tested?

Compiled, run time tested, multiple variations of overscan settings, old tv as secondary monitor.

Screenshots (if appropriate):

Master:

This PR:

Types of change

Bug fix (non-breaking change which fixes an issue)
Clean up (non-breaking change which removes non-working, unmaintained functionality)
Improvement (non-breaking change which improves existing functionality)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that will cause existing functionality to change)
Cosmetic change (non-breaking change that doesn't touch code)
None of the above (please explain below)

Checklist:

My code follows the Code Guidelines of this project
My change requires a change to the documentation, either Doxygen or wiki
I have updated the documentation accordingly
I have read the Contributing document
I have added tests to cover my change
All new and existing tests passed

FernetMenta

Using res and overscan is wrong because it breaks windowed mode. RectView and RectDest do already consider overscan. Means you have to fix the old code

enen92 · 2018-08-29T09:28:18Z

res.Overscan is already used above to calculate width_max and y. That being said, what exactly do you mean by old code? Do you mean my prior implementation was correct and is somehow affected by those values (which should be fixed)? Or that I should leave the code above as it is and rely on another approach (RectView and RectDest) instead of using overscan only for the background/Rectangle? Sorry, but it's hard to read between the lines.
Regards

FernetMenta · 2018-08-29T12:24:13Z

res.Overscan is already used above to calculate width_max and y.

This is wrong and this is what i mean with old code

enen92 · 2018-08-29T12:26:33Z

Ok, I'll give it a go

enen92 · 2018-10-15T20:44:15Z

@FernetMenta thanks. Can you check if this is correct now?

xbmc/cores/VideoPlayer/VideoRenderers/OverlayRenderer.cpp

@@ -432,7 +432,11 @@ COverlay* CRenderer::Convert(CDVDOverlay* o, double pts)
 #endif

  if(!r && o->IsOverlayType(DVDOVERLAY_TYPE_TEXT))
-    r = new COverlayText(static_cast<CDVDOverlayText*>(o));
+  {
+    int targetWidth = MathUtils::round_int(m_rv.Width());


xbmc/cores/VideoPlayer/VideoRenderers/OverlayRenderer.cpp

+  {
+    int targetWidth = MathUtils::round_int(m_rv.Width());
+    int targetHeight = MathUtils::round_int(m_rv.Height());
+    r = new COverlayText(static_cast<CDVDOverlayText*>(o), targetWidth, targetHeight);


xbmc/cores/VideoPlayer/VideoRenderers/OverlayRendererGUI.cpp

@@ -136,6 +136,10 @@ COverlayText::COverlayText(CDVDOverlayText * src)

  m_type = TYPE_GUITEXT;

+  // entire area of screen


enen92 · 2018-10-23T09:05:20Z

@FernetMenta mind taking another look? :)

FernetMenta · 2018-10-24T05:28:08Z

looks good. just one minor, see comment

xbmc/cores/VideoPlayer/VideoRenderers/OverlayRendererGUI.cpp

@@ -136,6 +136,8 @@ COverlayText::COverlayText(CDVDOverlayText * src)

  m_type = TYPE_GUITEXT;

+  // target rect (updated on PrepareRender)
+  m_rv = CRect(0, 0, 0, 0);


enen92 · 2018-10-24T12:38:21Z

Comment addressed, should be all good now. Thanks a lot (again) for all your comments!
regards

enen92 added Type: Fix non-breaking change which fixes an issue Type: Improvement non-breaking change which improves existing functionality Component: Video rendering v18 Leia labels Aug 28, 2018

enen92 requested a review from FernetMenta August 28, 2018 17:50

FernetMenta reviewed Aug 29, 2018

View reviewed changes

enen92 force-pushed the overscansubs branch from f802e96 to fa2f9e7 Compare October 15, 2018 20:22

enen92 changed the title ~~[Subtitles] Respect overscan when drawing the subtitles background~~ [Subtitles] Avoid using res and overscan (use rectview instead) Oct 15, 2018

FernetMenta reviewed Oct 16, 2018

View reviewed changes

xbmc/cores/VideoPlayer/VideoRenderers/OverlayRendererGUI.cpp Outdated

@@ -136,6 +136,10 @@ COverlayText::COverlayText(CDVDOverlayText * src)

m_type = TYPE_GUITEXT;

// entire area of screen

This comment was marked as spam.

Sign in to view

enen92 added the WIP PR that is still being worked on label Oct 19, 2018

enen92 force-pushed the overscansubs branch from fa2f9e7 to 1413edb Compare October 22, 2018 16:58

enen92 added Type: Cleanup non-breaking change which removes non-working or unmaintained functionality and removed WIP PR that is still being worked on labels Oct 22, 2018

enen92 force-pushed the overscansubs branch from 1413edb to 6273f0d Compare October 22, 2018 17:16

FernetMenta reviewed Oct 24, 2018

View reviewed changes

xbmc/cores/VideoPlayer/VideoRenderers/OverlayRendererGUI.cpp Outdated

@@ -136,6 +136,8 @@ COverlayText::COverlayText(CDVDOverlayText * src)

m_type = TYPE_GUITEXT;

// target rect (updated on PrepareRender)

m_rv = CRect(0, 0, 0, 0);

This comment was marked as spam.

Sign in to view

[Subtitles] Avoid using res and overscan (use rectview instead)

99a9025

enen92 force-pushed the overscansubs branch from 6273f0d to 99a9025 Compare October 24, 2018 11:40

MartijnKaijser added this to the Leia 18.0-beta5 milestone Oct 28, 2018

MartijnKaijser merged commit c929afb into xbmc:master Oct 28, 2018

enen92 mentioned this pull request Apr 14, 2019

Subtitles are rendered outside of videowindow control #15902

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Subtitles] Avoid using res and overscan (use rectview instead) #14371

[Subtitles] Avoid using res and overscan (use rectview instead) #14371

enen92 commented Aug 28, 2018 •

edited

Loading

FernetMenta left a comment

enen92 commented Aug 29, 2018 •

edited

Loading

FernetMenta commented Aug 29, 2018

enen92 commented Aug 29, 2018 •

edited

Loading

enen92 commented Oct 15, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

enen92 commented Oct 23, 2018

FernetMenta commented Oct 24, 2018

This comment was marked as spam.

enen92 commented Oct 24, 2018

		@@ -136,6 +136,10 @@ COverlayText::COverlayText(CDVDOverlayText * src)

		m_type = TYPE_GUITEXT;

		// entire area of screen

[Subtitles] Avoid using res and overscan (use rectview instead) #14371

[Subtitles] Avoid using res and overscan (use rectview instead) #14371

Conversation

enen92 commented Aug 28, 2018 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

Checklist:

FernetMenta left a comment

Choose a reason for hiding this comment

enen92 commented Aug 29, 2018 • edited Loading

FernetMenta commented Aug 29, 2018

enen92 commented Aug 29, 2018 • edited Loading

enen92 commented Oct 15, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

enen92 commented Oct 23, 2018

FernetMenta commented Oct 24, 2018

This comment was marked as spam.

enen92 commented Oct 24, 2018

enen92 commented Aug 28, 2018 •

edited

Loading

enen92 commented Aug 29, 2018 •

edited

Loading

enen92 commented Aug 29, 2018 •

edited

Loading