UDFGetVolumeIdentifier still broken; bug 1451
I need a simple commandline program to output the volume name of a dvd. Oddly enough, I can't find one, and udf looks cryptic enough that I didn't want to roll my own, so I tried writing one with libdvdread. It was easy... but didn't work; it ran smack dab into http://bugzilla.mplayerhq.hu/show_bug.cgi?id=1451 (This was with ubuntu-10.04 and their patched libdvdread-4.1.3.) The analysis there looks correct. Is this library maintained? I attached an example program to the bug. Thanks, Dan
On Sunday, 21 November 2010 at 23:34, Dan Kegel wrote:
I need a simple commandline program to output the volume name of a dvd. Oddly enough, I can't find one, and udf looks cryptic enough that I didn't want to roll my own, so I tried writing one with libdvdread. It was easy... but didn't work; it ran smack dab into http://bugzilla.mplayerhq.hu/show_bug.cgi?id=1451 (This was with ubuntu-10.04 and their patched libdvdread-4.1.3.)
The analysis there looks correct. Is this library maintained?
I attached an example program to the bug.
It seems to work fine with current SVN. Maybe it's time to make a new release. 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 Sun, Nov 21, 2010 at 11:53:40PM +0100, Dominik 'Rathann' Mierzejewski wrote :
Maybe it's time to make a new release.
This is a very good idea. Best Regards, -- Jean-Baptiste Kempf http://www.jbkempf.com/ +33 672 704 734
It seems to work fine with current SVN. Maybe it's time to make a new release.
Before a release we should at least fix the thing I broke when trying to rework the patches from John Stebbins. He already re-submitted all of the patches and included one to fix what I broke. I'll try to get at least that patch in. E -- Erik Hovland erik@hovland.org http://hovland.org/
It seems to work fine with current SVN. Maybe it's time to make a new release.
Before a release we should at least fix the thing I broke when trying to rework the patches from John Stebbins. He already re-submitted all of the patches and included one to fix what I broke. I'll try to get at least that patch in.
I just committed John's patch to avoid abort if a menu is missing. I am ready for a release. If you want the rest of John's patches in before the release, that might take a bit more review. The most beneficial of those patches (ref counting the main data structure) is also the most invasive. E -- Erik Hovland erik@hovland.org http://hovland.org/
Dominik 'Rathann' Mierzejewski <dominik@greysector.net> wrote:
On Sunday, 21 November 2010 at 23:34, Dan Kegel wrote:
It seems to work fine with current SVN. Maybe it's time to make a new release.
Ah, sorry, once I saw the open bug, I didn't check SVN. Perhaps you should mark the bug fixed now, and close it when you do the release. One more nit: I want to use this library inside volname, but the warning libdvdread: Encrypted DVD support unavailable. ************************************************ ** ** ** No css library available. See ** ** /usr/share/doc/libdvdread4/README.Debian ** ** for more information. ** ** ** ************************************************ is output every time I call DVDOpen(). This seems excessive, and makes me want to not use libdvdread inside volname. Assuming the one does not need encrypted dvd support to read volume names, can you move that error message later, so it doesn't annoy users without reason? Thanks! - Dan
On Sun, Nov 21, 2010 at 11:43 PM, Dan Kegel <dank@kegel.com> wrote:
One more nit: I want to use this library inside volname, but the warning
libdvdread: Encrypted DVD support unavailable. ************************************************ ** ** ** No css library available. See ** ** /usr/share/doc/libdvdread4/README.Debian ** ** for more information. ** ** ** ************************************************
is output every time I call DVDOpen(). This seems excessive, and makes me want to not use libdvdread inside volname.
Assuming the one does not need encrypted dvd support to read volume names, can you move that error message later, so it doesn't annoy users without reason?
I see the latter part of that message is debian/ubuntu's, but the first part and the logic to present it is yours, so my nit still stands even though I shouldn't have included the debian part of the message in my email. - Dan
One more nit: I want to use this library inside volname, but the warning
libdvdread: Encrypted DVD support unavailable. ************************************************ ** ** ** No css library available. See ** ** /usr/share/doc/libdvdread4/README.Debian ** ** for more information. ** ** ** ************************************************
is output every time I call DVDOpen(). This seems excessive, and makes me want to not use libdvdread inside volname.
Assuming the one does not need encrypted dvd support to read volume names, can you move that error message later, so it doesn't annoy users without reason?
I see the latter part of that message is debian/ubuntu's, but the first part and the logic to present it is yours, so my nit still stands even though I shouldn't have included the debian part of the message in my email.
The first part seems reasonable considering it is a true statement. It even goes to stderr, so it can be easily piped away. Are you proposing that we only report this fact when the caller tries to do something w/ an encrypted DVD? If that is the case, it is a bit harder because the non-CSS calls do not detect that CSS is the reason for failure. E -- Erik Hovland erik@hovland.org http://hovland.org/
On Sunday 21 November 2010, Dominik 'Rathann' Mierzejewski wrote:
It seems to work fine with current SVN. Maybe it's time to make a new release.
Then, could you also review and/or apply the patch I sent on October ? http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2010-October/001303.html If any action is required on my side such as removing the if (p) free(p), then let me know. Thanks, Frederic
Then, could you also review and/or apply the patch I sent on October ?
http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2010-October/001303.html
If any action is required on my side such as removing the if (p) free(p), then let me know.
Please take out the changes that change 0 to NULL. They only make a cosmetic change that makes it harder to review the actual changes. The added conditional at or around line 1351 has a couple of magic numbers in it (8*2 to be exact). It sure would help if they were #defines or const uint8_t w/ a reasonable name. The conditional says that it "may" be a good idea. Is it, or isn't it. Consider breaking this conditional out into a separate patch since it does not address the double free issue directly. Separate out the changes made to printouts in another patch like the one that says "libdvdread: Unable to seek PTL_MAIT table". Thanks for the patch and reminding us to take a look. Sorry for the slowness. E -- Erik Hovland erik@hovland.org http://hovland.org/
participants (5)
-
Dan Kegel -
Dominik 'Rathann' Mierzejewski -
Erik Hovland -
Frédéric Marchal -
Jean-Baptiste Kempf