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

[GLES] Improve subtitle rendering #15518

Merged
merged 1 commit into from Feb 28, 2019

Conversation

Projects
None yet
10 participants
@kszaq
Copy link
Contributor

commented Feb 14, 2019

Description

Most GLES hardware support BGRA textures via an extension. If supported, use it to avoid doing malloc for convering textures.

Motivation and Context

With some kind of subtitles there is a visible frame skip when subtitles are rendered. This is especially visible when subtitle is a 1920x1080 picture:
https://forum.kodi.tv/showthread.php?tid=315216
https://forum.libreelec.tv/thread/8100-bug-s905x-krypton-frame-skips-caused-by-burned-in-forced-subtitles/

How Has This Been Tested?

LibreELEC on Amlogic S905 (GLES 2.0 with GL_EXT_texture_format_BGRA8888) and S912 (GLES 3.0 and GL_EXT_texture_format_BGRA8888)

Screenshots (if appropriate):

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

@a1rwulf a1rwulf requested a review from lrusak Feb 15, 2019

@wrxtasy

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2019

Nice work @kszaq
Tested with Linux (CE) Kodi Leia - MINIX U9: (GLES 3.0)

Yes this make a noticeable difference when Overlays like Subtitles are first rendered onscreen.
The video frame skip is no longer there after this PR.

@lrusak

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2019

How is this any different then #15505 ?? It wasn't accepted there, why do you think it would be accepted again? 😕

@kszaq

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2019

#15505 was a workaround for broken headers, this PR introduces suport for unpack and BGRA extensions to OverlayRenderGL. I think I described it quite thoroughly.

Your comments from #15505 have been addressed.

@lrusak

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2019

#15505 was a workaround for broken headers, this PR introduces suport for unpack and BGRA extensions to OverlayRenderGL. I think I described it quite thoroughly.

Your comments from #15505 have been addressed.

You are still working around broken headers by defining GL defines. Also the extension GL_EXT_unpack_subimage only adds support for the _EXT versions like GL_UNPACK_ROW_LENGTH_EXT and not GL_UNPACK_ROW_LENGTH. Even though they both have the same definition one is defined in GLES2/gl2ext.h and the other in GLES3/gl3.h and I'd rather be explicit about it.

Overall I'd really rather wait for any more GLES3 changes until #13457 is merged and we can split GLES2 and GLES3 apart.

@kszaq kszaq force-pushed the kszaq:gles_unpack branch from 9a7d54b to 4915a9d Feb 16, 2019

@kszaq

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2019

@lrusak Understood. I have now adjusted the PR to only add support for BGRA textures in OverlayRendererGL for GLES. This also solves the frame drop issue.

[GLES] OverlayRendererGL: add support for BGRA extensions for GLES
Most GLES hardware support BGRA textures via an extension. If supported,
use it to avoid doing malloc for convering textures.

@kszaq kszaq force-pushed the kszaq:gles_unpack branch from 4915a9d to 82e9438 Feb 17, 2019

@popcornmix

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

@kszaq do you have a link to a sample file that shows the problem? (linked threads have dead download links).
I'd like to test it on a Pi.

@kszaq

This comment has been minimized.

@popcornmix

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

GL_EXT_texture_format_BGRA8888 is supported on Pi.

The shortcut this PR provides by using the extension saves ~36ms per new subtitle of this style on a Pi3 which is likely to avoid a frame drop when rendering a new sub.

So, +1 from me.

@smp79

This comment has been minimized.

Copy link

commented Feb 18, 2019

@popcornmix Another sample that is causing frame drops on Pi 3 when subtitles are enabled. I see no improvement with this PR.

@popcornmix

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

It looks like this PR saves ~63ms per new subtitle which would be about 1.5 frames skipped.
But there is still other work being done in the render thread which means we are still skipping some frames.
@kszaq does this sample play okay for you?

@kszaq

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

@popcornmix Thank you for testing and sharing your results!

@smp79 With your sample a single frame is dropped every time a new subtitle is rendered. Without this PR 1-3 frames are dropped every subtitle rendered.

@popcornmix

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

@kszaq have you been looking at this code closely enough to say where the time is spent that causes the remaining frame drop? I'm guessing it's freetype, but I haven't checked for sure.

Just wondering how long it takes and if there's anything more that can be done.

@kszaq

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

@popcornmix No, I haven't. Perhaps in a few days...

@kszaq

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

My guess is that rendering subtitles unnecessarily block rendering thread. While this PR speeds up rendering, I suspect it would be best to not let overlay renderer block video rendering, late subtitle is not an issue while a dropped frame is. Unfortunately I have no idea how to achieve this.

@popcornmix

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Yes anything that takes significant time (a reasonable fraction of a frame time) shouldn't run from render thread but a separate thread. I suspect the code that generates bitmaps from freetype fonts is called from render thread but I haven't investigated.

The test is, if commenting out that call removes the remaining frame skips.

If it does then launching it as a background job and only rendering it when complete (e.g. a few frames later) would be a preferable solution.

@kszaq

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

In both samples subtitles are in PGS format, so bitmaps and it looks like bitmap conversion and rendering blocks video rendering thread for too long. When I moved convert_rgba from OverlayRenderer to DVDOverlayCodecFFmpeg with sample from @smp79 I had no frames dropped but this is certainly not a proper solution. Unfortunately my C++ skills end on modifying single files and I have no idea how to implement threading for rendering subtitles.

@afl1

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

I came to the same result as @popcornmix when I did subtitles issue investigations. Rendering subtitles need own thread and only when rendering is completed can be overlayed. Currently subtitles rendering is done in main thread eating time from frame playback. I think this is principial bug and have to be fix by kodi developers. Any other partial fix is only patching this issue.

@afl1

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

I think there is not only issue with subtitles. Also OSD have to be rendered in in extra thread or subtitles and OSD can be done asynchronously to frame rendering in extra thread.

@pkerling

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

I came to the same result as @popcornmix when I did subtitles issue investigations. Rendering subtitles need own thread and only when rendering is completed can be overlayed.

As said in the thread, this wouldn't change anything for bitmap subs which already seem to be problematic.

Any other partial fix is only patching this issue.

See above - this is orthogonal. There is no reason to shuffle data around needlessly no matter where and how it is rendered.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Rendering subtitles need own thread and only when rendering is completed can be overlayed

Rendering can only be done by the render thread. That's how GL/GLES work. (maybe you have your own definition of rendering). Texture loading can be offloaded to another thread, if this thread shares the GL context with the render thread.

@pkerling

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

(maybe you have your own definition of rendering)

I think it was concerning font rendering (FreeType)

@kszaq

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

@lrusak Is there any other change I should make to this PR?

@lrusak

lrusak approved these changes Feb 28, 2019

Copy link
Contributor

left a comment

I'm fine with this band-aid if @MartijnKaijser wants to pull this in for the next release. Otherwise it can go into V19

@kszaq

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

Thank you.

I don't consider this a band-aid as even if the root cause of frame drop is solved, it's always nice to save ca. 50 ms of CPU time on unnecessary conversion.

@popcornmix

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

I'm in favour of getting this in. In some situations it will avoid the frame skip when a new subtitle appears. In others it should reduce it. More can obviously be done but this is a nice improvement.

@MartijnKaijser MartijnKaijser merged commit f7242ad into xbmc:master Feb 28, 2019

1 check passed

default You're awesome. Have a cookie
Details

@Rechi Rechi added this to the Leia 18.2-rc1 milestone Mar 1, 2019

@kszaq kszaq deleted the kszaq:gles_unpack branch Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.