[MPlayer-dev-eng] PATCH [8/12] CoreAVC support (Take 3)

[Reimar Döffinger]

Hello,
On Tue, Feb 27, 2007 at 12:44:25PM -0800, Alan Nisota wrote:
> >008newfunc.patch: adds several support for several new functions to
> >win32.c  Note the 'UNDEFF' macro is used to prevent certain functions
> >from being stubbed.  If they are, stubbed, the codec doesn't work properly.

> Index: loader/win32.c
> ===================================================================
> --- loader/win32.c.orig	2007-02-15 11:41:42.000000000 -0800
> +++ loader/win32.c	2007-02-15 11:44:20.000000000 -0800
> @@ -579,6 +579,9 @@
>  #define	MODULE_HANDLE_msvcrt	((HMODULE)0x126)
>  #define	MODULE_HANDLE_ole32	((HMODULE)0x127)
>  #define	MODULE_HANDLE_winmm	((HMODULE)0x128)
> +#define	MODULE_HANDLE_version	((HMODULE)0x129)
> +#define	MODULE_HANDLE_gdi32	((HMODULE)0x130)
> +#define	MODULE_HANDLE_oleaut32	((HMODULE)0x131)
>  
>  static HMODULE WINAPI expGetModuleHandleA(const char* name)
>  {
> @@ -605,11 +608,20 @@
>  	if(name && strcasecmp(name, "user32")==0)
>  	    result=MODULE_HANDLE_user32;
>  #endif
> +        if(name && strcasecmp(name, "oleaut32")==0 || !strcasecmp(name, "oleaut32.dll"))
> +           result=MODULE_HANDLE_oleaut32;

Please keep things consistent. Here, this means using tab for first
level of indentation, and using ==0 for the strcasecmp result comparison
(at least don't mix two different ways in one line).

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

two spaces after "int size,"

> @@ -1741,6 +1763,11 @@
>      return result;
>  }
>  
> +static int WINAPI expLoadStringW(long instance, long  id, void* buf, long size)
> +{
> +    return expLoadStringA(instance, id, buf, size);
> +}
> +

Can't you just instead add
{"LoadStringW", -1, (void*)expLoadStringA},
to the exports?
Maybe even introduce a nice macro, e.g.
REMAP(X, Y, Z) {#X, Y, (void*)exp##Z},
to be also used for similar cases like expdelete.

> @@ -1807,6 +1834,27 @@
>  	      "  Platform Id: VER_PLATFORM_WIN32_NT\n Version string: 'Service Pack 3'\n");
>      return 1;
>  }
> +
> +static long WINAPI expGetVersionExW(OSVERSIONINFOW* c)
> +{
> +    dbgprintf("GetVersionExW(0x%x) => 1\n");
> +    c->dwOSVersionInfoSize=sizeof(*c);
> +    c->dwMajorVersion=5;
> +    c->dwMinorVersion=0;
> +    c->dwBuildNumber=0x5000457;
> +#if 1
> +    // leave it here for testing win9x-only codecs
> +    c->dwPlatformId=VER_PLATFORM_WIN32_WINDOWS;
> +    strcpy(c->szCSDVersion, " B");
> +#else
> +    c->dwPlatformId=VER_PLATFORM_WIN32_NT; // let's not make DLL assume that it can read CR* registers
> +    strcpy(c->szCSDVersion, "Service Pack 3");
> +#endif
> +    dbgprintf("  Major version: 5\n  Minor version: 0\n  Build number: 0x5000457\n"
> +	      "  Platform Id: VER_PLATFORM_WIN32_NT\n Version string: 'Service Pack 3'\n");
> +    return 1;

If you hardcode all values, that dbgprintf seems quite pointless to
me...

> @@ -2449,6 +2497,12 @@
>  	return MODULE_HANDLE_ole32;
>      if (strcasecmp(name, "winmm.dll") == 0 || strcasecmp(name, "winmm") == 0)
>  	return MODULE_HANDLE_winmm;
> +    if (strcasecmp(name, "version.dll") == 0 || strcasecmp(name, "version") == 0)
> +        return MODULE_HANDLE_version;
> +    if (strcasecmp(name, "gdi32.dll") == 0 || strcasecmp(name, "gdi32") == 0)
> +        return MODULE_HANDLE_gdi32;
> +    if (strcasecmp(name, "oleaut32.dll") == 0 || strcasecmp(name, "oleaut32") == 0)
> +        return MODULE_HANDLE_oleaut32;

Code above uses tab not spaces before the return. I don't care much
though.

> -    dbgprintf("GetProcAddress(0x%x, '%s') => 0x%x\n", mod, name, result);
> +    if((unsigned int)name > 0xffff)
> +      dbgprintf("GetProcAddress(0x%x, '%s') => 0x%x\n", mod, name, result);
> +    else
> +      dbgprintf("GetProcAddress(0x%x, '%d') => 0x%x\n", mod, (int)name, result);

Minor cosmetics, you only added two spaces before one of the dbgprintfs

>      return result;
>  }
>  
> @@ -4856,6 +4919,16 @@
>    return name;
>  }
>  
> +static WIN_BOOL WINAPI expSetRect(LPRECT rect, int left, int top,
> +                                  int right, int bottom) {
> +  rect->left   = left;
> +  rect->top    = top;
> +  rect->right  = right;
> +  rect->bottom = bottom;
> +  dbgprintf("SetRect(0x%x (%d,%d)-(%d,%d) => 0\n", rect, left, top, right, bottom);
> +  return 0;

My documentation says: "Zero indicates failure.". So shouldn't you
return 1??

> +    UNDEFF(FlsAlloc, -1)
> +    UNDEFF(FlsGetValue, -1)
> +    UNDEFF(FlsSetValue, -1)
> +    UNDEFF(FlsFree, -1)

[...]
> @@ -5485,10 +5570,9 @@
>  	printf("ERROR: library=0\n");
>  	return (void*)ext_unknown;
>      }
> -    if(name==0)
> +    if((unsigned int)name<=0xffff)
>      {
> -	printf("ERROR: name=0\n");
> -	return (void*)ext_unknown;
> +        return LookupExternal(library, (int)name);
>      }
>      dbgprintf("External func %s:%s\n", library, name);
>      for(i=0; i<sizeof(libraries)/sizeof(struct libs); i++)
> @@ -5499,6 +5583,8 @@
>  	{
>  	    if(strcmp(name, libraries[i].exps[j].name))
>  		continue;
> +            if((unsigned int)(libraries[i].exps[j].func) == -1)
> +               return NULL; //undefined func

Like others said, this UNDEFF and <=0xffff stuff should be in a separate
patch each.

Greetings,
Reimar Döffinger