[PATCH] Universal VOB subtitle input

Archive of historical development discussions
Discussions / Development has moved to GitHub
Forum rules
*******************************
Please be aware we are now using GitHub for issue tracking and feature requests.
- This section of the forum is now closed to new topics.

*******************************
davidfstr
Enlightened
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

[PATCH] Universal VOB subtitle input

Post 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
davidfstr
Enlightened
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

Re: [PATCH] Universal VOB subtitle input

Post 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
TedJ
Veteran User
Posts: 5388
Joined: Wed Feb 20, 2008 11:25 pm

Re: [PATCH] Universal VOB subtitle input

Post by TedJ »

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

Re: [PATCH] Universal VOB subtitle input

Post by Deleted User 11865 »

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
Enlightened
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

Re: [PATCH] Universal VOB subtitle input

Post 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. :-(
davidfstr
Enlightened
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

Re: [PATCH] Universal VOB subtitle input

Post 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.
Deleted User 11865

Re: [PATCH] Universal VOB subtitle input

Post by Deleted User 11865 »

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: 5712
Joined: Sat Feb 09, 2008 7:21 pm

Re: [PATCH] Universal VOB subtitle input

Post 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.
davidfstr
Enlightened
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

Re: [PATCH] Universal VOB subtitle input

Post 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.
User avatar
JohnAStebbins
HandBrake Team
Posts: 5712
Joined: Sat Feb 09, 2008 7:21 pm

Re: [PATCH] Universal VOB subtitle input

Post 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.
davidfstr
Enlightened
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

Re: [PATCH] Universal VOB subtitle input

Post 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
User avatar
JohnAStebbins
HandBrake Team
Posts: 5712
Joined: Sat Feb 09, 2008 7:21 pm

Re: [PATCH] Universal VOB subtitle input

Post 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.
davidfstr
Enlightened
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

Re: [PATCH] Universal VOB subtitle input

Post by davidfstr »

Updates from me will come very slowly this week, as I have a major work deadline on Friday.
User avatar
JohnAStebbins
HandBrake Team
Posts: 5712
Joined: Sat Feb 09, 2008 7:21 pm

Re: [PATCH] Universal VOB subtitle input

Post by JohnAStebbins »

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 »

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
Enlightened
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

Re: [PATCH] Universal VOB subtitle input

Post 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
eddyg
Veteran User
Posts: 798
Joined: Mon Apr 23, 2007 3:34 am

Re: [PATCH] Universal VOB subtitle input

Post 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.
davidfstr
Enlightened
Posts: 149
Joined: Sun Apr 12, 2009 7:41 pm

Re: [PATCH] Universal VOB subtitle input

Post 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
eddyg
Veteran User
Posts: 798
Joined: Mon Apr 23, 2007 3:34 am

Re: [PATCH] Universal VOB subtitle input

Post by eddyg »

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 »

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: 10350
Joined: Sun Dec 24, 2006 1:05 pm

Re: [PATCH] Universal VOB subtitle input

Post 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.
dynaflash
Veteran User
Posts: 3820
Joined: Thu Nov 02, 2006 8:19 pm

Re: [PATCH] Universal VOB subtitle input

Post 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 :)
User avatar
s55
HandBrake Team
Posts: 10350
Joined: Sun Dec 24, 2006 1:05 pm

Re: [PATCH] Universal VOB subtitle input

Post 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
User avatar
s55
HandBrake Team
Posts: 10350
Joined: Sun Dec 24, 2006 1:05 pm

Re: [PATCH] Universal VOB subtitle input

Post by s55 »

Looks like strtok_r is not available on MinGW.
User avatar
s55
HandBrake Team
Posts: 10350
Joined: Sun Dec 24, 2006 1:05 pm

Re: [PATCH] Universal VOB subtitle input

Post by s55 »

Post Reply