After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 343348 - [matroska] add support for vobsub subtitles
[matroska] add support for vobsub subtitles
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 0.10.5
Assigned To: Tim-Philipp Müller
GStreamer Maintainers
Depends on: 350044
Blocks:
 
 
Reported: 2006-05-30 00:30 UTC by Sebastien Bacher
Modified: 2006-09-04 16:30 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
Patch implementing VOBSUB support in matroska-demux (12.76 KB, patch)
2006-08-04 17:40 UTC, Frédéric Riss
none Details | Review
Matroska VOBSUB support without leak (14.44 KB, patch)
2006-08-04 19:39 UTC, Frédéric Riss
committed Details | Review

Description Sebastien Bacher 2006-05-30 00:30:19 UTC
That bug has been opened on https://launchpad.net/distros/ubuntu/+source/totem/+bug/47056

"It seems that Totem ignores vobsub subtitles in mkv files. Xine doesn't.
..."
Comment 1 Sven Arvidsson 2006-06-11 22:21:06 UTC
It would be great if the support for VobSub was not limited to Matroska files but also applied to external files (.idx + .sub) this is often the easiest way to get subtitles when making a DVD backup.
Comment 2 Frédéric Riss 2006-08-04 17:40:45 UTC
Created attachment 70211 [details] [review]
Patch implementing VOBSUB support in matroska-demux

Find a first attempt at this in the attached patch. This is my first Gstreamer code, I'd appreciate comments.

Vobsubs might be compressed in the matroska file. Thus this patch not only adds support for VOBSUB subtitles, but also for zlib compressed blocks. 
A little hunk of the patch also uses the REFERENCEBLOCK data to skip non-key frames while seeking. This makes the life of the video and audio decoders easier (in my case, the ffmpeg XVid decoder emited a lot of warnings while seeking).

Something important to note: this patch is useless until someone fixes the dvdsubdec element to actually display the subtitles :-). The only thing that it will get you right now is SPU commands executed by dvdsubdec that you'll see in the element's debug output.

One last thing: while seeking with the patch applied, I get crashes in dvdsubdec. From time to time (but always while seeking), the element executes gst_dvd_sub_dec_merge_title() with a NULL dec->partialbuf. I don't know if it's an issue in dvdsubdec or in my patch, any idea?

2006-08-04  Frederic Riss <frederic.riss@gmail.com>

	* gst/matroska/matroska-ids.h: New defines for CONTENTENCODINGS 
	support and VOBSUB.
	(struct _GstMatroskaTrackContext): Add encodings field.
	* gst/matroska/matroska-demux.c (subtitle_src): Add mime type for
	VOBSUB support.
	(gst_read_track_encodings): New function.
	(gst_matroska_demux_add_stream): Handle 
	GST_MATROSKA_ID_CONTENTENCODINGS.
	(gst_matroska_decode_buffer): New function.
	(gst_matroska_demux_parse_blockgroup_or_simpleblock): Handle
	GST_MATROSKA_ID_REFERENCEBLOCK.
	(gst_matroska_demux_subtitle_caps): Handle 
	GST_MATROSKA_CODEC_ID_SUBTITLE_VOBSUB.
Comment 3 Frédéric Riss 2006-08-04 18:03:42 UTC
Oops the patch leaks... I planed to add the memory freeing for the new GArray, but forgot to add it before posting :-(

I'll first wait for some comments and then send an updated patch if there's interest.
Comment 4 Tim-Philipp Müller 2006-08-04 18:49:39 UTC
> Oops the patch leaks... I planed to add the memory freeing for the new GArray,
> but forgot to add it before posting :-(
> 
> I'll first wait for some comments and then send an updated patch if there's
> interest.

Patch looks really nice and clean, can't see much that needs changing at first glance, so please just go ahead and update your current patch :)

Comment 5 Frédéric Riss 2006-08-04 19:39:35 UTC
Created attachment 70222 [details] [review]
Matroska VOBSUB support without leak

Thanks for the quick review! Attached an updated patch without the memory leak.
Comment 6 Frédéric Riss 2006-08-05 10:26:07 UTC
I tried to add a 'depends' on bug 350044 which aims to fix dvdsubdec, but I don't have the rights to do so. 
Comment 7 Tim-Philipp Müller 2006-09-04 16:30:01 UTC
Committed with only a few minor style-related changes:

 2006-09-04  Tim-Philipp Müller  <tim at centricular dot net>

        Patch by: Frédéric Riss  <frederic.riss at gmail dot com>

        * gst/matroska/matroska-demux.c: (gst_matroska_track_free),
        (gst_matroska_demux_reset),
        (gst_matroska_demux_read_track_encodings),
        (gst_matroska_demux_add_stream), (gst_matroska_decode_buffer),
        (gst_matroska_demux_parse_blockgroup_or_simpleblock),
        (gst_matroska_demux_subtitle_caps):
        * gst/matroska/matroska-ids.h:
          Add support for VOBSUB subtitle tracks and zlib-compressed
          tracks. Make sure we start on a keyframe after a seek. (#343348)

Many thanks for the patch!


(In reply to comment #2)
> One last thing: while seeking with the patch applied, I get crashes in
> dvdsubdec. From time to time (but always while seeking), the element executes
> gst_dvd_sub_dec_merge_title() with a NULL dec->partialbuf. I don't know if 
> it's an issue in dvdsubdec or in my patch, any idea?

I've seen those as well, it was one of the reasons why I've set dvdsubdec's rank to NONE for the time being. I'm fairly sure that's a dvdsubdec issue though, since I've ran into it with dvddemux.



The more general playbin/Totem issue is already tracked in bug #350311, hence closing this bug.