[DVDnav-discuss] dvdnav patches from handbrake project

[Erik Hovland]

> I've recreated the patches against svn head since there's been a few
> changes since I originally created many of them.  New patches can be
> found here http://www.stebbins.biz/source/patches/. I'll summarize the
> patches again so that all the information is in one place.
>
> * nav-configure-uid0.patch
> * read-configure-uid0.patch
> These patches remove the special case test for uid=0.
> Some package tools run configure as fakeroot.  This triggers code in
> configure.ac that detects uid 0 to hardcode the installation path of the
> m4 macros to the system aclocal macro dir.  This ignores any DESTDIR the
> packaging tools may have set and installation fails since it attempts to
> write to a system dir without proper permissions.

It would be really nice if someone who is involved w/ a distribution who
packages libdvdnav and libdvdread could comment.

> * nav-dup-handle.patch
> This patch allows you safely to create a duplicate of a dvdnav_t.  This
> handle can be used in parallel with the handle it is duplicated from.
> HandBrake uses this to perform a recursive search of dvd menus in order
> to find the main feature of a disc.

At the least this looks like it should be split into two. The first being the
vm_stop and vm_close functions the second being the dup functions.

> * nav-forward-seek.patch
> This patch guarantees that a request to seek forward always results in
> the new position being moved forward.
> libdvdnav has a problem in that it is difficult to gracefully recover
> from a read error and continue on to subsequent blocks on the disc.  An
> application would like to seek forward past the current block after
> getting a read error in order to attempt to get past the bad block(s).
> But dvdnav_sector_search() does not guarantee that a requested forward
> seek will actually move the current position forward.  It truncates down
> to the nearest VOBU which will almost always cause a forward seek
> request by a single block (or a small number of blocks) to move the
> current position backward.  This behaviour puts the application into a
> loop of: read failure, attempted forward seek (which results in backward
> seek), read failure ...

This is the kind of patch that I would love to know what VLC or mplayer
devs think.

> * nav-missing-menu-abort.patch
> This patch prevents an abort when a nav command tries to send you to a
> menu that doesn't exist.
> Mac the ripper's feature title extraction removes menus from the
> resulting image, but does not remove navigation instructions that
> attempt to jump to those  menus.  This patch checks that a menu exists
> before acting on such instructions.  If the menu does not exist, the it
> puts the vm into the stopped state.

This has a high likelihood of being committed by me. I just have to get my
head around the error path handling.

> * nav-multi-pgc.patch
> This patch allow play of multi-pgc titles. Eliminates message "RANDOM or
> SHUFFLE titles not handled yet".
> The bit tested here does not indicate 'random or shuffle' - it only says
> that the title uses multiple PGCs. libdvdnav mostly deals correctly with
> mult PGC titles (modulo some weirdness when seeking).

Also has a good chance of getting in.

> * nav-program-info.patch
> Add dvdnav_program_play and dvdnav_current_title_program analogs to
> dvdnav_part_play and dvdnav_current_title_info.
> We add chapter marks to the output mkv or mp4 files while encoding.
> Using title parts is not reliable for this.  The start of a part does
> not necessarily fall strictly inside the main title.  Parts can have an
> intro sequence before getting into the title.  So we use program
> boundaries instead of part markers to test when we have reached a
> chapter point during encoding of the title.   The same would apply to
> displaying the current chapter during playback.  The program boundaries
> should be checked instead of looking for a part.

Only adds new API. As long as handbrake is using it, I am not opposed.

> * nav-read-name.patch
> Prevent display of garbage volume label when the file being scanned is
> not a dvd. This is a simple movement of the code that logs the volume
> name to after the point that some basic validation of the disc has been
> performed.

Seems fine to me. I will have to give it a bit of inspection to make sure there
isn't a locking problem (the BVL - Big VM Lock is a bit tricky).

> * nav-reset-deadlock.patch
> dvdnav_reset takes a lock, then latter calls dvdnav_clear which tries to
> take
> the lock again. dead. the unlock that is immediately after dvdnav_clear
> should be moved to before it.

Fine, but what about the conditional change. Is it part of the fix or
does it deal
w/ something else. If it deals w/ something else, please separate.

> * read-block2char.patch
> Fixes conversion of disk names to raw disk names.
> On darwin and other bsd based systems, dvd_reader.c converts regular device
> names to raw device names. e.g. "/dev/disk4" -> "/dev/rdisk4"
> The test that determines if the conversion needs to be done is broken.
> It is fixed in libdvdread version 0.9.7.

I didn't put this into my 0.9.7 commit because I did not have a darwin
or bsd system
to see if this was still relevant. Thanks for the confirmation. This
will likely get applied.

> * read-dup-lut-pgc.patch
> The is a big invasive patch. Many dvd's are showing up that have titles
> that have numerous repeated language unit tables and pgc's. The new
> release "Up" is an example.  I believe "Dark Knight" also exhibits this.
> When scanning such discs with lsdvd (and other programs that scan all
> titles), libdvdread will consume many GB of data for these repeated
> elements and gets very slow (e.g hours to read all titles) on OS X due
> to reading the duplicate data from uncached raw devices.
>
> This patch detects duplicates and reference counts pgc's and pgcit's.
> When a duplicate is detected, a reference count is incremented instead
> of allocating new memory and re-reading the data.
>
> I opted for reference counting instead of simply returning a failure on
> duplicate detection because it is perfectly valid to have a title with
> such duplicates.  I could easily imagine titles adding duplicates, then
> navigating around them.

This is the most interesting to me. I love patches that make this kind
of optimization.
But probably every other of the fixes patches will have to be addressed before
this one.

> * read-mingw-large-file.patch
> Add large file support for mingw
> libdvdread on mingw fails reading at 2G boundary. This means reading
> large iso files fails.

I will defer to j-b and anyone else who actively port to this platform.

> * read-raw-device.patch
> On windows, use correct path when attempting to open the raw dvd
> device.  Also check that it is a dvd device in order to avoid error
> message spam.

Same here about windows. I believe j-b already piped up about them.

Thanks again. Sorry it is taking so long. But all of the submissions
are important.
I do want to see your contributions get in.

E

-- 
Erik Hovland
erik at hovland.org
http://hovland.org/