[PATCH] fix dvdnav multi-cell menus
As previously sent to mplayer-dev-eng, but with a separate version not including any white-space changes (both attached patches are otherwise identical): 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). The attached patch fixes both of these issues, and I have tested it extensively (always with my MPEG2 sync patch, of course) with both normal, and interactive DVDs, and including several examples of actual still frames. It seems to solve the problem and cause no other new ones that I can find. Of course, I could be wrong about all of this (but I don't think so). Thanks. -- Steaphan Greene <sgreene@cs.binghamton.edu> Lecturer, Computer Science, Binghamton University GPG public key: http://www.cs.binghamton.edu/~sgreene/gpg.key.txt
On Thursday, 18 August 2011 at 18:07, Steaphan Greene wrote:
As previously sent to mplayer-dev-eng, but with a separate version not including any white-space changes (both attached patches are otherwise identical):
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).
The attached patch fixes both of these issues, and I have tested it extensively (always with my MPEG2 sync patch, of course) with both normal, and interactive DVDs, and including several examples of actual still frames. It seems to solve the problem and cause no other new ones that I can find.
Of course, I could be wrong about all of this (but I don't think so).
Could someone test/review this? 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 10/06/2011 10:01 AM, Dominik 'Rathann' Mierzejewski wrote:
On Thursday, 18 August 2011 at 18:07, Steaphan Greene wrote:
As previously sent to mplayer-dev-eng, but with a separate version not including any white-space changes (both attached patches are otherwise identical):
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).
The attached patch fixes both of these issues, and I have tested it extensively (always with my MPEG2 sync patch, of course) with both normal, and interactive DVDs, and including several examples of actual still frames. It seems to solve the problem and cause no other new ones that I can find.
Of course, I could be wrong about all of this (but I don't think so).
Could someone test/review this?
Well, I can confirm that this bug still exists in the current SVN, and this patch still works to fix it. I've also been using this modified version of dvdnav & mplayer for a while now, and have seen no bugs in dvd playback that weren't there before applying this patch. Of course, I am the original author of this patch, so if anyone else can back me up on this (at least, that it doesn't cause any problems for anyone else), I'm sure that would go a lot farther. I have, for example, only tested this on NTSC DVDs (which are all I have). One thing that does concern me about this patch is that mythtv seems to use the same dvdnav code, but does NOT show this same bug. Does this indicate that it is mplayer that is not properly handing this case? If so, I will re-target my efforts back to mplayer. Otherwise, if there is any way this could get into the upcoming release version, that would be nice. But, if it's too late at this point, I understand. I would give more details about which DVDs exactly show this bug, but, unfortunately, I live in the U.S., where I apparently don't have the right to view my own movies. So, I don't want to draw the attention of any of the publishers of the movies I am using for debugging or testing all this. Thanks. P.S. - Note my change of e-mail address from my previous postings. -- Steaphan Greene <steaphan@gmail.com> Lecturer, Computer Science, Binghamton University GPG public key: http://www.cs.binghamton.edu/~sgreene/gpg.key.txt
As previously sent to mplayer-dev-eng, but with a separate version not including any white-space changes (both attached patches are otherwise identical):
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? 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. E -- Erik Hovland erik@hovland.org http://hovland.org/
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@binghamton.edu> Lecturer, Computer Science, Binghamton University GPG public key: http://www.cs.binghamton.edu/~sgreene/gpg.key.txt
Does the first change look correct to you, though (advancing to the next cell in this multi-cell menu case)?
It looks fine to me. How does it behave when it is the only patch to the current release w/ other DVDs?
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?
My mistake. You did submit a reformatting patch. Thanks E -- Erik Hovland erik@hovland.org http://hovland.org/
Does the first change look correct to you, though (advancing to the next cell in this multi-cell menu case)?
It looks fine to me. How does it behave when it is the only patch to the current release w/ other DVDs? I did check it with the version I first wrote this for using many different DVDs. I've also been using it since with various DVDs with no
On 10/19/2011 01:17 PM, Erik Hovland wrote: problems, but haven't updated from SVN in a few weeks (been busy with a probable job change, and an upcoming move across the country). I believe I will look this patch over again in the context that some of the fault may lie in mplayer, and not in dvdnav. So, for now, I'd recommend not applying it. I will follow up in a couple of weeks, when I get time to look at this closely again. Thanks. -- Steaphan Greene<steaphan@gmail.com> Lecturer, Computer Science, Binghamton University
participants (4)
-
Dominik 'Rathann' Mierzejewski -
Erik Hovland -
Steaphan Greene -
Steaphan Greene