[MPlayer-dev-eng] [PATCH] CoreAVC [3/5] - Win32

[Reimar Döffinger]

Hello,
On Thu, Oct 05, 2006 at 08:45:53AM -0700, Alan Nisota wrote:
> On 10/5/06, Guillaume Poirier wrote:
> >However, the development you had to do on the wine wrapper to make it
> >possible to use CoreAVC may be worth being merged if you manage to
> >provide clean patches...
> I have done my best to seperate out the stuff that is only for CoreAVC
> with stuff that may be useful to the rest of mplayer.
> 
> Here is an updated patch with all the cosmetics removed

Hmm.. why is the signal handler needed? What irritates me is that it is
a handler for SIGSEGV... And I'm not sure if it's not a bit dangerous
security-wise to jump to locations on stack on a segfault (that might
also happen e.g. as side-effect of a malloc failing, though since it's
only for DLL entry functions it's not so likely to be critical).
Isn't it maybe possible to just patch away the offending instruction?

> +        if(name && strcasecmp(name, "oleaut32")==0 || !strcasecmp(name, "oleaut32.dll"))
> +           result=MODULE_HANDLE_oleaut32;

Since && has higher priority that || I don't think this does what you
meant.
Also since the surrounding code does you should use tabs here instead of
spaces.

> +static int WINAPI expVirtualProtect(void *ptr, int size,  int prot, int *oldprot)

Minor nit: Two spaces before "int prot".
Also it might be neccessary to actually implement this for systems that
use no-execute flags (can someone check?). Thus a TODO comment might be
a good idea.

> @@ -795,7 +807,8 @@
>         return (void*)WAIT_FAILED;
>      }
>      dbgprintf("WaitForSingleObject(0x%x, duration %d) =>\n",object, duration);
> -
> +    if ((int)ml == 1)
> +       return 0;

Why 0 and not (void*)WAIT_FAILED or WAIT_OBJECT_0 some other constant like all other
cases return?

>      // loop below was slightly fixed - its used just for checking if
>      // this object really exists in our list
>      if (!ml)
> @@ -901,6 +914,8 @@
>      /* 10l to QTX, if CreateMutex returns a real mutex,
>  * WaitForSingleObject
>         waits for ever, else it works ;) */
>      return mlist;
> +#else
> +    return 1;
>  #endif
>  }

Hmm.. is this related to the previous one? Does return 0 also work,
because that case is already handled in expWaitForSingleObject anyway...

> +static void * WINAPI expEncodePointer(void *value)
> +{
> +    return (void *)((long)value ^ getpid());
> +}
> +
> +static void * WINAPI expDecodePointer(void *value)
> +{
> +    return (void *)((long)value ^ getpid());
> +}

IMHO these should be NOPs (i.e. return the same value as they were
given). There is no "security" anyway when using binary codecs.

> +static int WINAPI expLoadStringW(long instance, long  id, void* buf, long size)
> +{
> +    return expLoadStringA(instance, id, buf, size);
> +}

does
#define expLoadStringW expLoadStringA
work as well? (same btw. for expEncodePointer and expDecodePointer since
they both use the same code).
Or actually just add
{"LoadStringW", -1, (void*)expLoadStringA},
instead of
FF(LoadStringW, -1)

> [expGetVersionExA changes]

not sure if they might not break something. Some codecs might try to use
more "enhanced" features we have not yet implemented when we increase
the reported version number. Can you investigate why they are necessary?

>  static int WINAPI expGetDesktopWindow()
>  {
> -    dbgprintf("GetDesktopWindow() => 0\n");
> -    return 0;
> +    dbgprintf("GetDesktopWindow() => 1\n");
> +    return 1;
>  }

What does a codec need the Desktop Window for?? And is this change
related to the WaitForSingleObject one or not?

> +#define UNDEFF(X, Y) \
> +    {#X, Y, -1},

Huh? Why -1 and then check for -1 in LookupExternalByName instead of
just using 0? Also what is the problem with the current code that would
just call do return add_stub(); for this case?

Greetings,
Reimar Döffinger