[MPlayer-dev-eng] New patch for play DVD's with dvdnav://

[Alexander Roalter]

On 02/16/2010 10:14 PM, jaime.gemelo at free.fr wrote:
> Hi everybody!
> 
> Carl Eugen writes:
>>> <jaime.gemelo <at> free.fr> writes:
>>>
>>>> Here is a new version of the "04_dvdnav.patch"
>>>          break;
>>>        }
>>>        case DVDNAV_SPU_STREAM_CHANGE: {
>>>          priv->state |= NAV_FLAG_STREAM_CHANGE;
>>> +        priv->state |= NAV_FLAG_STREAM_CHANGE;
>>>
>>> Duplicated line?
>>>
>>> [...]
> 
> DON'T DELETE THIS LINE! DVDs had problems to change the spu stream.
> That's why i do this.
> Now I meet no problems with commercial DVDs that i have (Star Wars series, Matrix series,
> The Lord of the Rings series, Gun Frontier, Cowboy Bebop, Final Fantasy Advent Children,
> etc.).

If executing setting a bit twice does something else as setting it once
(other than for example it being a memory mapped Write-to-clear or
something similar register), there is a race condition, which should not
be fixed this way, but in a cleaner way. If setting it twice has an
effect and setting it only once doesn't, then the timing is somewhat off
and setting the state should then be synchronized with the point where
priv->state is read... although I was not aware that multi-threading is
active here.

No matter if it solves problems with some DVDs you have, as long as
there's no explaination WHY this works with this line doubled, I think
this is not correct (this doesn't mean that it is wrong, though).

> 
> Carl Eugen wrote (source:
> http://lists-archives.org/mplayer-dev-eng/28391-dvdnav-support-improve-change-audio-and-subtitle-by-dvd-menu-and-other-things.html):
>>> Just two cosmetic comments:
>>> There is one occurrence of trailing white space in the patch - please remove it.
>>> And I believe
>>>   if (A)
>>>     if (B)
>>>       C();
>>> should be written as
>>>   if (A && B)
>>>     C();
>>>
>>> Carl Eugen
> It isn't always true. I have meet some problems in some conditions.
If enclosed in appropriate brackets/parenthesis, it is always true,
otherwise there are some side effects or misplaced/misused precedence
rules (or the compiler is broken), because both terms will be evaluated
exactly the same: on an && condition there's always short circuit
evaluation in the left-to-right order, which means if A is not met, B is
never tested, which is exactly the same as two nested if clauses.


>>>> Here is a patch that  resolves some things ;) :
>>>
>>> One patch on this list is expected to fix/implement exactly one thing:
>>> Some things => some patches
>>>
>>> [...]
>>>
>>>> To apply this patch, launch this command: patch -p1 -i "name of the patch"
>>>
>>> While everybody here is expected to know how to use patch, please prepare your
>>> patches by using "svn diff" - we believe everything else is unreadable.
> 
> You're not God. And all people (not only developpers) can visit this page in the aim to
> find solutions of his problems.
> Thus, for these persons, I decided to explain how they can apply a patch.
> And I use this command to create my patch:
> diff -abur directory_files_nochange directory_files_modified > mypatch.patch
> 
> In a friendly manner!

If you intend your patch to be found by those who need them and apply it
by themselves on the source, then of course you can use your own diff
format and specify how to apply it, but -- keep in mind I'm not a
maintainer for it -- to have it accepted in the code -- without the need
for patching on behalf of the users I think one should follow the rules
set up by the maintainer, who in this case is God. Therefore also the
whitespace/tab-issue should be observed.



Just my 2 ct,
Cheers,
Alex