[DVDnav-discuss] [PATCH] handle duplicate language units and pgc's more gracefully

[John Stebbins]

On 12/30/2012 06:57 AM, Fabian Keil wrote:
> John Stebbins <stebbins at jetheaddev.com> wrote:
>
>> On 12/29/2012 10:55 AM, Fabian Keil wrote:
>>> John Stebbins <stebbins at jetheaddev.com> wrote:
>>>
>>>> On 12/28/2012 04:15 AM, Fabian Keil wrote:
>>>>> John Stebbins <stebbins at jetheaddev.com> wrote:
>>>>>
>>>>>> On 11/04/2012 06:33 PM, Fabian Keil wrote:
>>>>>>> John Stebbins <stebbins at 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()?
>> 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.jpg
>
> 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: OpenPGP digital signature
URL: <http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/attachments/20121230/ef508e47/attachment-0001.asc>