Chapter Sync and Chapter Name Fixes/Support

Cyander · Post by **Cyander** » Thu Mar 29, 2007 7:10 am

Okay, I finally have something ready to show. The patch is a bit large, so I am having to link to it rather than just drop it in.

http://homepage.mac.com/cyander/Files/ChapterWork.zip

It includes 3 diffs (one against libhb, one against the CLI, one against the OS X UI), and MainMenu.nib.

MainMenu.nib will be the joy, since I managed to get out of sync with dynaflash's tweaks and so on.

The work is pretty extensive, since it adds a new marker in hb_buffer_t used to track chapter breaks as they worm their way through the pipeline, as well as all the supporting code to track chapter breaks, and adjust for DVD behavior. As well as better muxing behavior for chapter samples, and better creation of those text samples.

It also adds a new tab, plus supporting code for it in the OS X UI. Allowing for naming of chapters using UTF8

The CLI can now read CSV (comma-separated values) files for chapter naming information (and it supports UTF8). It is a quick, simple way to parse a text file for data, and can be used elsewhere if needed. Not sure exactly why, but the diff for test.c touched everything in the file, when only maybe about 20 lines are different. Not sure what I can do about that to make the diff easier.

hawkman · Post by **hawkman** » Fri Mar 30, 2007 2:24 am

I'm sure you're fed up of chapter times now, and having tested your work they seem almost spot on (thanks!) - but there's still a tiny issue with timings - I think.

Matching playback with DVD Player for a full-length rip I tried (haven't tested thoroughly with part-rips), there was a consistent gap of 4 frames delay in chapter times for HB's output versus what DVD Player thought - apart from two chapters: chapter 2 (the first chapter mark after the start) was 5 frames late, and the final chapter mark (36 in this case) was only 1 frame late. Weird eh?

Goodness knows if DVD Player's chapter tracking is frame-perfect anyway; certainly one or two looked the odd frame out of place judging by the scene cuts. Then again, HB's weren't right by the scene cuts either - so that could be sloppy editing of the DVD. Too many variables!

Either way, it's close enough. Much closer than the random offsets there were before. This is a great feature to have!

[edit] Having said that, I didn't do anything about MainMenu.nib - I assume that wouldn't make a difference though

Cyander · Post by **Cyander** » Tue Apr 03, 2007 11:48 pm

I know of the issues with chapter sync, and have improved it even further. There are some oddities with how mpeg2dec handles GOPs near chapter breaks, and passes that state back. I have addressed some of these issues, and also addressed an issue where if the last chapter only had one cell... the last chapter marker would be left out of the text track.

So, in the build I have (which is newer), the 'rule' is:

- Find Cell start for the chapter
- Find first GOP in the chapter (GOPs can spill over from a previous chapter's cells)
- Find first I-frame in the stream after the GOP marker.

This seems to be the most accurate way. It might still be different for a couple DVDs, but the logic is now consistent with over a dozen MPAA releases I have been able to try, and Handbrake's logic for finding the first frame in a chapter.

I don't think there is a way to make it any more accurate than this without the original masters.

hawkman · Post by **hawkman** » Tue Apr 03, 2007 11:51 pm

Cyander wrote:I don't think there is a way to make it any more accurate than this without the original masters.

Well, find them for me, dammit! I demand perfection!

Thanks for your continued work. Sounds hot.

Cyander · Post by **Cyander** » Wed Apr 04, 2007 12:02 am

The closest thing to perfection is to imitate the logic of the DVD player.

The problem with doing this is that DVDs are authored in a such a way to depend on complicated logic to make sure that the right frame is the one used for the chapter entry.

So what I was seeing was the following:

Code: Select all

Cell 1      |Cell 2
IPPPPPPPPPPP|PPIPPPPPPPPP
            ^  ^- First Frame displayed when skipping to cell 2
            |- Chapter break as authored to the DVD

Now, the number of P frames that spill can be variable, and to make matters worse... mpeg2dec would return the GOP header when it found it in the stream, not when it was about to decode the first I frame in the group.

This is kinda why you saw the problems, is that my previous logic wound up assuming that there were 2 P frames (not intentionally) between the GOP header being found, and the eventual I frame being decoded. Now I actually check for the I frame specifically, which makes it much more robust.

deckeda · Post by **deckeda** » Wed Apr 04, 2007 3:23 pm

I agree this is starting to split hairs, but if you (the collective "you", not necessarily you) wanted to verify if it was an authoring error as opposed to something else, would myDVDEdit tell you? http://www.mydvdedit.com/

After all, it claims to be "a tool which allows you to understand and even to modify the content of a DVD, at the deepest level."

Cyander · Post by **Cyander** » Wed Apr 04, 2007 9:33 pm

Well, it is always good to verify with a second app sometimes when debugging (and I do).

I think I might be unclear though. DVDs are authored in 'cells', not 'frames'. So a chapter break is aligned to a cell, and the cell's first frame is right there, but you have no way of knowing if that first frame in the cell is the right frame until you check what type of frame it is.

Code: Select all

Common Example:

Cell 1      | Cell 2     | Cell 3
IPPPPIPPPPIP|PPPIPPPPIPPP|PP... and so on
            ^ | ^- 3) I-Frame found, start rendering frames from here
            | \- 2) DVD player skips these 3 frames, since it can't render them without an I-Frame
            \- 1) DVD Player jumps to chapter break, resets rendering

Authors depend on this behavior to get chapters to start when they want them to, since you cannot always encode a DVD in such a way that a group of frames ends perfectly at the end of a cell (A group of frames starts with an I-frame, and continues to the next I-frame). But, by making a chapter's opening frame be the first I-frame in a cell, chapters will work the way the author intends.

So, since there is no frame-accurate data on the DVD itself, the best we could ever hope to accomplish is mimicing a DVD player, which we already do (just not with the diff as posted yet).

entropic · Post by **entropic** » Wed Apr 11, 2007 8:58 am

Is there a good reason why cyander's patches have not been applied to the repository yet?

hawkman · Post by **hawkman** » Wed Apr 11, 2007 9:04 am

Yes.

[edit] I suppose I feel vaguely charitable this morning.

- The dev team is extremely busy troubleshooting other new features at the moment
- It's a non-trivial change as there's been some divergence in the UI over the last couple of weeks which needs to be resolved

jbrjake · Post by **jbrjake** » Wed Apr 11, 2007 3:48 pm

I just proposed checking it in, on the dev channel.

Consensus:

We're waiting til 0.8.5b2.

The other option is we never have another binary release again and just keep on adding features and breaking things in perpetuity.

jbrjake · Post by **jbrjake** » Sat Apr 21, 2007 3:26 pm

Okay, Cyander.

0.8.5b1 is out, which means it's your time now =)

I'll need a diff against a current revision.

dynaflash · Post by **dynaflash** » Sat Apr 21, 2007 3:42 pm

jbrjake wrote:The other option is we never have another binary release again and just keep on adding features and breaking things in perpetuity.

In fine HandBrake tradition

Cyander · Post by **Cyander** » Sat Apr 21, 2007 6:01 pm

jbrjake wrote:Okay, Cyander.

0.8.5b1 is out, which means it's your time now =)

I'll need a diff against a current revision.

I'll work on that sometime this weekend... have some other engagements I already agreed to, but I should be able to find an hour or two to re-diff and resolve any issues.

maurj · Post by **maurj** » Sat Apr 21, 2007 10:11 pm

Hi Cyander,

Brilliant stuff

Will be great to finally get this properly integrated into HandBrake!

- maurj

Cyander · Post by **Cyander** » Mon Apr 23, 2007 2:08 am

I have posted up the diff (see the URL in the first post).

The CLI and library are synced up to about 10 minutes ago.

The NIB still needs to be hand-merged, unfortunately. Curse Apple and its use of binary NIBs for the serialized objects. :/

jbrjake · Post by **jbrjake** » Mon Apr 23, 2007 3:50 am

=) Thank you again, Cyander.

I'll test this out in the morning. Do you have/want svn access to commit it yourself?

jbrjake · Post by **jbrjake** » Mon Apr 23, 2007 3:23 pm

Cyander, I downloaded the patches, but they don't apply cleanly to the latest source. They're diffs against revision 461, not 545.

Cyander · Post by **Cyander** » Mon Apr 23, 2007 6:52 pm

That is very unsual, I will look into it...

I explicitly svn updated to 545, and resolving test.c by hand before making the diff. The test.c diff in particular shrunk by a ton because of 545 and my version both using the same sort of line breaks.

It should be a single diff "Final.diff"... this is what you are getting?

jbrjake · Post by **jbrjake** » Mon Apr 23, 2007 7:43 pm

No, when I download from the link you provided ("see the URL in the first post"), I get 3 diff files...so I'm guessing it's the old patch?

Cyander · Post by **Cyander** » Tue Apr 24, 2007 5:45 am

Figured it out... wrong filename on the server, try again.

jbrjake · Post by **jbrjake** » Tue Apr 24, 2007 10:21 pm

Hmm...I'm getting an odd crash when I launch a patched build:

Code: Select all

Version: 0.8.5b1 (2007042401)

PID:    9833
Thread: 0

Exception:  EXC_BREAKPOINT (0x0006)
Code[0]:    0x00000001
Code[1]:    0x9297c120


Thread 0 Crashed:
0   com.apple.Foundation 	0x9297c120 _NSRaiseError + 264
1   com.apple.Foundation 	0x9297be5c +[NSException raise:format:] + 40
2   com.apple.Foundation 	0x92954f4c -[NSObject(NSForwardInvocation) forward::] + 176
3   libobjc.A.dylib      	0x90a440b0 _objc_msgForward + 176
4   org.m0k.handbrake    	0x00013b2c -[PrefsController awakeFromNib] + 4268 (PrefsController.m:251)
5   com.apple.Foundation 	0x9296b668 -[NSSet makeObjectsPerformSelector:] + 164
6   com.apple.AppKit     	0x93726a70 -[NSIBObjectData nibInstantiateWithOwner:topLevelObjects:] + 864
7   com.apple.AppKit     	0x93712c9c loadNib + 240
8   com.apple.AppKit     	0x937126f4 +[NSBundle(NSNibLoading) _loadNibFile:nameTable:withZone:ownerBundle:] + 716
9   com.apple.AppKit     	0x93769bc4 +[NSBundle(NSNibLoading) loadNibFile:externalNameTable:withZone:] + 156
10  com.apple.AppKit     	0x937f9a70 +[NSBundle(NSNibLoading) loadNibNamed:owner:] + 344
11  com.apple.AppKit     	0x937f9810 NSApplicationMain + 344
12  org.m0k.handbrake    	0x000069bc _start + 760
13  org.m0k.handbrake    	0x000066c0 start + 48

Thread 1:
0   libSystem.B.dylib    	0x9000ab48 mach_msg_trap + 8
1   libSystem.B.dylib    	0x9000aa9c mach_msg + 60
2   com.unsanity.ape     	0xc0001b14 __ape_agent + 296
3   libSystem.B.dylib    	0x9002b508 _pthread_body + 96

Thread 0 crashed with PPC Thread State 64:
  srr0: 0x000000009297c120 srr1: 0x100000000002f030                        vrsave: 0x0000000000000000
    cr: 0x28000442          xer: 0x0000000000000000   lr: 0x000000009297c0f8  ctr: 0x000000009293df1c
    r0: 0x0000000000000000   r1: 0x00000000bffff4b0   r2: 0x00000000a293b508   r3: 0x00000000bffff060
    r4: 0x0000000000000000   r5: 0x000000009293d844   r6: 0x00000000bffff0e4   r7: 0x00000000000000ff
    r8: 0x00000000bffff0d0   r9: 0x000000000135d9b0  r10: 0x0000000090a3d628  r11: 0x0000000028000442
   r12: 0x000000009293df1c  r13: 0x0000000000500000  r14: 0x0000000000500000  r15: 0x00000000004fe50c
   r16: 0x00000000004fe4fc  r17: 0x000000000137fa10  r18: 0x00000000004fe4ec  r19: 0x00000000004fe5fc
   r20: 0x00000000004fe5dc  r21: 0x00000000004fe5ac  r22: 0x00000000004fe58c  r23: 0x000000000064720c
   r24: 0x0000000001371e10  r25: 0x00000000bffff628  r26: 0x0000000001371e10  r27: 0x00000000a2954adc
   r28: 0x000000000136df60  r29: 0x00000000a2940808  r30: 0x0000000001308fc0  r31: 0x000000009297c028

I tried trashing all my preferences, but it didn't help.

Cyander · Post by **Cyander** » Wed Apr 25, 2007 2:14 pm

I think there is something very odd about that crash, and I am surprised you didn't see it too: EXC_BREAKPOINT. (nevermind... Apple is intentionally throwing this exception for debuggers... durrrrrr)

That said... I am seeing it too... and wondering why. This didn't happen until I sync'd to 545.

EDIT: I looked quickly, and it sounds suspiciously like the NIB hasn't yet been merged... I did mention I haven't done that yet. I would use dynaflash's NIB from 545, and use mine as a template on what to add to dynaflash's.

If you revert the changes to the mac GUI, you can probably confirm this. The NIB will need to be merged before the code patch can be used on the mac GUI.

jbrjake · Post by **jbrjake** » Wed Apr 25, 2007 2:36 pm

Cyander wrote:EDIT: I looked quickly, and it sounds suspiciously like the NIB hasn't yet been merged... I did mention I haven't done that yet. I would use dynaflash's NIB from 545, and use mine as a template on what to add to dynaflash's.

My bad--I thought by hand-merging you just meant I had to substitute your .nib for the svn's.

Cyander · Post by **Cyander** » Wed Apr 25, 2007 5:56 pm

No problem, I have just been working late this last week, so I haven't had time to do the expensive merging of NIBs myself.

dynaflash · Post by **dynaflash** » Wed Apr 25, 2007 6:04 pm

Cyander, done. svn rev 549.

Thanks, looks nice

Chapter Sync and Chapter Name Fixes/Support

Chapter Sync and Chapter Name Fixes/Support

Why is this not in svn?