...

Huge invasive patch to consolidate memory: http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2009-November/001097.html

Bug fix for crashing: http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2009-November/001102.html

Follow up patch for same crashing issue: http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2009-December/001114.html

I have to say that I believe that we don't use those patches...

However, I have to say that many people report crashes on our forum on modern DVDs lately, so the patches against crashes can't be that bad... However, this is quite a bad timing for me, since we are on the edge of release VLC 1.1.0, and therefore I can't test too much the patches on a set of testers...

Don't sweat it. I was hoping to get some additional testing knowledge to help handbrake's cause. Both of their patches are interesting. And I agree w/ protecting against crashes. Can't be too bad. Keep plugging away at the VLC release. We can address these patches in this list for the time being. Thanks E -- Erik Hovland erik@hovland.org http://hovland.org/

On 05/27/2010 05:55 PM, John Stebbins wrote:

On 05/27/2010 10:08 AM, Erik Hovland wrote:

...

I do! Huge invasive patch to consolidate memory: http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2009-November/001097.html

Bug fix for crashing: http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2009-November/001102.html

Follow up patch for same crashing issue: http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2009-December/001114.html

001114 supersedes 001102.

There were some others as well.

I should have mentioned, all the latest versions of our current patches are available here: http://trac.handbrake.fr/browser/trunk/contrib/libdvdnav http://trac.handbrake.fr/browser/trunk/contrib/libdvdread

John Stebbins ha scritto:

On 05/27/2010 05:55 PM, John Stebbins wrote:

...

On 05/27/2010 10:08 AM, Erik Hovland wrote:

...

I do! Huge invasive patch to consolidate memory: http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2009-November/001097.html

Bug fix for crashing: http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2009-November/001102.html

Follow up patch for same crashing issue: http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2009-December/001114.html

001114 supersedes 001102.

There were some others as well.

I should have mentioned, all the latest versions of our current patches are available here: http://trac.handbrake.fr/browser/trunk/contrib/libdvdnav http://trac.handbrake.fr/browser/trunk/contrib/libdvdread

final versions? good, nice to know I won't have to dig the ML. In the weekend I should have some time to review at least the small ones.

On Fri, May 28, 2010 at 03:41:43PM +0200, Nico Sabbi wrote :

I applied A00 that is obviously ok. A02: I don't know anything about win32, so if it works it can be applied on the trust, but please explain the mismatch between MSC and WIN32 The win32 fix seems correct.

_MSVC means Microsoft Visual Studio Compiler. _WIN32 is recognized by all Win32 compilers, including gcc, mingw and icc It also works with Xcompilers. Just as a side note, '/' is a valid DIR_SEP on Win32, not only '\\' * Hence, I believe the if( strlen( path_copy ) > TITLES_MAX ) { could be rewritten in a nice way, sharing more code between the different platforms. * GetDriveType, are you targetting A or W ? Best Regards, -- Jean-Baptiste Kempf http://www.jbkempf.com/

On Fri, May 28, 2010 at 11:24:26AM -0700, John Stebbins wrote :

On 05/28/2010 06:51 AM, Jean-Baptiste Kempf wrote:

...

On Fri, May 28, 2010 at 03:41:43PM +0200, Nico Sabbi wrote :

Just as a side note, '/' is a valid DIR_SEP on Win32, not only '\\'

* Hence, I believe the if( strlen( path_copy ) > TITLES_MAX ) { could be rewritten in a nice way, sharing more code between the different platforms.

Right. We should be checking both in the case of win32. I'm not sure how you would go about sharing more code since in the win32 case you really have to do the test twice with the two different separators.

Something like if( strlen ( path_copy ) > TITLES_MAX ) { #ifdef WIN32 ... #else .. #endif path_copy[ end ] = '\0'; } So that one can see clearly that the code is linked together. Not really important I guess..

...

* GetDriveType, are you targetting A or W ?

A? W? I don't understand the question. Well, are you focusing on GetDriveTypeA or GetDriveTypeW ? Or is the selection depending on -DUNICODE ok ?

Best Regards, -- Jean-Baptiste Kempf http://www.jbkempf.com/

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. * 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. * 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 ... * nav-log-stderr.patch Changes log output to stderr. You may or may not want this. There is a comment in the code that indicates the author was uncertain whether he should use stdout or stderr. stderr is more appropriate for handbrake. * 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. * 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). * 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. * 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. * 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. * 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. * 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. * 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. * 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.

On 05/29/2010 02:52 AM, John Stebbins wrote:

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

I have found this behavior to be a major nuisance, seeking forward on certain positions always seeks nearer and nearer to a specific position in a certain chapter, there's no way of seeking beyond this position with the 10-seconds skip, skipping by a minute mostly works (but at times also not) and 10 minutes forward is simply too much. Just a question in the mean time: did someone else experience audio playback glitches on chapter breaks with dvdnav? This seems to have gone with current SVN. I have though one DVD (German "The Producers", sadly with Sony copy protection) that creates a working dumpstream, but played directly from disk starts the picture from a wrong position, and on every seek jumps back to this position (audio is ok, though). I can deliver more information on that if desired. Another DVD (German "Transformers 2", with a different copy protection) plays OK (with the aforementioned seek problems). There mplayer crashes at the menu (which I don't care about), but still has major problems stitching together the chapters, which are not contiguous on disk (apparently, on this disk the same main feature is present around 30 times, almost each time with a different program chain, if I understand correctly). On such chapter breaks, where there is need to seek to a different position, I get [ac3 @ 0xd97660]frame sync errort: -0.100 4/ 4 ??% ??% ??,?% 0 0 [ac3 @ 0xd97660]incomplete frame: 0.495 457/457 11% 0% 3.4% 0 0 [ac3 @ 0xd97660]frame sync error: 0.499 0/ 0 ??% ??% ??,?% 0 0 with an audible (and visible, some block artifacts) glitch. I assume the same will be valid also for DVDs with normal seamless branching, where also jumps are required... All the Alien (1-4) special editions come to mind with that, I still have to confirm this. To go back to the Transformers DVD, dumpstream seeks to a different position than the non-dumpstream playback. -- Cheers, Alex

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@hovland.org http://hovland.org/

On 05/29/2010 01:04 PM, John Stebbins wrote:

On 05/29/2010 10:47 AM, Erik Hovland wrote:

...

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

Yes, you are right. I didn't notice that there were 2 separate things going on here. The vm changes are to handle the case where the vm enters the stop state while playing a disc. This can happen for multiple reasons. Before the patch, the only way to get out of the stop state was to close dvdnav and re-open it. I will split these and update the patches.

Updated. Removed vm changes. Added this->file == NULL test to dvdnav_get_next_cache_block. Created new patch * nav-allow-restart.patch Allow dvdnav to be restarted after a the vm has entered the stop state for any reason.

...

...

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

Hmm, I probably could have done this better. dvdnav_reset doesn't clear this->current_position, but it does clear this->file. I could have alternately added code to dvdnav_clear to clear current_position.

The file test is also needed by the nav-dup-handle.patch. It sets file = NULL and deliberately doesn't clear dvdnav_t. Setting file to NULL forces re-opening it at the next dvdnav_get_next_cache_block.

I'll update this patch as well to put the parts where they belong.

Updated. Clear position_current in dvdnav_clear. Remove this->file test from dvdnav_get_next_cache_block. I also fixed a warning the could have lead to a problem in nav-forward-seek.patch. Note: I had to clear my browser cache before it would show me the updated versions of these patches.

On 05/29/2010 01:22 PM, Dan Nicholson wrote:

On Sat, May 29, 2010 at 10:47 AM, Erik Hovland <erik@hovland.org> wrote:

...

...

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.

The aclocal path mangling is silly. There are ways to deal with autoconf macros in non-standard locations. So, I'm good with the intent of the patch, and would be happy to see this kind of hack go away.

The part of those patches I question is leaving the AM_CONDITIONAL there. Am I wrong, or is this suppressing the installation of the autoconf macros since ACLOCAL_DIR won't be set? If that's something people want, it seems like the better thing would be a --with-aclocaldir switch that recognizes "no" or "" values to suppress installation. Without this, I'd have to require that the AM_CONDITIONAL line be removed from the patches so that the autoconf macros are still installed.

I'm not fluent in autoconf, so it wasn't obvious to me what the line was for. I updated the patches to remove that AM_CONDITIONAL, but that required additional changes in m4/Makefile.am. I'm out of my depth here, so please check that what I changed doesn't have any undesirable side effects.

...

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

John, Is it possible to consolidate the valid pointer checks in vm_jump_menu? See the attached patch. I am probably going to break the original patch up into at least two different commits. Even though they treat the same problem they work in different functions. Thanks E -- Erik Hovland erik@hovland.org http://hovland.org/

...

...

...

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

John,

Is it possible to consolidate the valid pointer checks in vm_jump_menu? See the attached patch. I am probably going to break the original patch up into at least two different commits. Even though they treat the same problem they work in different functions.

I can look at it. But it's going to have to wait a couple weeks. I'm very busy at a customers site for a while.

You have waited a long time and have been very patient. I'm sure I can do the same. E -- Erik Hovland erik@hovland.org http://hovland.org/

...

...

...

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

John,

Is it possible to consolidate the valid pointer checks in vm_jump_menu? See the attached patch. I am probably going to break the original patch up into at least two different commits. Even though they treat the same problem they work in different functions.

Thanks

Sorry it took me so long to get back to you on this.  The patch you proposed is incorrect.  The 2 cases are testing different things, so they can't be consolidated. One tests:      if(vm->vmgi == NULL || vm->vmgi->pgci_ut == NULL) { and the other is:      if(vm->vtsi == NULL || vm->vtsi->pgci_ut == NULL) {

I see that you've committed this change.  I'll include a reversion in my updated patch (which I'm currently testing).

Oops, I'll wait for your patch. Thanks E -- Erik Hovland erik@hovland.org http://hovland.org/

Hi, here's a little status update and ping to reviewers. On Tuesday, 31 August 2010 at 20:30, John Stebbins wrote:

I've updated these patches and removed the ones that have been applied already. Here's the current list of what remains.

* nav-allow-restart.patch http://www.stebbins.biz/source/patches/nav-allow-restart.patch The way vm_stop works, you can not currently start the vm again after a stop. This allows a restart.

Still not applied. Could someone review?

* nav-dup-handle.patch http://www.stebbins.biz/source/patches/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.

As above.

* nav-forward-seek.patch http://www.stebbins.biz/source/patches/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 ...

As above.

* nav-missing-menu-abort.patch http://www.stebbins.biz/source/patches/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.

Already applied.

* read-dup-lut-pgc.patch http://www.stebbins.biz/source/patches/read-dup-lut-pgc.patch This is a big 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.

Still not applied. Someone please review.

* read-raw-device.patch http://www.stebbins.biz/source/patches/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.

As above. Someone please test on Win32 and review. Regards, Dominik -- Fedora http://fedoraproject.org/wiki/User:Rathann RPMFusion http://rpmfusion.org | MPlayer http://mplayerhq.hu "Faith manages." -- Delenn to Lennier in Babylon 5:"Confessions and Lamentations"

On 10/06/2011 06:58 AM, Jean-Baptiste Kempf wrote:

On Thu, Oct 06, 2011 at 03:47:16PM +0200, Dominik 'Rathann' Mierzejewski wrote :

...

...

* read-raw-device.patch http://www.stebbins.biz/source/patches/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. As above. Someone please test on Win32 and review. GetDriveTypeA <= Why? Why hardcoding 2 in path_copy[2] = '\0'; ? What about /D: drive letters? '/D:'? I've never seen that. What's the '/' there for? If that's valid, then I guess we need to strchr for the ':'.

-- John GnuPG fingerprint: CADFA1DB594CA50EB79427284617578AADE759A1

On 05/31/2010 09:24 AM, Nico Sabbi wrote:

Jean-Baptiste Kempf ha scritto:

...

On Fri, May 28, 2010 at 11:24:26AM -0700, John Stebbins wrote :

...

Right. We should be checking both in the case of win32. I'm not sure how you would go about sharing more code since in the win32 case you really have to do the test twice with the two different separators.

Something like

if( strlen ( path_copy )> TITLES_MAX ) { #ifdef WIN32 ... #else .. #endif path_copy[ end ] = '\0'; } So that one can see clearly that the code is linked together. Not really important I guess..

Jean Baptiste: didn't you write that in win32 /video_ts is a valid path? Thus shouldn't the change in http://trac.handbrake.fr/browser/trunk/contrib/libdvdread/A02-msc_ver-defs.p... around line 451 unnecessary?

/video_ts is a valid path, but we can't control what the user inputs here. If the user types it as \video_ts, that needs to be handled somewhere.

On Mon, May 31, 2010 at 06:24:40PM +0200, Nico Sabbi wrote :

Jean Baptiste: didn't you write that in win32 /video_ts is a valid path? Thus shouldn't the change in

http://trac.handbrake.fr/browser/trunk/contrib/libdvdread/A02-msc_ver-defs.p... around line 451 unnecessary?

Well, as John said, on Win32, we actually need both if a user/an app can fill it. Best Regards, -- Jean-Baptiste Kempf http://www.jbkempf.com/

On Fri, May 28, 2010 at 03:41:43PM +0200, Nico Sabbi wrote :

P05: same as A02, scheduled for sunday Can you explain the rational of the patch?

the lseek and off_t part are ok IMHO, but why for rest of it? Best Regards, -- Jean-Baptiste Kempf http://www.jbkempf.com/

On 05/28/2010 06:41 AM, Nico Sabbi wrote:

I applied A00 that is obviously ok. A02: I don't know anything about win32, so if it works it can be applied on the trust, but please explain the mismatch between MSC and WIN32

This one was partially applied. _MSC_VER was replaced by _WIN32. Kempf has some comments on the rest that we'll need to resolve first.

A03 is stuff for Erik and/or Dan, let's wait. It's not clear to me which A03 you mean. The one in dvdnav or the one in dvdread.

A04 is still to be reviewed, but it's trivial ditto

P00-uhmm, darwin .. Sebastian, ok for you?

P05: same as A02, scheduled for sunday Kempf wanted a rational for this. He suggests we should only change what's used and accepts off_t and lseek changes. My rational was that

You don't want this one. It's a hack that we use in order to make it easier for users to install and use dvdcss on osx. We force them to use the dylib that comes with VLC. these defines cover all current and future large file issues. For example, stat is currently used in dvd_reader.c. Although stat doesn't currently require any of the 64bit extensions, defining off_t to off64_t changes the definition of struct stat. This is going to cause mis-alignment of the member variables since it is filled in by the libc with the assumption that off_t is 32 bits and dvdread thinks it's 64 bits. Redefining stat makes sure we don't have this alignment problem.

P06: please, explain http://trac.handbrake.fr/browser/trunk/contrib/libdvdread/P06-darwin.patch

the change in dvd_reader.c fits with the comment that in the original code is wrong, but which one is correct? The comment above the test says one thing and the test does another. The comment is correct. The patch fixes the test to do what the comment says. Remember strncmp returns 0 when equal.

...

http://trac.handbrake.fr/browser/trunk/contrib/libdvdread _______________________________________________ DVDnav-discuss mailing list DVDnav-discuss@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/dvdnav-discuss

_______________________________________________ DVDnav-discuss mailing list DVDnav-discuss@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/dvdnav-discuss

Jean-Baptiste Kempf ha scritto:

On Fri, May 28, 2010 at 11:04:08AM -0700, John Stebbins wrote :

...

This one was partially applied. _MSC_VER was replaced by _WIN32. Kempf has some comments on the rest that we'll need to resolve first.

Call me j-b :)

why don't you ask Diego to get an svn account and maintain the win32 stuff? You are the only one with a global view of the needs

Jean-Baptiste Kempf ha scritto:

On Mon, May 31, 2010 at 06:11:01PM +0200, Nico Sabbi wrote :

...

why don't you ask Diego to get an svn account and maintain the win32 stuff? You are the only one with a global view of the needs

Diego, like the DonDiego from FFmpeg?

exactly. I'm forwarding this email to root

Sure, I can help to fix/improve the win32 dvdnav port. My main aim is to not have to maintain a fork :)

Best Regards,

this isthe right place to operate

On 06/01/2010 09:31 AM, John Stebbins wrote:

To keep things organized, here's current status of these patches. These have been committed: * nav-multi-pgc.patch * nav-log-stderr.patch * nav-read-name.patch

This patch was partially applied: * read-raw-device.patch The part that handles opening the raw device correctly was omitted from the commit.

These are pending: * nav-configure-uid0.patch * read-configure-uid0.patch * nav-allow-restart.patch * nav-dup-handle.patch * nav-forward-seek.patch * nav-missing-menu-abort.patch * nav-program-info.patch * nav-reset-deadlock.patch * read-block2char.patch * read-dup-lut-pgc.patch * read-mingw-large-file.patch

That was weird, a link that was invisible in my e-mail client made it into the text. List cleaned up.