Libass #942

Merged
merged 3 commits into from May 8, 2012

Conversation

Projects
None yet
6 participants
@elupus
Member

elupus commented May 6, 2012

Use the libass changed flag during rendering to avoid re-uploading glyphs on each frame when nothing changed.

elupus added some commits May 6, 2012

[overlays] always run overlay conversion routine
This allows the conversion rutines to decide
if the previously generated gpu overlay is
still usable.
[libass] don't re-uploading glyphs to GPU if not changed
This uses the changed flag from libass to avoid uploading
the texture containing glyphs and the vertex buffer with
positions on every renderered frame.

This will not avoid upload in the case of only position
change which should be possible too.

@ghost ghost assigned elupus May 6, 2012

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus May 6, 2012

Member

I would like to get this in this merge window if possible. But would need some testing on opengl, have only tested win32 and build tested linux. Testing this on some low perf platform would be nice.

Member

elupus commented May 6, 2012

I would like to get this in this merge window if possible. But would need some testing on opengl, have only tested win32 and build tested linux. Testing this on some low perf platform would be nice.

@Memphiz

This comment has been minimized.

Show comment Hide comment
@Memphiz

Memphiz May 7, 2012

Member

This also affects ios i guess? Have it on my testing - todo -list for after work ...

Member

Memphiz commented May 7, 2012

This also affects ios i guess? Have it on my testing - todo -list for after work ...

@alanwww1

This comment has been minimized.

Show comment Hide comment
@alanwww1

alanwww1 May 7, 2012

Member

@elupus: I'd test it on Linux, just tell me what to look at or what to pay attention for.

Thanks

Member

alanwww1 commented May 7, 2012

@elupus: I'd test it on Linux, just tell me what to look at or what to pay attention for.

Thanks

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus May 7, 2012

Member

Verify that ass subtitles still work and still animate properly. Check cpu
/ gpu usage when subtitles är visible on screen. It should be lower as long
as they are static. Sadly even if only part of the sub is animated, it will
be as bad as before.

Member

elupus commented May 7, 2012

Verify that ass subtitles still work and still animate properly. Check cpu
/ gpu usage when subtitles är visible on screen. It should be lower as long
as they are static. Sadly even if only part of the sub is animated, it will
be as bad as before.

@Memphiz

This comment has been minimized.

Show comment Hide comment
@Memphiz

Memphiz May 7, 2012

Member

Thumbs up from me. I think you've got around 5000 new friends from the iOS fanbase with this commit :). At least on my problematic testfiles this commits fix the cpu peak out / framerate dropping. Great :)

We should stick this in in may, when the other platform testers give a sign-off too.

Member

Memphiz commented May 7, 2012

Thumbs up from me. I think you've got around 5000 new friends from the iOS fanbase with this commit :). At least on my problematic testfiles this commits fix the cpu peak out / framerate dropping. Great :)

We should stick this in in may, when the other platform testers give a sign-off too.

@linusyang

This comment has been minimized.

Show comment Hide comment
@linusyang

linusyang May 8, 2012

Contributor

@Memphiz Good news for all iOS/ATV2 users!

Contributor

linusyang commented May 8, 2012

@Memphiz Good news for all iOS/ATV2 users!

elupus added a commit that referenced this pull request May 8, 2012

Merge pull request #942 from elupus/libass
[libass] cpu/gpu usage reduction by changed flag

Use the libass changed flag during rendering to avoid re-uploading glyphs on each frame when nothing changed.

@elupus elupus merged commit 6d4087c into xbmc:master May 8, 2012

@jpsdr

This comment has been minimized.

Show comment Hide comment
@jpsdr

jpsdr May 29, 2012

Contributor

I think the result is unfortunately this : http://trac.xbmc.org/ticket/13023

Contributor

jpsdr commented May 29, 2012

I think the result is unfortunately this : http://trac.xbmc.org/ticket/13023

@jpsdr

This comment has been minimized.

Show comment Hide comment
@jpsdr

jpsdr May 29, 2012

Contributor

I've removed (put in comment) the following code :
if(o->m_overlay)
{
if(changes == 0)
return o->m_overlay->Acquire();
}
in the file OverlayRenderer.cpp, and it solved the issue.
It seems that unfortunately the value of "changes" is not accurate enough... or things should be done differently.

Contributor

jpsdr commented May 29, 2012

I've removed (put in comment) the following code :
if(o->m_overlay)
{
if(changes == 0)
return o->m_overlay->Acquire();
}
in the file OverlayRenderer.cpp, and it solved the issue.
It seems that unfortunately the value of "changes" is not accurate enough... or things should be done differently.

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus May 29, 2012

Member

That screenshot was awesome. I now have a hunch what is going on. I think we are not removing old libass overlays. Previously that would not show since they would be rendered exactly on top of eachother.

This might finally explains the insane CPU usage from ass subtitles. I'll look at it later.

Member

elupus commented May 29, 2012

That screenshot was awesome. I now have a hunch what is going on. I think we are not removing old libass overlays. Previously that would not show since they would be rendered exactly on top of eachother.

This might finally explains the insane CPU usage from ass subtitles. I'll look at it later.

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus May 29, 2012

Member

Since my computer is insanely slow at the moment and download is taking forever. If you could try adding

remove = true;

at

https://github.com/xbmc/xbmc/blob/master/xbmc/cores/dvdplayer/DVDCodecs/Overlay/DVDOverlaySSA.h#L36

Member

elupus commented May 29, 2012

Since my computer is insanely slow at the moment and download is taking forever. If you could try adding

remove = true;

at

https://github.com/xbmc/xbmc/blob/master/xbmc/cores/dvdplayer/DVDCodecs/Overlay/DVDOverlaySSA.h#L36

@jpsdr

This comment has been minimized.

Show comment Hide comment
@jpsdr

jpsdr May 30, 2012

Contributor

Doing this ?
{
m_libass = libass;
remove = true;
libass->Acquire();
}
I'll try this evening, and report result.

Contributor

jpsdr commented May 30, 2012

Doing this ?
{
m_libass = libass;
remove = true;
libass->Acquire();
}
I'll try this evening, and report result.

@jpsdr

This comment has been minimized.

Show comment Hide comment
@jpsdr

jpsdr May 30, 2012

Contributor

... "remove" is underlined when i add it => unknow variable... So, i don't even try to compile. Are you sure it is the correct name or correct place ?

Contributor

jpsdr commented May 30, 2012

... "remove" is underlined when i add it => unknow variable... So, i don't even try to compile. Are you sure it is the correct name or correct place ?

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus May 30, 2012

Member

It could be "replace" too. You can check base class CDVDOverlay.
On May 30, 2012 6:45 PM, "jpsdr" <
reply@reply.github.com>
wrote:

... "remove" is underlined when i add it => unknow variable... So, i
don't even try to compile. Are you sure it is the correct name or correct
place ?


Reply to this email directly or view it on GitHub:
#942 (comment)

Member

elupus commented May 30, 2012

It could be "replace" too. You can check base class CDVDOverlay.
On May 30, 2012 6:45 PM, "jpsdr" <
reply@reply.github.com>
wrote:

... "remove" is underlined when i add it => unknow variable... So, i
don't even try to compile. Are you sure it is the correct name or correct
place ?


Reply to this email directly or view it on GitHub:
#942 (comment)

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus May 30, 2012

Member

I've added replace = true; in master and it indeed fixes the insane CPU usage and this bug. Probably should be backported to eden.

Member

elupus commented May 30, 2012

I've added replace = true; in master and it indeed fixes the insane CPU usage and this bug. Probably should be backported to eden.

@jpsdr

This comment has been minimized.

Show comment Hide comment
@jpsdr

jpsdr Sep 2, 2012

Contributor

Hello.
Something always bothering me since this commit, without realy being able to truly point it, until it "pops" to me playing one of the test file i regularly use for regression check. Basicaly, i had often the feeling that some subtitles disapeared "too soon". With that file, it has suddenly been obvious. You can get the file here : http://jpsdr.free.fr/XBMC/Test-4.rar
I've checked the file with mpc-hc, and subtitles are displayed properly, but not with xbmc. You can quickly check, at the begining, when "The Vajra are not our true ennemy." disapeared, karaoke at top left also disapeard, when it shouldn't. Also, when next karaoke begin, you can see during a very bref time (at leat on a TV at 24fps), something which is like the previous karaoke.

Contributor

jpsdr commented Sep 2, 2012

Hello.
Something always bothering me since this commit, without realy being able to truly point it, until it "pops" to me playing one of the test file i regularly use for regression check. Basicaly, i had often the feeling that some subtitles disapeared "too soon". With that file, it has suddenly been obvious. You can get the file here : http://jpsdr.free.fr/XBMC/Test-4.rar
I've checked the file with mpc-hc, and subtitles are displayed properly, but not with xbmc. You can quickly check, at the begining, when "The Vajra are not our true ennemy." disapeared, karaoke at top left also disapeard, when it shouldn't. Also, when next karaoke begin, you can see during a very bref time (at leat on a TV at 24fps), something which is like the previous karaoke.

@jpsdr

This comment has been minimized.

Show comment Hide comment
@jpsdr

jpsdr Sep 2, 2012

Contributor

I've made several tests.
I've first commented the following code :
if(o->m_overlay)
{
if(changes == 0)
return o->m_overlay->Acquire();
}
in the file OverlayRenderer.cpp
but, it doesn't solve the issue, display of subtitles was still broken.
Then, i've also commented the following : "replace = true" in DVDOverlaySSA.h, and it solved the issue.

Contributor

jpsdr commented Sep 2, 2012

I've made several tests.
I've first commented the following code :
if(o->m_overlay)
{
if(changes == 0)
return o->m_overlay->Acquire();
}
in the file OverlayRenderer.cpp
but, it doesn't solve the issue, display of subtitles was still broken.
Then, i've also commented the following : "replace = true" in DVDOverlaySSA.h, and it solved the issue.

@jpsdr

This comment has been minimized.

Show comment Hide comment
@jpsdr

jpsdr Sep 4, 2012

Contributor

I've opened a ticket : http://trac.xbmc.org/ticket/13302

Contributor

jpsdr commented Sep 4, 2012

I've opened a ticket : http://trac.xbmc.org/ticket/13302

@jester-xbmc

This comment has been minimized.

Show comment Hide comment
@jester-xbmc

jester-xbmc Sep 4, 2012

@jpsdr as you seem to have a working patch, can you (if you don't have one already) do a fork from XBMC on github, do the change and then do a PR (Pull Request) so it's easier for the dev's to pull it in (as this is a closed PR)
@elupus Also I remember there was a PR with backports from Eden, i'm not sure this one made it

@jpsdr as you seem to have a working patch, can you (if you don't have one already) do a fork from XBMC on github, do the change and then do a PR (Pull Request) so it's easier for the dev's to pull it in (as this is a closed PR)
@elupus Also I remember there was a PR with backports from Eden, i'm not sure this one made it

@jpsdr

This comment has been minimized.

Show comment Hide comment
@jpsdr

jpsdr Sep 4, 2012

Contributor

It's not from my point of view a "working" patch. It's just information for elupus, because what i've done is almost a total removing of all the work of this commit. A true real fix is above my knowledge, again, it's just the result of my experiments for when elupus will take a look at it.

Contributor

jpsdr commented Sep 4, 2012

It's not from my point of view a "working" patch. It's just information for elupus, because what i've done is almost a total removing of all the work of this commit. A true real fix is above my knowledge, again, it's just the result of my experiments for when elupus will take a look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment