[PATCH] Universal VOB subtitle input

Developer discussion and patch submissions only!
Forum rules
This forum is for developer discussion and patch submission only.
davidfstr
Regular User
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

[PATCH] Universal VOB subtitle input

Post by davidfstr » Thu May 06, 2010 8:54 am

Adds support for reading VOB subtitle tracks from file inputs.

Thought this would be an easy patch, but the decavcodecvi work-object (which is used to get video frames from ffmpeg) was failing to pass the sequence number through from the original input packet. Because of this, the subtitle-to-video syncing code in the sync work-object never passed any subtitle packets to the VOB subtitle decoder. I have amended decavcodec.c to preserve this information.

I have updated documentation around the maintenance of the sequence field and around the bits of the decavcodec work-object that I figured out.

Have tested:
* MKV VOB -> MKV VOB passthru (works great)
* MKV VOB -> MKV VOB burned in (colors seem to be distorted)

Any ideas on the color issues?

The patch:
http://handbrake.fr/pastebin/pastebin.php?show=1396

davidfstr
Regular User
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

Re: [PATCH] Universal VOB subtitle input

Post by davidfstr » Fri May 07, 2010 5:58 am

Debugging the color anomaly now.

I've selected two scenarios for comparison:
1. Input VOB subtitles from DVD and burn in to output file
2. Input VOB subtitles from MKV and burn in to output file

These should behave exactly the same way, assuming the VOB subtitles were passed into the source MKV unmodified.

Observations:
1. The input packets to the decvobsub work-object in both scenarios are identical, with regard to their data buffers. This is expected.
2. The output packets from the decvobsub work-object in the scenarios differ. What?!

Great... We appear to have the same input to the same function but get two different results.

Maybe the non-data parts of the input packet are also significant...

Debug patch, for the interested:
http://handbrake.fr/pastebin/pastebin.php?show=1400

TedJ
Veteran User
Posts: 5388
Joined: Wed Feb 20, 2008 11:25 pm

Re: [PATCH] Universal VOB subtitle input

Post by TedJ » Fri May 07, 2010 9:27 am

The colour palette used for vobsubs is normally stored in the corresponding .IFO file, IIRC.

User avatar
Rodeo
HandBrake Team
Posts: 11668
Joined: Tue Mar 03, 2009 8:55 pm

Re: [PATCH] Universal VOB subtitle input

Post by Rodeo » Sat May 08, 2010 1:48 am

Your patch seems to only allow VOBSUB input from ffmpeg-demuxed sources.

Would be nice to hook it to libhb/demuxmpeg.c (maybe it is decmpeg2.c instead, it looks like it's where HB looks for CC608SUB in file sources) so that it can see VOBSUBs in e.g. VOB files (right now it can't).

davidfstr
Regular User
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

Re: [PATCH] Universal VOB subtitle input

Post by davidfstr » Sat May 08, 2010 4:15 am

Rodeo wrote:Your patch seems to only allow VOBSUB input from ffmpeg-demuxed sources.
Well the decvobsub work-object (and all the decoding logic) was already there before. It just used to only be usable when reading bitmap subtitles from DVDs. In the non-debug patch, I don't even touch decvobsub.c.
Rodeo wrote:Would be nice to hook it to libhb/demuxmpeg.c (maybe it is decmpeg2.c instead, it looks like it's where HB looks for CC608SUB in file sources) so that it can see VOBSUBs in e.g. VOB files (right now it can't).
I was under the impression that HandBrake supported inputs from exactly 2 types of sources: DVDs and FFMPEG-decoded streams. VOBSUBs already work for DVDs. I'm just adding support for FFMPEG decoded streams. Are VOB files treated as a fundamentally different third form of input?

I managed to leave my power adapter at work, so I won't be able to do any more investigating tonight. :-(

davidfstr
Regular User
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

Re: [PATCH] Universal VOB subtitle input

Post by davidfstr » Sat May 08, 2010 4:28 am

TedJ wrote:The colour palette used for vobsubs is normally stored in the corresponding .IFO file, IIRC.
Score! I forgot about the color table. Apparently it's stored in 'job->title->palette' for titles generated from DVDs. I know I didn't fill it out for the FFMPEG demuxer, so it's probably random.

User avatar
Rodeo
HandBrake Team
Posts: 11668
Joined: Tue Mar 03, 2009 8:55 pm

Re: [PATCH] Universal VOB subtitle input

Post by Rodeo » Sat May 08, 2010 4:39 am

davidfstr wrote:
Rodeo wrote:Would be nice to hook it to libhb/demuxmpeg.c (maybe it is decmpeg2.c instead, it looks like it's where HB looks for CC608SUB in file sources) so that it can see VOBSUBs in e.g. VOB files (right now it can't).
I was under the impression that HandBrake supported inputs from exactly 2 types of sources: DVDs and FFMPEG-decoded streams. VOBSUBs already work for DVDs. I'm just adding support for FFMPEG decoded streams. Are VOB files treated as a fundamentally different third form of input?
Well, most file input goes through FFmpeg entirely, but not everything. HB has its own MPEG PS/TS demuxer, and does not use FFmpeg to decode MPEG-2, AC3 or DTS (other contribs are used for that). If I am not mistaken, it can also decode at least some forms of PCM audio on its own, too.

So some DVD VOB files (MPEG-2 + AC3/DTS/PCM, VOBSUB, CC) can be decoded without tapping into FFmpeg at all.

Right now HB still finds Closed Captions in such files because it's looking for them in libhb/decmpeg2.c - not sure if it's there you should be looking for VOBSUB subtitles though, you'd have to ask a dev.

User avatar
JohnAStebbins
HandBrake Team
Posts: 5018
Joined: Sat Feb 09, 2008 7:21 pm

Re: [PATCH] Universal VOB subtitle input

Post by JohnAStebbins » Sat May 08, 2010 3:27 pm

davidfstr, palette data gets stored in the codec private data of mkv. ffmpeg delivers this in AVStream->codec->extradata. the data len is AVStream->codec->extradata_size. Look at handbrake's muxmkv.c for an example of the format of this data.

davidfstr
Regular User
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

Re: [PATCH] Universal VOB subtitle input

Post by davidfstr » Sun May 09, 2010 1:41 am

JohnAStebbins wrote:davidfstr, palette data gets stored in the codec private data of mkv. ffmpeg delivers this in AVStream->codec->extradata. the data len is AVStream->codec->extradata_size. Look at handbrake's muxmkv.c for an example of the format of this data.
Fun. Looks like we don't already have parsing code for this MKV private data, so I'll whip something together. Although it's a shame that I can't find an actual spec for the format being used, so I'll just have to guess:

Code: Select all

MkvVobSubtitlePrivateData = ( Line )*
Line = [^:]+: [^\n]*\n
In the absence of a spec, I will write a very strict parser that is unforgiving of misplaced whitespace.

User avatar
JohnAStebbins
HandBrake Team
Posts: 5018
Joined: Sat Feb 09, 2008 7:21 pm

Re: [PATCH] Universal VOB subtitle input

Post by JohnAStebbins » Sun May 09, 2010 6:12 am

http://www.matroska.org/technical/specs ... mages.html
all the parsing code i've looked at in other apps just uses scanf. So spacing can be varible but exact case is required.

davidfstr
Regular User
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

Re: [PATCH] Universal VOB subtitle input

Post by davidfstr » Sun May 09, 2010 9:35 pm

I've modified the FFMPEG demuxer to fill out the palette color lookup table correctly from the MKV private codec data.

When doing this I realized that the current way that palette colors are stored, per-title instead of per-track, precludes us from supporting multiple VOB subtitle tracks - since different tracks could have different palettes. Hence I have added code to add_ffmpeg_subtitle() to prevent the reading of multiple VOB subtitle tracks for the time being.

Of course this problem still remains with VOB subtitle tracks coming from all other input sources (including DVDs), so maybe I should just fix the underlying problem. Probably by moving 'title->palette' to 'subtitle->palette'. Comments?

Current patch:
http://handbrake.fr/pastebin/pastebin.php?show=1407

User avatar
JohnAStebbins
HandBrake Team
Posts: 5018
Joined: Sat Feb 09, 2008 7:21 pm

Re: [PATCH] Universal VOB subtitle input

Post by JohnAStebbins » Mon May 10, 2010 2:58 am

moving the palette from title to subtitle is fine with me. just remember that you'll have to modify the dvd.c and dvdnav.c to duplicate the palette from the dvd to every subtitle track. dvd's only have a single palette for all tracks.

davidfstr
Regular User
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

Re: [PATCH] Universal VOB subtitle input

Post by davidfstr » Tue May 11, 2010 5:08 am

Updates from me will come very slowly this week, as I have a major work deadline on Friday.

User avatar
JohnAStebbins
HandBrake Team
Posts: 5018
Joined: Sat Feb 09, 2008 7:21 pm

Re: [PATCH] Universal VOB subtitle input

Post by JohnAStebbins » Tue May 11, 2010 5:12 am

no worries. several of us have been in the same boat lately.

eddyg
Veteran User
Posts: 798
Joined: Mon Apr 23, 2007 3:34 am

Re: [PATCH] Universal VOB subtitle input

Post by eddyg » Wed May 12, 2010 2:48 am

davidfstr wrote: Of course this problem still remains with VOB subtitle tracks coming from all other input sources (including DVDs), so maybe I should just fix the underlying problem. Probably by moving 'title->palette' to 'subtitle->palette'. Comments?
I'm good with that.

Cheers, Ed.

davidfstr
Regular User
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

Re: [PATCH] Universal VOB subtitle input

Post by davidfstr » Thu May 20, 2010 2:00 am

I'm finally back.

Moved the VOB subtitle palette from per-title to per-track. This should be the last modification needed.

Patch summary:

Code: Select all

Support for VOB subtitles from FFMPEG-decoded file inputs. Contributed by davidfstr.
* FFMPEG demuxer updated to recognize VOB subtitle tracks and to decode the subtitle palette from MKV track private data.
* decavcodecvi work-object updated to preserve sequence numbers on generated packets.
* VOB color palette now maintained per-track instead of per-title.
* Updated related documentation.
The patch:
http://handbrake.fr/pastebin/pastebin.php?show=1421

eddyg
Veteran User
Posts: 798
Joined: Mon Apr 23, 2007 3:34 am

Re: [PATCH] Universal VOB subtitle input

Post by eddyg » Thu May 20, 2010 3:33 am

Hi David,

Looks pretty good to me.

1) Should:

hb_log( "decvobsub: input color palette is empty; not demuxed properly?" );

be an error?

hb_error( "decvobsub: input color palette is empty; not demuxed properly?" );?

I agree that it needn't be fatal though.

2) And as for rgb2yuv() I'm surprised (as you probably were too) that one of the many HB contribs didn't already have that. I'd prefer to see it in common.c, maybe prefixed with a hb_ to prevent clashes in any future contrib. The one in muxmkv should probably be moved as well.

3) I like the parsing in ffmpeg_parse_vobsub_extradata(), easy to read and understand.

Cheers Ed.

davidfstr
Regular User
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

Re: [PATCH] Universal VOB subtitle input

Post by davidfstr » Thu May 20, 2010 6:13 am

eddyg wrote:1) Should:

hb_log( "decvobsub: input color palette is empty; not demuxed properly?" );

be an error?

hb_error( "decvobsub: input color palette is empty; not demuxed properly?" );?

I agree that it needn't be fatal though.
Every use of hb_error() that I see right now is a fatal error of some sort that results in the transcoding process aborting or a method returning an error code. Since this detection logic does neither, I'd be in favor of using the original hb_log().
eddyg wrote:2) And as for rgb2yuv() I'm surprised (as you probably were too) that one of the many HB contribs didn't already have that. I'd prefer to see it in common.c, maybe prefixed with a hb_ to prevent clashes in any future contrib. The one in muxmkv should probably be moved as well.
That's sensible. I'll move them as you suggest.
eddyg wrote:3) I like the parsing in ffmpeg_parse_vobsub_extradata(), easy to read and understand.
Thanks. I actually rewrote the parser logic three times trying to get something reasonably simple. Finally, as you see, I ended up pushing most of the logic into sscanf().

Updated patch:
http://handbrake.fr/pastebin/pastebin.php?show=1422

eddyg
Veteran User
Posts: 798
Joined: Mon Apr 23, 2007 3:34 am

Re: [PATCH] Universal VOB subtitle input

Post by eddyg » Thu May 20, 2010 8:34 am

I'm ok with this going in. Do you have commit permission?

dynaflash
Veteran User
Posts: 3820
Joined: Thu Nov 02, 2006 8:19 pm

Re: [PATCH] Universal VOB subtitle input

Post by dynaflash » Thu May 20, 2010 1:55 pm

eddyg wrote:I'm ok with this going in. Do you have commit permission?
eddyg, no he doesn't yet. I committed his last stuff for him and credited him on it.

User avatar
s55
HandBrake Team
Posts: 8812
Joined: Sun Dec 24, 2006 1:05 pm

Re: [PATCH] Universal VOB subtitle input

Post by s55 » Thu May 20, 2010 2:21 pm

Should probably add him to the AUTHORS file as this is a significant portion of work. (http://trac.handbrake.fr/browser/trunk/AUTHORS)
David, feel free to PM me if you want to be added and what information you wish to be displayed if you do.

dynaflash
Veteran User
Posts: 3820
Joined: Thu Nov 02, 2006 8:19 pm

Re: [PATCH] Universal VOB subtitle input

Post by dynaflash » Thu May 20, 2010 3:30 pm

Committed as http://trac.handbrake.fr/changeset/3308. Thanks again davidfstr and be sure to pm s55 to get into the AUTHORS file :)

User avatar
s55
HandBrake Team
Posts: 8812
Joined: Sun Dec 24, 2006 1:05 pm

Re: [PATCH] Universal VOB subtitle input

Post by s55 » Fri May 21, 2010 2:35 pm

Windows Build Broke:
: ./libhb/libhb.a(stream.o):stream.c:(.text+0x3ca4): undefined reference to `_strtok_r'
: ./libhb/libhb.a(stream.o):stream.c:(.text+0x3ccf): undefined reference to `_strtok_r'
: collect2: ld returned 1 exit status
: make: *** [HandBrakeCLI.exe] Error 1
: make: *** Waiting for unfinished jobs....
: Creating library file: ./libhb/hb.liblibhb/stream.o:stream.c:(.text+0x3ca4): undefined reference to `_strtok_r'
: libhb/stream.o:stream.c:(.text+0x3ccf): undefined reference to `_strtok_r'
: collect2: ld returned 1 exit status

User avatar
s55
HandBrake Team
Posts: 8812
Joined: Sun Dec 24, 2006 1:05 pm

Re: [PATCH] Universal VOB subtitle input

Post by s55 » Fri May 21, 2010 2:37 pm

Looks like strtok_r is not available on MinGW.

User avatar
s55
HandBrake Team
Posts: 8812
Joined: Sun Dec 24, 2006 1:05 pm

Re: [PATCH] Universal VOB subtitle input

Post by s55 » Fri May 21, 2010 3:33 pm


Post Reply