Page 1 of 3

[PATCH] Universal VOB subtitle input

Posted: Thu May 06, 2010 8:54 am
by davidfstr
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

Re: [PATCH] Universal VOB subtitle input

Posted: Fri May 07, 2010 5:58 am
by davidfstr
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

Re: [PATCH] Universal VOB subtitle input

Posted: Fri May 07, 2010 9:27 am
by TedJ
The colour palette used for vobsubs is normally stored in the corresponding .IFO file, IIRC.

Re: [PATCH] Universal VOB subtitle input

Posted: Sat May 08, 2010 1:48 am
by Rodeo
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).

Re: [PATCH] Universal VOB subtitle input

Posted: Sat May 08, 2010 4:15 am
by davidfstr
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. :-(

Re: [PATCH] Universal VOB subtitle input

Posted: Sat May 08, 2010 4:28 am
by davidfstr
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.

Re: [PATCH] Universal VOB subtitle input

Posted: Sat May 08, 2010 4:39 am
by Rodeo
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.

Re: [PATCH] Universal VOB subtitle input

Posted: Sat May 08, 2010 3:27 pm
by JohnAStebbins
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.

Re: [PATCH] Universal VOB subtitle input

Posted: Sun May 09, 2010 1:41 am
by davidfstr
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.

Re: [PATCH] Universal VOB subtitle input

Posted: Sun May 09, 2010 6:12 am
by JohnAStebbins
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.

Re: [PATCH] Universal VOB subtitle input

Posted: Sun May 09, 2010 9:35 pm
by davidfstr
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

Re: [PATCH] Universal VOB subtitle input

Posted: Mon May 10, 2010 2:58 am
by JohnAStebbins
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.

Re: [PATCH] Universal VOB subtitle input

Posted: Tue May 11, 2010 5:08 am
by davidfstr
Updates from me will come very slowly this week, as I have a major work deadline on Friday.

Re: [PATCH] Universal VOB subtitle input

Posted: Tue May 11, 2010 5:12 am
by JohnAStebbins
no worries. several of us have been in the same boat lately.

Re: [PATCH] Universal VOB subtitle input

Posted: Wed May 12, 2010 2:48 am
by eddyg
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.

Re: [PATCH] Universal VOB subtitle input

Posted: Thu May 20, 2010 2:00 am
by davidfstr
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

Re: [PATCH] Universal VOB subtitle input

Posted: Thu May 20, 2010 3:33 am
by eddyg
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.

Re: [PATCH] Universal VOB subtitle input

Posted: Thu May 20, 2010 6:13 am
by davidfstr
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

Re: [PATCH] Universal VOB subtitle input

Posted: Thu May 20, 2010 8:34 am
by eddyg
I'm ok with this going in. Do you have commit permission?

Re: [PATCH] Universal VOB subtitle input

Posted: Thu May 20, 2010 1:55 pm
by dynaflash
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.

Re: [PATCH] Universal VOB subtitle input

Posted: Thu May 20, 2010 2:21 pm
by s55
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.

Re: [PATCH] Universal VOB subtitle input

Posted: Thu May 20, 2010 3:30 pm
by dynaflash
Committed as http://trac.handbrake.fr/changeset/3308. Thanks again davidfstr and be sure to pm s55 to get into the AUTHORS file :)

Re: [PATCH] Universal VOB subtitle input

Posted: Fri May 21, 2010 2:35 pm
by s55
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

Re: [PATCH] Universal VOB subtitle input

Posted: Fri May 21, 2010 2:37 pm
by s55
Looks like strtok_r is not available on MinGW.

Re: [PATCH] Universal VOB subtitle input

Posted: Fri May 21, 2010 3:33 pm
by s55