[DVDnav-discuss] [PATCH] fix dvdnav multi-cell menus

[Steaphan Greene]

On 10/10/2011 02:24 PM, Erik Hovland wrote:
>> Many DVDs with long menus (interactive DVDs or DVD "games" often
>> treat the entire "movie" as a menu) don't work correctly in
>> mplayer's dvdnav. As soon as they change to a new cell, the DVD
>> stops working - It just drops the audio and corrupts the video by,
>> I believe, playing the previous video chunk over and over.  The
>> navigation controls continue to work, though.
>>
>> The problem seems to be that when a new cell is encountered, the
>> dvdnav code indicates it should wait for sync with the
>> application. However, the code does not actually advance to the
>> next cell (it assumes this case is hit only with still frames, even
>> though the condition also catches the case of cell-changes within
>> menus).
>>
>> Also, once the code to advance to the next block is enabled, by
>> fixing that condition, it still fails in the same way.  This seems
>> to be because the "wait_sync = 1;" is a terminal condition here.
>> Again, it assumes this is for still frames only and is waiting for
>> the next still frame to be selected (which sets sync_wait back to
>> 0).
>
> This part of the patch troubles me. It makes me think that there are
> two separate patches here.
>
> By changing the value of sync_wait in the sync_wait check you are
> effectively saying that the sync_wait has timed out after one
> execution of the function dvdnav_get_next_cache_block(). And I see
> no check to see that anything is actually synchronized. In fact I
> expect that the the "application" will clear sync_wait, but it
> doesn't. Instead sync_wait will be set again in the same function.
> What is the effect of making just this change?

I made this change under the assumption that mplayer was already using
this library correctly in this case.  If I am understanding you here,
that may have been incorrect.  I'm thinking now that the first problem
is in dvdnav, but the second may be in mplayer (not handling wait_sync
and clearing it itself somehow).  I will look further into this second
part, perhaps by seeing how mythtv handles this.

Does the first change look correct to you, though (advancing to the next
cell in this multi-cell menu case)?


> You are welcome to attach a patch that does the reindenting of the
> if clause as a reformatting patch. Keeping them separate highlights
> the fix you are trying to make. Then reformatting the code makes it
> a little more readable.

That is what I did, no?  Or, do you mean the reformatting patch should
not also include the actual change?

-- 
Steaphan Greene <sgreene at binghamton.edu>
Lecturer, Computer Science, Binghamton University
GPG public key: http://www.cs.binghamton.edu/~sgreene/gpg.key.txt