OverlayContainer: release render resources on cleanup #2363
Conversation
|
This is the effect of the bug where subtitles become rectangles after GPU texture memory is full: I've tested this patch on the Pi and textures are now freed, and it fixes the linked to problem. |
|
Why are they elsewhere? , this indicate a leak. |
|
I have not figured out yet. I think subs are loaded from a file and kept in a collection in that place. Submitting them to OverlayContainer just increases the ref count. |
|
Oh right you are. There is a collection of all subs in there. Heh, odd this Ok to merge. |
|
@elupus I just noticed that this change is not thread safe. pOverlay->m_overlay might be accessed by the render thread while video thread calls SAFE_RELEASE on it. With regard on buffering it's not a good idea to release hw resources in that place. The object may still be referenced by OverlayRenderer. How do you think about getting a copy from CDVDSubtitleParserCollection::Parse ? |
|
We don't really release the resource though, only the reference. But yea that is a issue. Will ponder a bit. |
|
here is an alternative fix: FernetMenta@abb9a8c |
|
nah.. that looks messy. What types of subs are having this issue btw? All So I'm back to thinking this is some other problem at play here. On Sun, Mar 3, 2013 at 5:39 PM, Rainer Hochecker
|
|
Also that proposal breaks the whole idea to not re-upload static overlays On Sun, Mar 3, 2013 at 7:16 PM, Joakim Plate elupus@ecce.se wrote:
|
|
The problem is observed with ass subs in seperate file. E.g. |
you lost me here. As far as I understand the code, OverlayRenderer uploads the overlay and sets hw resources to m_overlay. Player would submit the same overlay again and OverlayRenderer increases reference. It would not re-upload overlays as long OverlayRenderer has referenced. Once OR releases its last reference it clears hw resources. Note that subtitles which are parsed from the input stream are fine, only those provided by a separate file show this issue. |
|
Yes. but if overlay renderer releases a resource, which is still about to get added to next frame, it will be lost and re-uploaded when dvdplayer finally manages to add it to render. But as i said, normal text subs won't trigger this. So it must be ass based subs only then? Then there is a bug in that code. It keeps no buffer, so it must be leaking. |
|
I like the option of copying the overlay object in CDVDSubtitleParserText::Parse(). It's easy, it's just two simple types that need copy constructors. |
|
Actually the copy should be in CDVDSubtitleLineCollection::Get |
|
I was looking into CDVDSubtitleLineCollection::Get and struggled with the ref count which is provate. Need to make it protected. What do you think is the better approach: switch case in method get and call copy constructor explicitly add a pure virtual method "Clone" to CDVDOverlay what other type apart from ssa needs special handling? |
|
I like the virtual Clone method. Feel free to move my code into a clone method. |
|
updated the pr with your code + clone method |
|
Looks good. Can be merged as long as some minor testing has been done. |
|
Minor testing of updated patch is looking fine on Pi. |
dvdplayer: make sure subtitle file parsers clone overlays Since the subtitle parser will keep copies of the overlay structures even after they have been used, hw gpu resources was never released. So instead clone them as they leave parser, to have hw resources be released when not used anymore.
If subtitles are loaded from a separate file, it seems that they are ref counted elsewhere. Render resources are not cleared until the video is stopped. The Pi runs out of GPU memory after an hour of playback.
Credits to @popcornmix for tracking this down.