[PATCH] Universal VOB subtitle input

[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

[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

[TedJ]

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

[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).

[davidfstr]

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.

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]

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.

[Rodeo]

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.

[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.

[davidfstr]

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.

[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.

[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

[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.

[davidfstr]

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

[JohnAStebbins]

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

[eddyg]

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]

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]

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]

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().

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.

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]

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

[dynaflash]

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.

[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.

[dynaflash]

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

[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

[s55]

Looks like strtok_r is not available on MinGW.

[s55]

j45 pointed out that http://www.codase.com/search/display?fi ... w==&lang=c can be added to ports.c (or http://www.rajivchakravorty.com/source- ... ource.html)