Patch from the Living dead for dvdnav
Hello, I am super-annoyed, since I have a patch on dvdnav, that I apply on all VLC release, on Windows and Mac OS X, since (at least 2006). I understand absolutely nothing of this obscure patch on this obscure piece of code... It does work quite, but I can't explain it, since it predates my work on the project. I thought I should share it, with you, upstream, as someone could make some sense out of it. With my best (sorry) regards -- Jean-Baptiste Kempf http://www.jbkempf.com/ - +33 672 704 734 Sent from my Electronic Device
I am super-annoyed, since I have a patch on dvdnav, that I apply on all VLC release, on Windows and Mac OS X, since (at least 2006). Any commit notes or anything?
On Wed, Nov 02, 2011 at 05:33:35PM -0600, Roger Pack wrote :
I am super-annoyed, since I have a patch on dvdnav, that I apply on all VLC release, on Windows and Mac OS X, since (at least 2006). Any commit notes or anything?
Nothing useful except "dvdnav: support for new incarnation hosted in mplayerhq" or "Fix libdvdnav in a reliable and quite clean way. Unsure about my sanity now though." Best Regards, -- Jean-Baptiste Kempf http://www.jbkempf.com/ - +33 672 704 734 Sent from my Electronic Device
I am super-annoyed, since I have a patch on dvdnav, that I apply on all VLC release, on Windows and Mac OS X, since (at least 2006).
Are you saying you only apply this patch to Windows and Mac OS X builds of VLC?
I understand absolutely nothing of this obscure patch on this obscure piece of code...
Obscure is right. Only constants are used and the values do not seem to follow a pattern.
It does work quite, but I can't explain it, since it predates my work on the project.
I thought I should share it, with you, upstream, as someone could make some sense out of it.
If you submitted the same patch but w/ the different OS #ifdefs it might be accepted. But otherwise, this thing is a mess. E -- Erik Hovland erik@hovland.org http://hovland.org/
On Wed, Nov 02, 2011 at 04:36:55PM -0700, Erik Hovland wrote :
I am super-annoyed, since I have a patch on dvdnav, that I apply on all VLC release, on Windows and Mac OS X, since (at least 2006).
Are you saying you only apply this patch to Windows and Mac OS X builds of VLC?
Yes, because I don't build for other platforms, that have a correct package-manager.
I understand absolutely nothing of this obscure patch on this obscure piece of code...
Obscure is right. Only constants are used and the values do not seem to follow a pattern.
I didn't say "piece of crap" patch, but I wasn't far.
I thought I should share it, with you, upstream, as someone could make some sense out of it.
If you submitted the same patch but w/ the different OS #ifdefs it might be accepted. But otherwise, this thing is a mess.
Ok Best Regards, -- Jean-Baptiste Kempf http://www.jbkempf.com/ - +33 672 704 734 Sent from my Electronic Device
On 11/02/2011 04:30 PM, Jean-Baptiste Kempf wrote:
Hello,
I am super-annoyed, since I have a patch on dvdnav, that I apply on all VLC release, on Windows and Mac OS X, since (at least 2006).
I understand absolutely nothing of this obscure patch on this obscure piece of code...
It does work quite, but I can't explain it, since it predates my work on the project.
I thought I should share it, with you, upstream, as someone could make some sense out of it.
With my best (sorry) regards
Am I reading this cross-eyed or is this just totally stupid? off = lseek( fd, 32 * (off_t) DVD_VIDEO_LB_LEN, SEEK_SET ); - if( off == ( 32 * (off_t) DVD_VIDEO_LB_LEN ) ) { + if (off == 16) { unless lseek errors, off should always == 32 * DVD_VIDEO_LB_LEN and that new test should never succeed. -- John GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01 83F0 49F1 D7B2 60D4 D0F7
Am I reading this cross-eyed or is this just totally stupid?
off = lseek( fd, 32 * (off_t) DVD_VIDEO_LB_LEN, SEEK_SET ); - if( off == ( 32 * (off_t) DVD_VIDEO_LB_LEN ) ) { + if (off == 16) {
unless lseek errors, off should always == 32 * DVD_VIDEO_LB_LEN and that new test should never succeed.
I was just reading and saying the same things to myself. If Jean applies this I don't think it ever fires. Or lseek returns something magical on OSX and Windows. I take it HandBrake does not apply this patch? E -- Erik Hovland erik@hovland.org http://hovland.org/
On Wed, Nov 02, 2011 at 04:46:13PM -0700, John Stebbins wrote :
Am I reading this cross-eyed or is this just totally stupid?
This is totally stupid. Looking closer, the patch evolved during the mess, the original, attached here, was less stupid, but seems to depend on a non-installed dvdread header. -- Jean-Baptiste Kempf http://www.jbkempf.com/ - +33 672 704 734 Sent from my Electronic Device
On 11/02/2011 04:58 PM, Jean-Baptiste Kempf wrote:
On Wed, Nov 02, 2011 at 04:46:13PM -0700, John Stebbins wrote :
Am I reading this cross-eyed or is this just totally stupid? This is totally stupid.
Looking closer, the patch evolved during the mess, the original, attached here, was less stupid, but seems to depend on a non-installed dvdread header.
After reading this over, I think I see what is going on here. This patch is using the ECMA-167 primary volume descriptor that is located at sector 16 instead of the UDF primary volume descriptor that the original code assumes is at sector 32. If I'm reading the UDF specification correctly, there is no guarantee that the UDF primary volume descriptor will be at this location. You need to read an anchor volume descriptor pointer first which will point to the main volume descriptor. So maybe there are some discs out the for which the original code does not work. The patch however is doing something pretty funky for the serial number. It is using the volume creation date as a serial number which isn't going to be unique by any measure. The original code uses the first 16 bytes of the volume set identifier (which is 128 bytes in length). Looking at a sample of the volume set identifier, the first 8 bytes look like some kind of serial number. The next 8 are zero. And then the disc title is repeated again. Anyway, it might be interesting to think about doing a patch that look up the udf volume descriptor correctly instead of falling back to the ecma-167 volume descriptor. -- John GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01 83F0 49F1 D7B2 60D4 D0F7
After reading this over, I think I see what is going on here. This patch is using the ECMA-167 primary volume descriptor that is located at sector 16 instead of the UDF primary volume descriptor that the original code assumes is at sector 32. If I'm reading the UDF specification correctly, there is no guarantee that the UDF primary volume descriptor will be at this location. You need to read an anchor volume descriptor pointer first which will point to the main volume descriptor. So maybe there are some discs out the for which the original code does not work.
The patch however is doing something pretty funky for the serial number. It is using the volume creation date as a serial number which isn't going to be unique by any measure. The original code uses the first 16 bytes of the volume set identifier (which is 128 bytes in length). Looking at a sample of the volume set identifier, the first 8 bytes look like some kind of serial number. The next 8 are zero. And then the disc title is repeated again.
Anyway, it might be interesting to think about doing a patch that look up the udf volume descriptor correctly instead of falling back to the ecma-167 volume descriptor.
I am mostly w/ John on this one. What needs to happen is for this part of the code to get reworked to be compliant w/ the specification and then work from there. What I am especially uncomfortable w/ is that we do not have an example DVD where this code breaks down. What we do know is that many users use an unpatched version of libdvdnav and they are not reporting a problem that can be associated w/ this area of code. E -- Erik Hovland erik@hovland.org http://hovland.org/
On Thu, Nov 03, 2011 at 11:11:58AM -0700, Erik Hovland wrote :
What I am especially uncomfortable w/ is that we do not have
Erik, please, do not misunderstand me: I am just sending upstream, the patches I have, especially the ones I don't feel confident about, so upstream can be the best. I don't expect those to be merged. However, I doubt Original Coder did it just for fun... :) Best Regards, -- Jean-Baptiste Kempf http://www.jbkempf.com/ - +33 672 704 734 Sent from my Electronic Device
On Thu, Nov 3, 2011 at 11:15 AM, Jean-Baptiste Kempf <jb@videolan.org> wrote:
On Thu, Nov 03, 2011 at 11:11:58AM -0700, Erik Hovland wrote :
What I am especially uncomfortable w/ is that we do not have
Erik, please, do not misunderstand me: I am just sending upstream, the patches I have, especially the ones I don't feel confident about, so upstream can be the best. I don't expect those to be merged.
However, I doubt Original Coder did it just for fun... :)
Sorry, I was just being blunt. I understand that you aren't trying to get this patch accepted, just inspected. I get where you are coming from. E -- Erik Hovland erik@hovland.org http://hovland.org/
On Thu, Nov 03, 2011 at 11:17:32AM -0700, Erik Hovland wrote :
On Thu, Nov 3, 2011 at 11:15 AM, Jean-Baptiste Kempf <jb@videolan.org> wrote:
On Thu, Nov 03, 2011 at 11:11:58AM -0700, Erik Hovland wrote :
What I am especially uncomfortable w/ is that we do not have
Erik, please, do not misunderstand me: I am just sending upstream, the patches I have, especially the ones I don't feel confident about, so upstream can be the best. I don't expect those to be merged.
However, I doubt Original Coder did it just for fun... :)
Sorry, I was just being blunt. I understand that you aren't trying to get this patch accepted, just inspected. I get where you are coming from.
No problem. To me, the conclusion of this is thread is that: - this patch is crap and should be dropped (I'll do that quite soon). Which is what I expected, to be honest. - a patch for a better handly of udf volume descriptor would be nice to have. Thanks a lot for the support :D Best Regards, -- Jean-Baptiste Kempf http://www.jbkempf.com/ - +33 672 704 734 Sent from my Electronic Device
participants (4)
-
Erik Hovland -
Jean-Baptiste Kempf -
John Stebbins -
Roger Pack