Re: [PATCH (try 3) 2/2] dpvoice: Turn GetCompressionTypes into a semi-stub.

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2015-01-16 um 08:44 schrieb Alex Henrie:
> Star Trek Armada II needs GetCompressionTypes to return at least 
> one value, see https://bugs.winehq.org/show_bug.cgi?id=29238
> Is the game happy if you return DV_OK and set *pdwNumElements = 0?
> MS-PCM is guaranteed to be present on Windows XP, and it's already 
> implemented in Wine, so advertising this codec shouldn't cause any 
> trouble.
> Even though we have the codec we probably don't have the dpvoice
> infrastructure to hook up a dpvoice client to the codec, right? That's
> not necessarily a reason to reject the semi-stub though.
> +extern HRESULT DPVOICE_GetCompressionTypes(DVCOMPRESSIONINFO 
> *pData, DWORD *pdwDataSize, DWORD *pdwNumElements, DWORD dwFlags) 
> DECLSPEC_HIDDEN;
> Please avoid hungarian notation names (dwThisIsAdWord, *pThisIsAPointer).
> +        string_loc = (LPWSTR)((char*)pData + sizeof(pcm_type)); +
>  memcpy(pData, &pcm_type, sizeof(pcm_type)); + memcpy(string_loc, 
> pcm_name, sizeof(pcm_name)); + pData->lpszName = string_loc;
> I think there should be a nicer way to do this. I didn't test this with
> the compiler, but I guess you can do something like this:

struct
{
DVCOMPRESSIONINFO info;
const WCHAR name[];
}
codec =
{
{80, {0x8de12fd4,0x7cb3,0x48ce
{0xa7,0xe8,0x9c,0x47,0xa2,0x2e,0x8a,0xc5}}, &codec.name,
NULL, 0, 64000}},
{'M','S','-','P','C','M',' ','6','4',' ','k','b','i','t','/','s',0};
};

...

memcpy(pData, codec, sizeof(codec));

I don't know how well the variable-size array at the end of the struct
works. It also won't easily work for more than one element.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJUuUN7AAoJEN0/YqbEcdMwFwcP/iSIxX72VcW9TSqbWHnq1Pi6
7Yv9zcL3h/3Mx0rERFiaT15Tmz7+YNyKfngR6kZwM7ms5usqePHCW1y1dT7ImCfM
gSAgd5WBTKuk5qpr0k2zLgl4O3TVJn4zTP8wKNa+lwGDJgY7d3ThcgDd/LI6kkiU
KI2Uv+jyh00Co2G6egmhlZDfwWo3dI/b+7fndC1jnYCcyIIxjLywdiQ80SCi3IgB
sktBaQW7mvzzQDtMKgwR/BNUSze6AQeWQ5+xmWivIm4jsPGETmr3r0VnRtxxMduA
LzXJJewnGojJoaQ1dsjsXJJ+S+3rKCd5cKrbnlyFXJ2pqHGrBdkQwbFTWse+8Tsf
+Au00otO85InuwenrvKutN92c4pP8hfyxAQbKJUjVCnD5ppMDXkRrx0xKfAy7SWB
FpQuO6KLeWMotDYcC21kwY3jGLAxKOWGh02p7S5AUIlnZ0m1kJ3PK1DxhomRHYax
4pgQx0jb/MITw1/L5hJ8KlNcucLJxC2FBGpgRC32LAUHMVcNurs6bHQbgbl9UKUS
zH4gOA7PzNTGG3G3To/IqmayhmzNMLk/b0XusIGKF1Ao3uyU3Yw+5RuzPlGaKOkT
cAhRIl5Ujbcjt8JKgtdq4x1VeGMutHlePhPuUpgZX98n+/Bgk9uZ5rScVLW1ELGW
XQ6zfKUWW2qirySBqy+A
=StHs
-----END PGP SIGNATURE-----

[PATCH (try 3) 2/2] dpvoice: Turn GetCompressionTypes into a semi-stub.

[Alex Henrie]

2015-01-16 9:59 GMT-07:00 Stefan Dösinger <stefandoesinger(a)gmail.com>:
> Am 2015-01-16 um 08:44 schrieb Alex Henrie:
> > Star Trek Armada II needs GetCompressionTypes to return at least
> > one value, see https://bugs.winehq.org/show_bug.cgi?id=29238
> > Is the game happy if you return DV_OK and set *pdwNumElements = 0?

No. *pdwNumElements has to be at least 1 or the game crashes.
> > MS-PCM is guaranteed to be present on Windows XP, and it's already
> > implemented in Wine, so advertising this codec shouldn't cause any
> > trouble.
> > Even though we have the codec we probably don't have the dpvoice
> > infrastructure to hook up a dpvoice client to the codec, right? That's
> > not necessarily a reason to reject the semi-stub though.

Right. Very little of dpvoice is implemented right now, but this
semi-stub gives us a starting point for building the rest.
> > +extern HRESULT DPVOICE_GetCompressionTypes(DVCOMPRESSIONINFO
> > *pData, DWORD *pdwDataSize, DWORD *pdwNumElements, DWORD dwFlags)
> > DECLSPEC_HIDDEN;
> > Please avoid hungarian notation names (dwThisIsAdWord, *pThisIsAPointer).

I copied and pasted the variable names from
dpvclient_GetCompressionTypes and dpvserver_GetCompressionTypes, so I
am merely matching the variable conventions already present in this
DLL.
> > +        string_loc = (LPWSTR)((char*)pData + sizeof(pcm_type)); +
> >  memcpy(pData, &pcm_type, sizeof(pcm_type)); + memcpy(string_loc,
> > pcm_name, sizeof(pcm_name)); + pData->lpszName = string_loc;
> > I think there should be a nicer way to do this. I didn't test this with
> > the compiler, but I guess you can do something like this:
>
> struct
> {
> DVCOMPRESSIONINFO info;
> const WCHAR name[];
> }
> codec =
> {
> {80, {0x8de12fd4,0x7cb3,0x48ce
> {0xa7,0xe8,0x9c,0x47,0xa2,0x2e,0x8a,0xc5}}, &codec.name,
> NULL, 0, 64000}},
> {'M','S','-','P','C','M',' ','6','4',' ','k','b','i','t','/','s',0};
> };

There are a couple of problems with this suggestion:

1. C only allows one flexible array per struct. We would have to
switch to fixed, manually hardcoded array sizes in order to add more
codecs to the list. Also, ANSI C does not allow flexible arrays in
structs at all, so this syntax could be a problem for some compilers.

2. In Windows, the codec names are localized and stored in resource
files in other DLLs. For PCM, the codec name is obtained by combining
a resource in msacm32.dll with a resource in dpvacm.dll. These
resources are translated into German and possibly other languages. A
complete implementation of GetCompressionTypes would not have static
variables for codec names at all because it would use LoadStringW to
get the localized codec names, and it would have to keep track of the
string length of each codec name in the current language. Thus, using
a struct would be out of the question. See
https://github.com/wine-compholio/wine-staging/issues/251#issuecomment-69478...
for more information.

So long story short, yes a struct might be a little easier as long as
we only have one codec, but sooner or later we'd have to get rid of
the struct and switch to something else.

-Alex

[PATCH (try 3) 2/2] dpvoice: Turn GetCompressionTypes into a semi-stub.

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2015-01-16 um 20:15 schrieb Alex Henrie:
> No. *pdwNumElements has to be at least 1 or the game crashes.
> Ok
> Right. Very little of dpvoice is implemented right now, but this 
> semi-stub gives us a starting point for building the rest.
> That's fine with me, but you'll have to convince Alexandre that this
> is a step in the right direction. To phrase it another way: How much
> effort would a proper enumeration take? Do you have plans to implement
> bigger parts of dpvoice?

Does avoiding this crash improve usability of the game? For example,
does single player work without installing native dplay?
> I copied and pasted the variable names from 
> dpvclient_GetCompressionTypes and dpvserver_GetCompressionTypes, so
> I am merely matching the variable conventions already present in
> this DLL.
> Yeah, I know they exist in many places (and probably originate from
> the headers). Hungarian notation is something we want to get rid of
> from all of Wine, so deviating from existing code in that regard is fine.
> > struct { DVCOMPRESSIONINFO info; const WCHAR name[]; } codec = { 
> > {80, {0x8de12fd4,0x7cb3,0x48ce 
> > {0xa7,0xe8,0x9c,0x47,0xa2,0x2e,0x8a,0xc5}}, &codec.name, NULL, 0,
> > 64000}}, {'M','S','-','P','C','M',' ','6','4','
> > ','k','b','i','t','/','s',0}; };
>
> There are a couple of problems with this suggestion:
>
> 1. C only allows one flexible array per struct. We would have to 
> switch to fixed, manually hardcoded array sizes in order to add
> more codecs to the list. Also, ANSI C does not allow flexible
> arrays in structs at all, so this syntax could be a problem for
> some compilers.
>
> 2. In Windows, the codec names are localized and stored in
> resource files in other DLLs.
> Overall it is a bit related to the first problem. I can live with
> either way if we decide to keep the semi-stub.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJUukPyAAoJEN0/YqbEcdMwsJIQAIIJqcYvKA66PIIkOCkvz/ov
bKTI2RAM1CDeW1temyAsS8vkZLAimNNejC7lK7R/GYa+W4UO3XwO7vSBBf5Xtmt4
ZgqkbGJNnAGUzQknMDeDQ9rc8uloSD5c/yZKY9qqC2eFD/5NozFXb6DqlRI8bBOh
3iT1Ri4pgpfJT2GL25GwOLC/5fxqhhoxvDfIiWahYes/sizQyEr2aoMu9REPQr8P
C4/ozd11hKIbR2IXwKOHx3AWV3mU9nJiwqNRimRubq7HfZZrciUJlVyeWyBhxKRC
PxKkJ/YbkZoSEbE+i+jK6HXPFPtdnl26lOL7sMJdgNFgGZxFEZGpQwjfvEGnX0Zy
CDcI03fd5asmI9+hhIVPlf3HxijhAPPsmO6/Yyt/gGboTK3UTQOUONLSMVzbbYWO
zTkx1i94/StYsOs+p/il0c2E6k9KbXyqG61lt7xQ38NmngiHM11DMN96fLN8UL2k
vHIPZudE8dCyvkYTcnWiywQjlEPGQgEFB4+gJiTiDCFVxHaZid1ko92SOgL95Bal
sp+tsoQ4mPGkue2aLnc30O40pXA+Kk1Sn6sLR8FldHlFbNbOYobywxddpF+O9gU9
77YUDoJ/2Qwf4d8QuefGHiNLcHD1q8t/1d0GlQSXgf4CjsBpmk1eLdL39W1GBMqq
c6L7mM6L2ZCFaiKYrNC1
=kMZU
-----END PGP SIGNATURE-----

[PATCH (try 3) 2/2] dpvoice: Turn GetCompressionTypes into a semi-stub.

[Alex Henrie]

2015-01-17 4:13 GMT-07:00 Stefan Dösinger <stefandoesinger(a)gmail.com>:
> > Right. Very little of dpvoice is implemented right now, but this
> > semi-stub gives us a starting point for building the rest.
> > That's fine with me, but you'll have to convince Alexandre that this
> > is a step in the right direction. To phrase it another way: How much
> > effort would a proper enumeration take?

A proper enumeration would take a lot of effort because it changes
depending on whether certain DLLs are present, and not all of those
DLLs are present in Wine. Also, the strings would have to be
localized.
> Do you have plans to implement bigger parts of dpvoice?

Most of dpvoice is patent-protected in the United States:

https://www.google.com/patents/US7720908
https://www.google.com/patents/US7730206

GetCompressionTypes is one of the few pieces that we can legally implement.

Fortunately, practically nothing actually uses dpvoice. Star Trek
Armada II doesn't even use dpvoice, it's just a dependency of its game
engine.
> Does avoiding this crash improve usability of the game? For example,
> does single player work without installing native dplay?

Single player works fine. Multiplayer crashes without warning, leaving
the X server at 800x600 resolution.
> > I copied and pasted the variable names from
> > dpvclient_GetCompressionTypes and dpvserver_GetCompressionTypes, so
> > I am merely matching the variable conventions already present in
> > this DLL.
> > Yeah, I know they exist in many places (and probably originate from
> > the headers). Hungarian notation is something we want to get rid of
> > from all of Wine, so deviating from existing code in that regard is fine.

Getting rid of the Hungarian notation in DPVOICE_GetCompressionTypes
would make it look odd next to the other 3 functions declared in
dvoice_private.h. Maybe after these two patches are committed, we
could work on another patch to uniformly eliminate the Hungarian
notation throughout dpvoice.

-Alex

[PATCH (try 3) 2/2] dpvoice: Turn GetCompressionTypes into a semi-stub.

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2015-01-17 um 19:23 schrieb Alex Henrie:
> A proper enumeration would take a lot of effort because it changes 
> depending on whether certain DLLs are present, and not all of
> those DLLs are present in Wine. Also, the strings would have to be 
> localized.
> This would probably work by the DLLs registering themselves in the
> registry and dpvoice enumerating the registry keys. The codecs
> probably already register themselves.
> Most of dpvoice is patent-protected in the United States:
>
> https://www.google.com/patents/US7720908 
> https://www.google.com/patents/US7730206
>
> GetCompressionTypes is one of the few pieces that we can legally
> implement.
>
> Fortunately, practically nothing actually uses dpvoice. Star Trek 
> Armada II doesn't even use dpvoice, it's just a dependency of its
> game engine.
> I think under these circumstances adding a semi-stub like yours that
> makes a game work better is a reasonable thing to do.
> Getting rid of the Hungarian notation in
> DPVOICE_GetCompressionTypes would make it look odd next to the
> other 3 functions declared in dvoice_private.h. Maybe after these
> two patches are committed, we could work on another patch to
> uniformly eliminate the Hungarian notation throughout dpvoice.
> Generally style-only changes are discouraged and the right thing to do
> is to change the style when you're changing the code for other
> reasons. I personally disagree with that rule since it makes the same
> style issues pop up again and again and again.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJUvA8xAAoJEN0/YqbEcdMwu1UP/0GRPvR8VEanomFu0jJujT86
o2MRJldHb/dfS4N7ZyJSxAPi000AlsGkI+ocZn/+kavmgKbHhHbsWkJW6jygJg1K
iSWyY/ziVTRM4Bphij85wzlJl+tYzquM8q4wde1OIA6UHnc/CYTwsAtECx5MuRIK
OBA//e9xvm42G/LyyqM3EIZcSwduJcAULoDWcaPxnlhhN3DfLesGOs27h69C2Ypl
OAcD+qnWmWFb2o5eyZ0dK/1RHJi5MgTRBDhATCq88nDaDmuL1dP4WNe85+wkX34/
4Z2+4gSTlujdB61VYfOlixO0XtIgHGQNs4H+qt4q8aPr0XeADT+fSYV/b6XKU+oI
LITTqLiDaWmo4eqhA71XzhN3+9+zn67rPpR23pwlOiHWWiM9VtVsJ+yR6Da3qWbB
mSpRUFb9Slq/4ciYWqWXKPfsp/rG51hrqWXxpn4L5eDfHW0XPSAjzj93JTH1TEmh
+vyjp1T2ik+92m4TjWASpGaxhrGRYK0lG9LZEma01PUTMbFk6Tz3lS+ILq24wJ6o
HPEJBxWQ2PzlljRVBrgMZ/8mC8lkeKjNStKG3KlX1Ufu894cx9pdt4OeBSIACYLG
uoCHBz213nztSS4pYpoxW+UrBb5QEp/ekXVr8gSpoOTVUhmf3WnVmwzaGRhb4j7l
NS0OFRsVQUL5E7TtLyfF
=JwKc
-----END PGP SIGNATURE-----