GNOME Bugzilla – Bug 599649
Support for frame-based subtitles using playbin2 and subparse
Last modified: 2009-11-12 12:27:08 UTC
Created attachment 146258 [details] [review] Adds support for frame-based subtitles in playbin2 and subparse Frame-based Subtitle parsers need to know the video framerate to generate proper timestamps for the subtitle buffers so that video and subtitles are properly synchronized. Unfortunately, as far as my understanding goes, obtaining the video framerate from the subtitle element is not possible, since there is no communication between the video and text pipelines. Currently, there is only one frame-based subtitle format supported in gstreamer (MicroDVD in subparse element), but it only works properly if the subtitle file provides proper video framerate information, which is not always the case, and even if it does, it may not match the actual video framerate. Also, some other frame-based subtitle formats (like AQ Title) do not even provide video framerate information at all along with the subtitles. However, playbin2 does have access to reliable framerate information and it can provide this information to the subparse element, enabling it to synchronize frame-based subtitles and video properly. Attached is a patch that implements this. This patch does the following: In playin2, it gets the video stream framerate from the video decoder caps, and then provides it to the subtitle parser element by means of the "video-fps" property. In subparse, it adds the "video-fps". I do not claim this to be the best solution, it is just the one I came up with, but if there is a better approach to solve this problem I would work on a new patch. Also, if this approach is considered good, a better solution would be to have a subtitle parser base class exposing the "video-fps" property, so that any subtitle parser element can get access to this information. A similar approach can be used for playbin, however playbin starts the subtitle decoding before it I can get the video framerate information, meaning that the first buffer of subtitles will be parsed before subparse gets to know the video framerate. Probably this can be fixed somehow but I think it would need a bit of rework in playbin.
BTW, this would also fix bug 579516
*** Bug 579516 has been marked as a duplicate of this bug. ***
Review of attachment 146258 [details] [review]: For the property, better use a property of type GstFraction. It can still be converted to a double if any format requires it but the conversion to double looses precision. Then you don't use that property anywhere in subparse, would be nice to see how exactly this is used in the MicroDVD parser for example :) Then for the playbin2 part. In playbin2 you can have multiple video streams, I guess you should always use the framerate of the currently selected video stream and update it when the video stream changes, etc. And then about setting the video framerate on subparse, subtitles can also be in uridecodebin, not only in suburidecodebin. But all that becomes much easier once the patches in bug #595123 have landed. Then the video-fps handling can all be done inside subtitleoverlay without the need of touching playbin2... we already have the current video and subtitle stream there, could get the framerate and set it on the parser if it has a video-fps property.
> But all that becomes much easier once the patches in bug #595123 have landed. > Then the video-fps handling can all be done inside subtitleoverlay without the > need of touching playbin2... we already have the current video and subtitle > stream there, could get the framerate and set it on the parser if it has a > video-fps property. That sounds like a better idea (at least until people demand that they should be able to set the assumed framerate for the subtitle stream manually for some reason). I was also toying with the idea of sending a custom event of some sort (from the overlay element onto the text/sub pad) to inform the subtitle parser if the currently configured framerate.
(In reply to comment #4) > > But all that becomes much easier once the patches in bug #595123 have landed. > > Then the video-fps handling can all be done inside subtitleoverlay without the > > need of touching playbin2... we already have the current video and subtitle > > stream there, could get the framerate and set it on the parser if it has a > > video-fps property. > > That sounds like a better idea (at least until people demand that they should > be able to set the assumed framerate for the subtitle stream manually for some > reason). Well, then they could still set it on subparse manually :) Or what do you mean? > I was also toying with the idea of sending a custom event of some sort > (from the overlay element onto the text/sub pad) to inform the subtitle parser > if the currently configured framerate. Do you think that should still be added? IMHO it's cleaner to have it all inside subtitleoverlay, where the current video caps and subtitle elements are all bundled into a single bin. But such event might be useful in other circumstances maybe, I don't know
Ok, I looked at the microdvd parser and will add a commit to my playbin2-subtitles branch later that adds framerate support ;)
Ok, fix for this is at http://cgit.freedesktop.org/~slomo/gst-plugins-base/
> > I was also toying with the idea of sending a custom event .... > > Do you think that should still be added? IMHO it's cleaner to have it all > inside subtitleoverlay, where the current video caps and subtitle elements are > all bundled into a single bin. But such event might be useful in other > circumstances maybe, I don't know I agree it's probably cleaner, I just thought I'd mention it in case you hadn't thought of it. I guess the main (only?) advantage of sending an event would be that even with manual pipelines involving e.g. subparse ! textoverlay it would Just Work.
(In reply to comment #3) > Review of attachment 146258 [details] [review]: > > For the property, better use a property of type GstFraction. It can still be > converted to a double if any format requires it but the conversion to double > looses precision. Yeah, makes sense. > Then you don't use that property anywhere in subparse, would be nice to see how > exactly this is used in the MicroDVD parser for example :) I do :). I set parser->state.fps in handle_buffer to the value set in the property (only when self->first_buffer is TRUE), which is then used in MicroDVD's parse line function. Since MicroDVD's parse line only uses the fps from the file (or the hardcoded 24.00) if self->state.fps == 0, it will end up using the one established by playbin2, so I did not need to change any in MicroDVD's parser. > Then for the playbin2 part. In playbin2 you can have multiple video streams, I > guess you should always use the framerate of the currently selected video > stream and update it when the video stream changes, etc. Yes, I noticed that but I thought I would first submit a patch with this proof of concept and see what you guys think, since I was not very convinced about this being the best way to go for the fix. > And then about setting the video framerate on subparse, subtitles can also be > in uridecodebin, not only in suburidecodebin. Ah, this was something I did not had in mind. > But all that becomes much easier once the patches in bug #595123 have landed. > Then the video-fps handling can all be done inside subtitleoverlay without the > need of touching playbin2... we already have the current video and subtitle > stream there, could get the framerate and set it on the parser if it has a > video-fps property. Aha, that's sounds like a much better approach indeed.
commit eb2d2078116da50f3701002713ba27732dd147e6 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Sat Nov 7 11:46:49 2009 +0100 subtitleoverlay: Set the video framerate on parsers if possible Fixes bug #599649.