[PATCH] handle duplicate language units and pgc's more gracefully
This patch never got accepted (probably because it's touches a lot and made you nervous), but HandBrake has been applying this for 3 years now. Some discs have large numbers of repeated language units and pgc's that cause extreme memory consumption and file i/o issues. scanning all the titles on such discs is unusably slow. detect such repeats and ref-count the data structures to avoid reading and storing duplicate info.
I like this patch. I can see the utility of this change. The one thing that makes me nervous is that the reference counting is done by modifying the public structs and storing the reference count there. I am likely to accept this patch just b/c it has been going for so long, but I would like to think about the solution a bit before giving up on a solution that puts the ref counting outside of the structs. Any thoughts are appreciated. E On Sat, Oct 13, 2012 at 6:00 AM, John Stebbins <stebbins@jetheaddev.com> wrote:
This patch never got accepted (probably because it's touches a lot and made you nervous), but HandBrake has been applying this for 3 years now.
Some discs have large numbers of repeated language units and pgc's that cause extreme memory consumption and file i/o issues. scanning all the titles on such discs is unusably slow. detect such repeats and ref-count the data structures to avoid reading and storing duplicate info.
_______________________________________________ DVDnav-discuss mailing list DVDnav-discuss@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/dvdnav-discuss
-- Erik Hovland erik@hovland.org http://hovland.org/
John Stebbins <stebbins@jetheaddev.com> wrote:
This patch never got accepted (probably because it's touches a lot and made you nervous), but HandBrake has been applying this for 3 years now.
Some discs have large numbers of repeated language units and pgc's that cause extreme memory consumption and file i/o issues. scanning all the titles on such discs is unusably slow. detect such repeats and ref-count the data structures to avoid reading and storing duplicate info.
A couple of days ago dvdbackup got killed after the system ran out of swap space (2 GB), I applied this patch and it solved the problem. Awesome. While I assumed that the changes from malloc() to calloc() might also make crashes due to bogus pgc field values less likely, the patch actually causes at least one crash that doesn't happen without it: Program terminated with signal 10, Bus error. #0 0x0000000802a2390a in dvdnav_describe_title_chapters (this=0x80cd03e00, title=17, times=0x7fffff8fa298, duration=0x7fffff8fa2a0) at /usr/obj-ports/usr/ports/multimedia/libdvdnav/work/libdvdnav-4.2.0/src/searching.c:628 628 if(ptt[i].pgn > pgc->nr_of_programs) { (gdb) p *pgc Cannot access memory at address 0xe000e000000a620 Everyone's favourite function dvdnav_describe_title_chapters() strikes again ... I haven't tracked down the cause yet. Letting the dup_* functions always return -1 doesn't seem to affect the problem and neither does modifying find_dup_pgc() to compare the whole pgc instead of just the start_byte or removing the added return in ifoRead_VTS_PTT_SRPT(). As far as I can tell the DVD doesn't actually have duplicated pgcs. Fabian
On 11/04/2012 02:51 PM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
This patch never got accepted (probably because it's touches a lot and made you nervous), but HandBrake has been applying this for 3 years now.
Some discs have large numbers of repeated language units and pgc's that cause extreme memory consumption and file i/o issues. scanning all the titles on such discs is unusably slow. detect such repeats and ref-count the data structures to avoid reading and storing duplicate info. A couple of days ago dvdbackup got killed after the system ran out of swap space (2 GB), I applied this patch and it solved the problem. Awesome.
While I assumed that the changes from malloc() to calloc() might also make crashes due to bogus pgc field values less likely, the patch actually causes at least one crash that doesn't happen without it:
Program terminated with signal 10, Bus error. #0 0x0000000802a2390a in dvdnav_describe_title_chapters (this=0x80cd03e00, title=17, times=0x7fffff8fa298, duration=0x7fffff8fa2a0) at /usr/obj-ports/usr/ports/multimedia/libdvdnav/work/libdvdnav-4.2.0/src/searching.c:628 628 if(ptt[i].pgn > pgc->nr_of_programs) { (gdb) p *pgc Cannot access memory at address 0xe000e000000a620
Everyone's favourite function dvdnav_describe_title_chapters() strikes again ...
I haven't tracked down the cause yet.
Letting the dup_* functions always return -1 doesn't seem to affect the problem and neither does modifying find_dup_pgc() to compare the whole pgc instead of just the start_byte or removing the added return in ifoRead_VTS_PTT_SRPT().
As far as I can tell the DVD doesn't actually have duplicated pgcs.
Fabian
What disc does it crash on? HandBrake does not use dvdnav_describe_title_chapters(), so this is a code path we have not tested well enough it sounds like. dvdbackup uses this?
John Stebbins <stebbins@jetheaddev.com> wrote:
On 11/04/2012 02:51 PM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
This patch never got accepted (probably because it's touches a lot and made you nervous), but HandBrake has been applying this for 3 years now.
Some discs have large numbers of repeated language units and pgc's that cause extreme memory consumption and file i/o issues. scanning all the titles on such discs is unusably slow. detect such repeats and ref-count the data structures to avoid reading and storing duplicate info. A couple of days ago dvdbackup got killed after the system ran out of swap space (2 GB), I applied this patch and it solved the problem. Awesome.
While I assumed that the changes from malloc() to calloc() might also make crashes due to bogus pgc field values less likely, the patch actually causes at least one crash that doesn't happen without it:
Program terminated with signal 10, Bus error. #0 0x0000000802a2390a in dvdnav_describe_title_chapters (this=0x80cd03e00, title=17, times=0x7fffff8fa298, duration=0x7fffff8fa2a0) at /usr/obj-ports/usr/ports/multimedia/libdvdnav/work/libdvdnav-4.2.0/src/searching.c:628 628 if(ptt[i].pgn > pgc->nr_of_programs) { (gdb) p *pgc Cannot access memory at address 0xe000e000000a620
Everyone's favourite function dvdnav_describe_title_chapters() strikes again ...
I haven't tracked down the cause yet.
Letting the dup_* functions always return -1 doesn't seem to affect the problem and neither does modifying find_dup_pgc() to compare the whole pgc instead of just the start_byte or removing the added return in ifoRead_VTS_PTT_SRPT().
As far as I can tell the DVD doesn't actually have duplicated pgcs.
What disc does it crash on? HandBrake does not use dvdnav_describe_title_chapters(), so this is a code path we have not tested well enough it sounds like. dvdbackup uses this?
dvdbackup doesn't use dvdnav_describe_title_chapters() either. I failed to mention that the crashing application is vlc. The DVD is a rip of Monty Python season 3 DVD 1 where unreadable sector regions have been padded with surrounding sectors using a flawed padding strategy, which probably made the "copy" even less standard-compliant than the original. Given the content, it seems very appropriate that the attached patch, which prevents this and other crashes, is somewhat silly ... I made the skip conditions up while going through backtraces until vlc stopped crashing. I haven't properly tested yet how they affect other DVDs. Fabian
On 11/04/2012 06:33 PM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
On 11/04/2012 02:51 PM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
This patch never got accepted (probably because it's touches a lot and made you nervous), but HandBrake has been applying this for 3 years now.
Some discs have large numbers of repeated language units and pgc's that cause extreme memory consumption and file i/o issues. scanning all the titles on such discs is unusably slow. detect such repeats and ref-count the data structures to avoid reading and storing duplicate info. A couple of days ago dvdbackup got killed after the system ran out of swap space (2 GB), I applied this patch and it solved the problem. Awesome.
While I assumed that the changes from malloc() to calloc() might also make crashes due to bogus pgc field values less likely, the patch actually causes at least one crash that doesn't happen without it:
Program terminated with signal 10, Bus error. #0 0x0000000802a2390a in dvdnav_describe_title_chapters (this=0x80cd03e00, title=17, times=0x7fffff8fa298, duration=0x7fffff8fa2a0) at /usr/obj-ports/usr/ports/multimedia/libdvdnav/work/libdvdnav-4.2.0/src/searching.c:628 628 if(ptt[i].pgn > pgc->nr_of_programs) { (gdb) p *pgc Cannot access memory at address 0xe000e000000a620
Everyone's favourite function dvdnav_describe_title_chapters() strikes again ...
I haven't tracked down the cause yet.
Letting the dup_* functions always return -1 doesn't seem to affect the problem and neither does modifying find_dup_pgc() to compare the whole pgc instead of just the start_byte or removing the added return in ifoRead_VTS_PTT_SRPT().
As far as I can tell the DVD doesn't actually have duplicated pgcs. What disc does it crash on? HandBrake does not use dvdnav_describe_title_chapters(), so this is a code path we have not tested well enough it sounds like. dvdbackup uses this? dvdbackup doesn't use dvdnav_describe_title_chapters() either.
I failed to mention that the crashing application is vlc.
The DVD is a rip of Monty Python season 3 DVD 1 where unreadable sector regions have been padded with surrounding sectors using a flawed padding strategy, which probably made the "copy" even less standard-compliant than the original.
Given the content, it seems very appropriate that the attached patch, which prevents this and other crashes, is somewhat silly ...
I made the skip conditions up while going through backtraces until vlc stopped crashing. I haven't properly tested yet how they affect other DVDs.
Thanks. I have the whole Monty Python set. So when I get home in a couple of weeks, I'll try to reproduce and see if I can track down the root cause. Can you give me more details about how you created the rip so I can reproduce? It may be a bad rip, but I don't want to make dvdnav more fragile, so it's worth looking into.
John Stebbins <stebbins@jetheaddev.com> wrote:
On 11/04/2012 06:33 PM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
On 11/04/2012 02:51 PM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
This patch never got accepted (probably because it's touches a lot and made you nervous), but HandBrake has been applying this for 3 years now.
Some discs have large numbers of repeated language units and pgc's that cause extreme memory consumption and file i/o issues. scanning all the titles on such discs is unusably slow. detect such repeats and ref-count the data structures to avoid reading and storing duplicate info. A couple of days ago dvdbackup got killed after the system ran out of swap space (2 GB), I applied this patch and it solved the problem. Awesome.
While I assumed that the changes from malloc() to calloc() might also make crashes due to bogus pgc field values less likely, the patch actually causes at least one crash that doesn't happen without it:
Program terminated with signal 10, Bus error. #0 0x0000000802a2390a in dvdnav_describe_title_chapters (this=0x80cd03e00, title=17, times=0x7fffff8fa298, duration=0x7fffff8fa2a0) at /usr/obj-ports/usr/ports/multimedia/libdvdnav/work/libdvdnav-4.2.0/src/searching.c:628 628 if(ptt[i].pgn > pgc->nr_of_programs) { (gdb) p *pgc Cannot access memory at address 0xe000e000000a620
Everyone's favourite function dvdnav_describe_title_chapters() strikes again ...
I haven't tracked down the cause yet.
Letting the dup_* functions always return -1 doesn't seem to affect the problem and neither does modifying find_dup_pgc() to compare the whole pgc instead of just the start_byte or removing the added return in ifoRead_VTS_PTT_SRPT().
As far as I can tell the DVD doesn't actually have duplicated pgcs. What disc does it crash on? HandBrake does not use dvdnav_describe_title_chapters(), so this is a code path we have not tested well enough it sounds like. dvdbackup uses this? dvdbackup doesn't use dvdnav_describe_title_chapters() either.
I failed to mention that the crashing application is vlc.
The DVD is a rip of Monty Python season 3 DVD 1 where unreadable sector regions have been padded with surrounding sectors using a flawed padding strategy, which probably made the "copy" even less standard-compliant than the original.
Given the content, it seems very appropriate that the attached patch, which prevents this and other crashes, is somewhat silly ...
I made the skip conditions up while going through backtraces until vlc stopped crashing. I haven't properly tested yet how they affect other DVDs.
I haven't noticed any problems with the patches so far. I recently had to use another patch (attached) for "Ghost Protocol", but I haven't verified that the problem actually was due to the pgc deduplication.
Thanks. I have the whole Monty Python set. So when I get home in a couple of weeks, I'll try to reproduce and see if I can track down the root cause. Can you give me more details about how you created the rip so I can reproduce? It may be a bad rip, but I don't want to make dvdnav more fragile, so it's worth looking into.
The rip was created with vobcopy patched with: http://www.fabiankeil.de/sourcecode/vobcopy-1.2.0-wip.patch The command line was: vobcopy -m -F $BLOCK_COUNT -v -s -S -i /cdrom/ BLOCK_COUNT was most likely 64. The time stamps seem to imply that the commit bea80e5930 might have been used, however due to rebasing it likely isn't an exact match and recreating the problem with this patch set may not be possible. Fabian
On 12/28/2012 04:15 AM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
On 11/04/2012 06:33 PM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
On 11/04/2012 02:51 PM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
This patch never got accepted (probably because it's touches a lot and made you nervous), but HandBrake has been applying this for 3 years now.
Some discs have large numbers of repeated language units and pgc's that cause extreme memory consumption and file i/o issues. scanning all the titles on such discs is unusably slow. detect such repeats and ref-count the data structures to avoid reading and storing duplicate info. A couple of days ago dvdbackup got killed after the system ran out of swap space (2 GB), I applied this patch and it solved the problem. Awesome.
While I assumed that the changes from malloc() to calloc() might also make crashes due to bogus pgc field values less likely, the patch actually causes at least one crash that doesn't happen without it:
Program terminated with signal 10, Bus error. #0 0x0000000802a2390a in dvdnav_describe_title_chapters (this=0x80cd03e00, title=17, times=0x7fffff8fa298, duration=0x7fffff8fa2a0) at /usr/obj-ports/usr/ports/multimedia/libdvdnav/work/libdvdnav-4.2.0/src/searching.c:628 628 if(ptt[i].pgn > pgc->nr_of_programs) { (gdb) p *pgc Cannot access memory at address 0xe000e000000a620
Everyone's favourite function dvdnav_describe_title_chapters() strikes again ...
I haven't tracked down the cause yet.
Letting the dup_* functions always return -1 doesn't seem to affect the problem and neither does modifying find_dup_pgc() to compare the whole pgc instead of just the start_byte or removing the added return in ifoRead_VTS_PTT_SRPT().
As far as I can tell the DVD doesn't actually have duplicated pgcs. What disc does it crash on? HandBrake does not use dvdnav_describe_title_chapters(), so this is a code path we have not tested well enough it sounds like. dvdbackup uses this? dvdbackup doesn't use dvdnav_describe_title_chapters() either.
I failed to mention that the crashing application is vlc.
The DVD is a rip of Monty Python season 3 DVD 1 where unreadable sector regions have been padded with surrounding sectors using a flawed padding strategy, which probably made the "copy" even less standard-compliant than the original.
Given the content, it seems very appropriate that the attached patch, which prevents this and other crashes, is somewhat silly ...
I made the skip conditions up while going through backtraces until vlc stopped crashing. I haven't properly tested yet how they affect other DVDs. I haven't noticed any problems with the patches so far.
I recently had to use another patch (attached) for "Ghost Protocol", but I haven't verified that the problem actually was due to the pgc deduplication.
Thanks. I have the whole Monty Python set. So when I get home in a couple of weeks, I'll try to reproduce and see if I can track down the root cause. Can you give me more details about how you created the rip so I can reproduce? It may be a bad rip, but I don't want to make dvdnav more fragile, so it's worth looking into. The rip was created with vobcopy patched with: http://www.fabiankeil.de/sourcecode/vobcopy-1.2.0-wip.patch
The command line was: vobcopy -m -F $BLOCK_COUNT -v -s -S -i /cdrom/ BLOCK_COUNT was most likely 64.
The time stamps seem to imply that the commit bea80e5930 might have been used, however due to rebasing it likely isn't an exact match and recreating the problem with this patch set may not be possible.
Fabian
This error is not due to the pgc deduplication. It is caused by an invalid cell_playback_offset value on the disc. libdvdread does not allocate a cell_playback in this case and applications must check that the value is not null. -- John GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01 83F0 49F1 D7B2 60D4 D0F7
John Stebbins <stebbins@jetheaddev.com> wrote:
On 12/28/2012 04:15 AM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
On 11/04/2012 06:33 PM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
What disc does it crash on? HandBrake does not use dvdnav_describe_title_chapters(), so this is a code path we have not tested well enough it sounds like. dvdbackup uses this? dvdbackup doesn't use dvdnav_describe_title_chapters() either.
I failed to mention that the crashing application is vlc.
The DVD is a rip of Monty Python season 3 DVD 1 where unreadable sector regions have been padded with surrounding sectors using a flawed padding strategy, which probably made the "copy" even less standard-compliant than the original.
Given the content, it seems very appropriate that the attached patch, which prevents this and other crashes, is somewhat silly ...
I made the skip conditions up while going through backtraces until vlc stopped crashing. I haven't properly tested yet how they affect other DVDs. I haven't noticed any problems with the patches so far.
I recently had to use another patch (attached) for "Ghost Protocol", but I haven't verified that the problem actually was due to the pgc deduplication.
This error is not due to the pgc deduplication. It is caused by an invalid cell_playback_offset value on the disc. libdvdread does not allocate a cell_playback in this case and applications must check that the value is not null.
Do you count libdvdnav as application here, or is vlc really expected to check for this (and all the other things libdvdread might not allocate) before calling dvdnav_describe_title_chapters()? Fabian
On 12/29/2012 10:55 AM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
On 11/04/2012 06:33 PM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
What disc does it crash on? HandBrake does not use dvdnav_describe_title_chapters(), so this is a code path we have not tested well enough it sounds like. dvdbackup uses this? dvdbackup doesn't use dvdnav_describe_title_chapters() either.
I failed to mention that the crashing application is vlc.
The DVD is a rip of Monty Python season 3 DVD 1 where unreadable sector regions have been padded with surrounding sectors using a flawed padding strategy, which probably made the "copy" even less standard-compliant than the original.
Given the content, it seems very appropriate that the attached patch, which prevents this and other crashes, is somewhat silly ...
I made the skip conditions up while going through backtraces until vlc stopped crashing. I haven't properly tested yet how they affect other DVDs. I haven't noticed any problems with the patches so far.
I recently had to use another patch (attached) for "Ghost Protocol", but I haven't verified that the problem actually was due to the pgc deduplication. This error is not due to the pgc deduplication. It is caused by an invalid cell_playback_offset value on the disc.
On 12/28/2012 04:15 AM, Fabian Keil wrote: libdvdread does not allocate a cell_playback in this case and applications must check that the value is not null. Do you count libdvdnav as application here, or is vlc really expected to check for this (and all the other things libdvdread might not allocate) before calling dvdnav_describe_title_chapters()?
Fabian
I think some variant of your patch should be committed (although one of the comparisons seems to be trivailly false all the time). I was just pointing out that some applications also *use* cell_playback directly and need to check it for null. So some applications would never provoke this potential crash in dvdnav_describe_title_chapters(). There are actually many other places in libdvdnav where cell_playback is used unchecked. I'm guessing the reason these don't provoke obvious frequent crashes is that a title that contains a pgc with cell_playback_offset == 0 is an invalid title and won't be accessible from the dvd menus. So that makes me think that an application that provokes the crash that is fixed by your patch is probably not strictly providing access based on the dvd menus and therefor may be providing other means to provoke similar crashes. FYI, this is the comparison that I believe is always false "&pgc->cell_playback[cellnr-1] == NULL". It takes the address of a variable which will always be non-null and compares it to null. -- John GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01 83F0 49F1 D7B2 60D4 D0F7
John Stebbins <stebbins@jetheaddev.com> wrote:
On 12/29/2012 10:55 AM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
On 11/04/2012 06:33 PM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote: > What disc does it crash on? HandBrake does not use > dvdnav_describe_title_chapters(), so this is a code path we have not > tested well enough it sounds like. dvdbackup uses this? dvdbackup doesn't use dvdnav_describe_title_chapters() either.
I failed to mention that the crashing application is vlc.
The DVD is a rip of Monty Python season 3 DVD 1 where unreadable sector regions have been padded with surrounding sectors using a flawed padding strategy, which probably made the "copy" even less standard-compliant than the original.
Given the content, it seems very appropriate that the attached patch, which prevents this and other crashes, is somewhat silly ...
I made the skip conditions up while going through backtraces until vlc stopped crashing. I haven't properly tested yet how they affect other DVDs. I haven't noticed any problems with the patches so far.
I recently had to use another patch (attached) for "Ghost Protocol", but I haven't verified that the problem actually was due to the pgc deduplication. This error is not due to the pgc deduplication. It is caused by an invalid cell_playback_offset value on the disc.
On 12/28/2012 04:15 AM, Fabian Keil wrote: libdvdread does not allocate a cell_playback in this case and applications must check that the value is not null. Do you count libdvdnav as application here, or is vlc really expected to check for this (and all the other things libdvdread might not allocate) before calling dvdnav_describe_title_chapters()?
I think some variant of your patch should be committed (although one of the comparisons seems to be trivailly false all the time). I was just pointing out that some applications also *use* cell_playback directly and need to check it for null. So some applications would never provoke this potential crash in dvdnav_describe_title_chapters().
Thanks for the explanation and the patch review.
There are actually many other places in libdvdnav where cell_playback is used unchecked. I'm guessing the reason these don't provoke obvious frequent crashes is that a title that contains a pgc with cell_playback_offset == 0 is an invalid title and won't be accessible from the dvd menus. So that makes me think that an application that provokes the crash that is fixed by your patch is probably not strictly providing access based on the dvd menus and therefor may be providing other means to provoke similar crashes.
My impression is that vlc uses it to generate the title lengths in the title menu. The crashes in dvdnav_describe_title_chapters() already happen while opening the DVD, though, which at least makes debugging easier. Unfortunately DVDs that trigger crashes with unpatched libdvdnav versions usually also have a somewhat useless title menu, as it's unclear from the lengths what the actual main title is: http://www.fabiankeil.de/tmp/screenshot-vlc-title-menu-for-ghost-protocol.jp... From a user's perspective it would be great to also have a menu that only shows titles that are reachable through the menu, but I assume that's not trivial.
FYI, this is the comparison that I believe is always false "&pgc->cell_playback[cellnr-1] == NULL". It takes the address of a variable which will always be non-null and compares it to null.
(gdb) p &pgc->cell_playback[cellnr-1] $2 = (cell_playback_t *) 0x0 (gdb) p pgc->cell_playback $3 = (cell_playback_t *) 0x0 (gdb) p cellnr $4 = 1 I agree that the check is useless, though, and the adjusted patch (attached) indeed still works as expected. I believe it was the first (insufficient) fix attempt and I only added the more reasonable pgc->cell_playback check later on without realising that it's sufficient. After all the main goal was being able to watch the movie ... Fabian
On 12/30/2012 06:57 AM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
On 11/04/2012 06:33 PM, Fabian Keil wrote: > John Stebbins <stebbins@jetheaddev.com> wrote: >> What disc does it crash on? HandBrake does not use >> dvdnav_describe_title_chapters(), so this is a code path we have not >> tested well enough it sounds like. dvdbackup uses this? > dvdbackup doesn't use dvdnav_describe_title_chapters() either. > > I failed to mention that the crashing application is vlc. > > The DVD is a rip of Monty Python season 3 DVD 1 where unreadable > sector regions have been padded with surrounding sectors using a > flawed padding strategy, which probably made the "copy" even less > standard-compliant than the original. > > Given the content, it seems very appropriate that the attached patch, > which prevents this and other crashes, is somewhat silly ... > > I made the skip conditions up while going through backtraces until > vlc stopped crashing. I haven't properly tested yet how they affect > other DVDs. I haven't noticed any problems with the patches so far.
I recently had to use another patch (attached) for "Ghost Protocol", but I haven't verified that the problem actually was due to the pgc deduplication. This error is not due to the pgc deduplication. It is caused by an invalid cell_playback_offset value on the disc.
On 12/28/2012 04:15 AM, Fabian Keil wrote: libdvdread does not allocate a cell_playback in this case and applications must check that the value is not null. Do you count libdvdnav as application here, or is vlc really expected to check for this (and all the other things libdvdread might not allocate) before calling dvdnav_describe_title_chapters()? I think some variant of your patch should be committed (although one of the comparisons seems to be trivailly false all
On 12/29/2012 10:55 AM, Fabian Keil wrote: the time). I was just pointing out that some applications also *use* cell_playback directly and need to check it for null. So some applications would never provoke this potential crash in dvdnav_describe_title_chapters(). Thanks for the explanation and the patch review.
There are actually many other places in libdvdnav where cell_playback is used unchecked. I'm guessing the reason these don't provoke obvious frequent crashes is that a title that contains a pgc with cell_playback_offset == 0 is an invalid title and won't be accessible from the dvd menus. So that makes me think that an application that provokes the crash that is fixed by your patch is probably not strictly providing access based on the dvd menus and therefor may be providing other means to provoke similar crashes. My impression is that vlc uses it to generate the title lengths in the title menu. The crashes in dvdnav_describe_title_chapters() already happen while opening the DVD, though, which at least makes debugging easier.
Unfortunately DVDs that trigger crashes with unpatched libdvdnav versions usually also have a somewhat useless title menu, as it's unclear from the lengths what the actual main title is: http://www.fabiankeil.de/tmp/screenshot-vlc-title-menu-for-ghost-protocol.jp...
From a user's perspective it would be great to also have a menu that only shows titles that are reachable through the menu, but I assume that's not trivial.
True, it is non-trivial. But this is exactly the reason I created the dvdnav_dup() patch. This patch is in Erik's branch, but hasn't made it to the official repo yet. Creating duplicate dvdnav contexts allows you to do a recursive decent through menus doing virtual button pushes as you go. HandBrake uses this to hunt down a candidate for the main feature on the disc. It's not 100% accurate because my menu parser is a little lame still, but it works very well given it's lameness ;) -- John GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01 83F0 49F1 D7B2 60D4 D0F7
John Stebbins <stebbins@jetheaddev.com> wrote:
On 12/30/2012 06:57 AM, Fabian Keil wrote:
John Stebbins <stebbins@jetheaddev.com> wrote:
There are actually many other places in libdvdnav where cell_playback is used unchecked. I'm guessing the reason these don't provoke obvious frequent crashes is that a title that contains a pgc with cell_playback_offset == 0 is an invalid title and won't be accessible from the dvd menus. So that makes me think that an application that provokes the crash that is fixed by your patch is probably not strictly providing access based on the dvd menus and therefor may be providing other means to provoke similar crashes. My impression is that vlc uses it to generate the title lengths in the title menu. The crashes in dvdnav_describe_title_chapters() already happen while opening the DVD, though, which at least makes debugging easier.
Unfortunately DVDs that trigger crashes with unpatched libdvdnav versions usually also have a somewhat useless title menu, as it's unclear from the lengths what the actual main title is: http://www.fabiankeil.de/tmp/screenshot-vlc-title-menu-for-ghost-protocol.jp...
From a user's perspective it would be great to also have a menu that only shows titles that are reachable through the menu, but I assume that's not trivial.
True, it is non-trivial. But this is exactly the reason I created the dvdnav_dup() patch. This patch is in Erik's branch, but hasn't made it to the official repo yet. Creating duplicate dvdnav contexts allows you to do a recursive decent through menus doing virtual button pushes as you go. HandBrake uses this to hunt down a candidate for the main feature on the disc. It's not 100% accurate because my menu parser is a little lame still, but it works very well given it's lameness ;)
Sounds great. Wouldn't it make sense to have the "virtual button pushing" parts in libdvdnav as well? It seems to me that DVD players like vlc could profit from them and it might also be useful for dvdbackup whose main-feature detection is supposedly somewhat lacking (I haven't tested it, though). Fabian
On Sun, Dec 30, 2012 at 11:01 AM, John Stebbins <stebbins@jetheaddev.com> wrote:
Creating duplicate dvdnav contexts allows you to do a recursive decent through menus doing virtual button pushes as you go.
I've done this without deduping. Just involves a lot of calls to dvdnav_reset() and rewalking the path. On the other hand, I assume this approach solves the problem when one of those button pushes toggles the ``play track repeatedly forever'' feature some DVDs have? :-/ mrc
On 1/2/13, Mike Castle <dalgoda+mplayer@gmail.com> wrote:
On Sun, Dec 30, 2012 at 11:01 AM, John Stebbins <stebbins@jetheaddev.com> wrote:
Creating duplicate dvdnav contexts allows you to do a recursive decent through menus doing virtual button pushes as you go.
I've done this without deduping. Just involves a lot of calls to dvdnav_reset() and rewalking the path.
On the other hand, I assume this approach solves the problem when one of those button pushes toggles the ``play track repeatedly forever'' feature some DVDs have? :-/
I wonder if DVD's whose title loops have a related problem? Though I know nothing about it :) -r
On Sunday, 30 December 2012 at 15:57, Fabian Keil wrote: [...]
From d77bda56143b4c85b872ff7e0574070d024aff0b Mon Sep 17 00:00:00 2001 From: Fabian Keil <fk@fabiankeil.de> Date: Sat, 8 Dec 2012 21:52:08 +0100 Subject: [PATCH] Let dvdnav_describe_title_chapters() skip PGCs with missing cells
Fixes segfaults with a rip of Ghost Protocol.
Finally applied to SVN, thanks. 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"
participants (6)
-
Dominik 'Rathann' Mierzejewski -
Erik Hovland -
Fabian Keil -
John Stebbins -
Mike Castle -
Roger Pack