[MPlayer-dev-eng] [PATCH] Replaced deprecated QuickDraw calls in vo_quartz

[Guillaume POIRIER]

Hello,

On Thu, Nov 13, 2008 at 1:18 AM, Diego Biurrun <diego at biurrun.de> wrote:
> On Wed, Oct 29, 2008 at 07:26:58PM +0100, Gregor Riepl wrote:
>>
>> here's the updated patch.
>
> Here is a preliminary review...
>
>> --- configure (revision 27848)
>> +++ configure (working copy)
>> @@ -3521,14 +3521,28 @@
>>      else
>>       _macosx=no
>>       _noaomodules="macosx $_noaomodules"
>> -     _novomodules="quartz $_novomodules"
>> +     _novomodules="macosx quartz $_novomodules"
>
> That is a separate bug fix, committed.
>
>>  if test "$_macosx" = yes ; then
>>    cat > $TMPC <<EOF
>> +#include <CoreAudio/CoreAudio.h>
>> +int main(void) { return 0; }
>> +EOF
>
> Just the presence of the header is enough?  Don't you need to call a
> function or something?

Well, for the moment, yes, since the goal of this hunk is just to
detect that CoreAudio is supported by the underlying OS, and it has
been so since OSX 10.0. SVN version of the configure tests both Quartz
and CoreAudio support, when Quartz in restricted to 32-bits, and
CoreAudio both 32- and 64-bits compatible.


>> +  if cc_check -framework CoreAudio; then
>> +    _ld_extra="$_ld_extra -framework CoreAudio -framework AudioUnit -framework AudioToolbox"
>> +    _coreaudio=yes
>> +    _def_coreaudio='#define CONFIG_COREAUDIO 1'
>> +    _aosrc="$_aosrc ao_macosx.c"
>> +    _aomodules="macosx $_aomodules"
>> +  else
>> +    _coreaudio=no
>> +    _def_coreaudio='#undef CONFIG_COREAUDIO'
>> +    _noaomodules="macosx $_noaomodules"
>> +  fi
>> +  cat > $TMPC <<EOF
>>  #include <Carbon/Carbon.h>
>>  #include <QuickTime/QuickTime.h>
>> -#include <CoreAudio/CoreAudio.h>
>>  int main(void) {
>
> It would be good if this ao_macosx check could be a separate patch.

Ok, but other then that, it's OK? I can commit hunks seperately.


>> @@ -3536,34 +3550,31 @@
>> -cat > $TMPC <<EOF
>> -#include <Carbon/Carbon.h>
>> +  cat > $TMPC <<EOF
>> +#include <Cocoa/Cocoa.h>
>>  #include <QuartzCore/CoreVideo.h>
>> +#include <OpenGL/OpenGL.h>
>>  int main(void) { return 0; }
>>  EOF
>> -     if cc_check -framework Carbon -framework QuartzCore -framework OpenGL; then
>> +     _tmp_CFLAGS="$CFLAGS"
>> +     CFLAGS="$CFLAGS -x objective-c"
>
> Does the check fail without this?

Without what? I'm sorry but I'm not sure I get what you refer to by "this".


>  And why do you use a temporary
> variable..
>
>
>> +     if cc_check -framework Cocoa -framework QuartzCore -framework OpenGL -framework Carbon; then
>
> ..when you can just add the flag to the compiler call here?

Right, that's easy to fix...


>> --- osdep/macosx_finder_args.c        (revision 27848)
>> +++ osdep/macosx_finder_args.c        (working copy)
>> @@ -85,7 +85,7 @@
>>
>> -     QuitApplicationEventLoop();
>> +     //QuitApplicationEventLoop();
>
> Separate patch?  Also, you should add a comment that explains why this
> line is commented out or directly remove it.

Here is what Gregor wrote about this:
"I also removed QuitApplicationEventLoop from
osdep/macosx_finder_args.c, as it's gone for 64bit and the docs say
it's normally not necessary.
for some reason, there's a regression here, though: the quit menu item
doesn't work (other menu entries and cmd-q do), but it doesn't work
with QuitApplicationEventLoop either, so that looks unrelated."

I guess it should go in a separate patch.

Guillaume
-- 
One should not give up hope on imbeciles. With a little training, you
can make them into soldiers.
 -- Pierre Desproges