[1/2] mmsystem.dll16: fix MCI_STATUS mapping for digitalvideo
Damjan, @@ -407,6 +441,20 @@ static MMSYSTEM_MapType MCI_UnMapMsg16To32W(WORD wMsg, DWORD dwFlags, DWORD_PTR + mdsp16->dwCallback = mdsp32w->dwCallback; + mdsp16->dwItem = mdsp32w->dwItem; + mdsp16->dwTrack = mdsp32w->dwTrack; This is superfluous. dwReturn is the only documented output. This was different with your former MCI_WHERE patch, where a RECT is output. I'd try hard not to turn one message call (Status) into 2 (GetDevCaps + Status). I've seen logs of apps that continuously poll MCI_STATUS_POSITION. Instead, I'd continue the path lead by winmm/mci.c:MCI_MapMsgAtoW: Allocate the largest structure and fill it depending on dwFlags, e.g.: case MCI_OPEN: { /* MCI_ANIM_OPEN_PARMS is the largest known MCI_OPEN_PARMS * structure, larger than MCI_WAVE_OPEN_PARMS */ BTW, I've a note that MCI_INFO needs similar treatment, but never wrote the patch... Select the right set of flags to fill the slots, otherwise fallback to defaults like: /* We don't know how many DWORD follow, as * the structure depends on the device. */ if (HIWORD(dwParam1)) memcpy(&mci_openW->dwStyle, &mci_openA->dwStyle, sizeof(MCI_ANIM_OPEN_PARMSW) - sizeof(MCI_OPEN_PARMSW)); Of course, one may argue that this is a heuristic and can fail. Well, GetDevCaps is a heuristic as well that only supports devtype_digital_video. An advantage IMHO of the existing approach is that it does not invent within the same source code base yet another mechanism to map 16 to 32 or A to W. I'd rather follow the existing model until the time it proves unbearable, or immediately switch the *whole* code base to another model, not the single MCI_STATUS function. For instance, you can argue that it makes sense for winmm to cache the device type (winmm:MCI_Open has it in several cases). It would arguably also make sense to cache the MCI auto_open'ed property, but I digress. Your MCI_get_device_type is bogus. MCI_GETDEVCAPS_DEVICE_TYPE belongs into dwItem, not dwFlags. Regards, Jörg Höhle
On Fri, May 6, 2011 at 6:23 PM, <Joerg-Cyril.Hoehle(a)t-systems.com> wrote:
Damjan,
@@ -407,6 +441,20 @@ static MMSYSTEM_MapType MCI_UnMapMsg16To32W(WORD wMsg, DWORD dwFlags, DWORD_PTR + mdsp16->dwCallback = mdsp32w->dwCallback; + mdsp16->dwItem = mdsp32w->dwItem; + mdsp16->dwTrack = mdsp32w->dwTrack; This is superfluous. dwReturn is the only documented output. This was different with your former MCI_WHERE patch, where a RECT is output.
I'm aware it will be superfluous in the majority of cases, but are you convinced no MCI driver ever modifies fields it shouldn't, and no application depends on that exceptional behaviour?
I'd try hard not to turn one message call (Status) into 2 (GetDevCaps + Status). I've seen logs of apps that continuously poll MCI_STATUS_POSITION. Instead, I'd continue the path lead by winmm/mci.c:MCI_MapMsgAtoW: Allocate the largest structure and fill it depending on dwFlags, e.g.: case MCI_OPEN: { /* MCI_ANIM_OPEN_PARMS is the largest known MCI_OPEN_PARMS * structure, larger than MCI_WAVE_OPEN_PARMS */ BTW, I've a note that MCI_INFO needs similar treatment, but never wrote the patch...
Select the right set of flags to fill the slots, otherwise fallback to defaults like: /* We don't know how many DWORD follow, as * the structure depends on the device. */ if (HIWORD(dwParam1)) memcpy(&mci_openW->dwStyle, &mci_openA->dwStyle, sizeof(MCI_ANIM_OPEN_PARMSW) - sizeof(MCI_OPEN_PARMSW));
Of course, one may argue that this is a heuristic and can fail. Well, GetDevCaps is a heuristic as well that only supports devtype_digital_video.
What determines what subtype of MCI_STATUS_PARMS is passed to mciSendCommand() for an MCI_STATUS message? According to the API, "For VCR devices" uses one type, "For digital-video devices" uses another type, etc. But must the digital video MCI driver assume every MCI_STATUS lpStatus parameter points to MCI_DGV_STATUS_PARMS, or must it assume it got MCI_DGV_STATUS_PARMS only when it deals with a DGV-specific status request like MCI_DGV_STATUS_DISKSPACE and fall back to the generic MCI_STATUS_PARMS for all other cases?
An advantage IMHO of the existing approach is that it does not invent within the same source code base yet another mechanism to map 16 to 32 or A to W. I'd rather follow the existing model until the time it proves unbearable, or immediately switch the *whole* code base to another model, not the single MCI_STATUS function.
For instance, you can argue that it makes sense for winmm to cache the device type (winmm:MCI_Open has it in several cases). It would arguably also make sense to cache the MCI auto_open'ed property, but I digress.
Your MCI_get_device_type is bogus. MCI_GETDEVCAPS_DEVICE_TYPE belongs into dwItem, not dwFlags.
Let's finish this discussion, then I'll see whether to fix that bug or eliminate the entire function.
Regards, Jörg Höhle
Regards Damjan Jovanovic
Hi, Damjan Jovanovic wrote:
What determines what subtype of MCI_STATUS_PARMS is passed to mciSendCommand() for an MCI_STATUS message?
I consider the string interface the defining property of MCI devices. This sets it apart from e.g. COM interfaces. Further I believe the string and command interfaces to be interchangeable. This has several consequences. A. The string interfaces only provides one return value. There's no need to provide other output variables since the string interface cannot return them. This answers
are you convinced no MCI driver ever modifies fields it shouldn't, and no application depends on that exceptional behaviour? with yes. If you suspect a deviating behaviour, then by all means, write a test.
For instance, I was surprised when my tests revealed that in some version of MS some of the MCI drivers modify dwRetSize (in INFO_PARMS). I read into VirtualAlloc() the other day to find out how to write-protect memory so as to detect whether some a priori read-only slot is written to. Of course, this does not prevent e.g. a root kit to hide as an MCI driver and write whatever into the PARMS structure. B. The MCI_*_PARMS and the MCI resource files must be congruent. The MCI parser does not know any PARMS structure. It uses the resource description to reconstruct the layout of the PARMS structure. C. Each device recognizes one structure per command. The structure is defined by the device's resource file (defaulting to winmm_res.rc). This answers
must [the driver] assume it got MCI_DGV_STATUS_PARMS only when it deals with a DGV-specific status request like MCI_DGV_STATUS_DISKSPACE and fall back to the generic MCI_STATUS_PARMS for all other cases? with no. Driver X can and must assume that it received a MCI_X_STATUS_PARMS. That's what it receives from the MCI parser.
All MCI_*_STATUS_PARMS need not have their slots in the same place, but it would be unwise to deviate from the order of the core commands. Then you couldn't send an MCI_STATUS_MODE using the generic MCI_STATUS_PARMS to an unknown MCI device! Only "status x mode" would work. Wine's A/W and 16/32 MCI mappers depend on this common ordering. Note that the device type only selects the command set when defaulting to winmm_res.rc. A driver providing its own resource (like mciavi) can return any device type it sees fit -- I still wonder why mciavi returns type "digitalvideo" instead of "animation". In the end, the resource defines the layout and extra parameters that the driver purports to recognize. What I mean is that dispatching on devtype is no guarantee for correctness, rather than a good guess. These were the rules, now the exceptions. :-) - I believe MS goofed with mciavi/mciqtz32 because MCI_DGV_SETVIDEO_PARMS and mciavi_res.rc publish 4 slots - over, quality, algorithm, number - in non-matching order. Hence you cannot use the generic MCI parser to construct the layout in digitalv.h I never wrote a test to see whether there are some MS hacks to map one to the other, or whether the .h is incorrect. - Using the MCI string interface always involves an alias, whereas you can use the command interface without it. - The MCI_BREAK system command has an hwndBreak slot and associated MCI_BREAK_HWND flag that the string interface does not know about. - dwRetSize update mentioned above. After I discovered that, I had Wine implement the "straight" behaviour because the modification is not documented, not all drivers implement it across all versions, not all commands do it, native implements it bogusly for dwRetSize=0 and its value is unclear with W/A: so let's not write to it. I put a comment in the code to signal this irregularity. Does an application depend on the update? Who knows? Possible rewrites and future directions: - Several times I thought about passing a bUnicode flag within winmm's MCI parser to delay the mapping from A to W. Never written. - I thought about reusing the PARMS structure in-place, replacing string A with W pointers. This would eliminate the current hassle of not knowing the size of structure. All that one needs to know is the location of the string pointers. Never written. That's obviously not feasible with the 16/32 mapping due to differing offsets. Another, less fancy approach would be to parse the resource strings even for MCI commands so as to know the layout. It would slow down every command. But it would be correct. - One problem remains with OPEN. Basically, it looks like the MCI needs to preparse the "open ..." string to determine which device to open. Only when the device is known does winmm know which resource to use for parsing! E.g. mciwave accepts an extra buffer parameter. I don't believe native would do two passes. It may well be that native's parser is an implementation of the IBM patent I mentioned some month ago. Perhaps that helps solve the issue. - The OPEN issue affects the 16/32 mapper too. After all, MCI_ANIM_OPEN_PARMS contains a HWND beyond the common parameters... How to find out whether the OPEN will go to a device that uses extra slots? ... And how to fill these slots? The only solution path that I currently see is that mmsystem16 would duplicate some of winmm's job: send the 3 messages to load and open the driver to obtain the resource, then only call MCI_OPEN. Any of these steps means much more code than the simple^Histic approach which has worked surprisingly well for the last 10 years: "if flag 00020000 is set in OPEN, it must be MCI_*_OPEN_PARENT, as none of the other 4 standard drivers (wave, MIDI, avi qtz, CDA) uses that bit." Likewise MCI_DGV_STATUS_DISKSPACE Regards, Jörg Höhle
On Mon, May 9, 2011 at 6:35 PM, <Joerg-Cyril.Hoehle(a)t-systems.com> wrote:
Any of these steps means much more code than the simple^Histic approach which has worked surprisingly well for the last 10 years: "if flag 00020000 is set in OPEN, it must be MCI_*_OPEN_PARENT, as none of the other 4 standard drivers (wave, MIDI, avi qtz, CDA) uses that bit." Likewise MCI_DGV_STATUS_DISKSPACE
Thank you for the detailed explanation. The problem then becomes that if a driver expects MCI_DGV_STATUS_PARMS, and we pass MCI_STATUS_PARMS because the request didn't looks like it's destined for a digitalvideo device, it might go and read/write a field that's outside the bounds of the allocated structure. But since it seems that the extra fields should generally only be used if special flags were also passed, and those flags would allow us to detect the device type and pass the right structure, I suppose the current heuristics are good enough. And now I can see why Microsoft abandoned further MCI development :-).
Regards, Jörg Höhle
Regards Damjan Jovanovic
Executive summary: I believe LPSTR is all wrong in mmsystem16.h's MCI defs. Hi, I've some doubts about the correctness of some definitions in mmsystem16.h, esp. the use of SEGPTR vs. LPSTR types. Please compare typedef struct { DWORD dwCallback; LPSTR lpstrReturn; ... } MCI_DGV_INFO_PARMS16, * LPMCI_DGV_INFO_PARMS16; http://source.winehq.org/source//include/wine/mmsystem16.h typedef struct { DWORD dwCallback; SEGPTR lpstrReturn; ... } MCI_INFO_PARMS16, *LPMCI_INFO_PARMS16; These definitions do not match. SEGPTR requires the use of the MapSL() helper function to correctly access the segmented pointer, while LPSTR is a linear string pointer. For reasons I recently explained in http://www.winehq.org/pipermail/wine-devel/2011-May/090060.html I very much doubt that both pointer types were used simultaneously -- unless the digital video device (mciavi16.dll?) embedded different return types in its resource file. It's not impossible, just very unlikely. Within one winmm, The return type MCI_STRING (value 0001) can *either* denote linear C strings, or segmented ones. Should 2 types really have been used as the above structure definitions suggest, then one will have a distinct return value in its resource definition file (e.g. mciavi_res.rc or extractable from the .dll). For instance, that's how we derived that there must exist a distinct value for 64bit use we then named MCI_INTEGER64 in Wine's mmddk.h However, I feel it's more likely that LPSTR above is wrong, that no distinct return type was used and that segmented pointers were used in most places within the MCI in 16 bit times, partly because Wine currently uses the SEGPTR definitions and MapSL() helpers in its current code base and would have crashed otherwise (but if that code is never exercised?). Likewise for MCI_OPEN_PARMS16 and MCI_DGV_OPEN_PARMS16. Wine's current MCI code would choke if it had to deal with 2 different types of pointers here. To give another example, I find it illogical that MCI_LOAD_PARMS16 takes an LPCSTR, while MCI_SOUND_PARMS16 takes a SEGPTR according to mmsystem16.h. It's not about a different name, it's entirely different content! I'd be pleased if somebody could grab out an old SDK and have a look at the includes or otherwise shed light on this issue. Thank you for your help, Jörg Höhle
<Joerg-Cyril.Hoehle(a)t-systems.com> writes:
I'd be pleased if somebody could grab out an old SDK and have a look at the includes or otherwise shed light on this issue.
You won't find anything in the SDK. SEGPTR is a Wine invention to help distinguish pointers that have already been converted. The SDK uses LPSTR etc. throughout, but these are obviously all segmented pointers in 16-bit code. -- Alexandre Julliard julliard(a)winehq.org
On Mon, May 23, 2011 at 2:54 PM, <Joerg-Cyril.Hoehle(a)t-systems.com> wrote:
Executive summary: I believe LPSTR is all wrong in mmsystem16.h's MCI defs.
Hi,
I've some doubts about the correctness of some definitions in mmsystem16.h, esp. the use of SEGPTR vs. LPSTR types.
Please compare typedef struct { DWORD dwCallback; LPSTR lpstrReturn; ... } MCI_DGV_INFO_PARMS16, * LPMCI_DGV_INFO_PARMS16; http://source.winehq.org/source//include/wine/mmsystem16.h typedef struct { DWORD dwCallback; SEGPTR lpstrReturn; ... } MCI_INFO_PARMS16, *LPMCI_INFO_PARMS16;
These definitions do not match. SEGPTR requires the use of the MapSL() helper function to correctly access the segmented pointer, while LPSTR is a linear string pointer.
For reasons I recently explained in http://www.winehq.org/pipermail/wine-devel/2011-May/090060.html I very much doubt that both pointer types were used simultaneously -- unless the digital video device (mciavi16.dll?) embedded different return types in its resource file. It's not impossible, just very unlikely.
Within one winmm, The return type MCI_STRING (value 0001) can *either* denote linear C strings, or segmented ones. Should 2 types really have been used as the above structure definitions suggest, then one will have a distinct return value in its resource definition file (e.g. mciavi_res.rc or extractable from the .dll). For instance, that's how we derived that there must exist a distinct value for 64bit use we then named MCI_INTEGER64 in Wine's mmddk.h
However, I feel it's more likely that LPSTR above is wrong, that no distinct return type was used and that segmented pointers were used in most places within the MCI in 16 bit times, partly because Wine currently uses the SEGPTR definitions and MapSL() helpers in its current code base and would have crashed otherwise (but if that code is never exercised?).
Likewise for MCI_OPEN_PARMS16 and MCI_DGV_OPEN_PARMS16. Wine's current MCI code would choke if it had to deal with 2 different types of pointers here.
To give another example, I find it illogical that MCI_LOAD_PARMS16 takes an LPCSTR, while MCI_SOUND_PARMS16 takes a SEGPTR according to mmsystem16.h. It's not about a different name, it's entirely different content!
I'd be pleased if somebody could grab out an old SDK and have a look at the includes or otherwise shed light on this issue.
Thank you for your help, Jörg Höhle
AFAICT Wine's 16 bit headers cannot be used to compile Win16 code into a Winelib. The only purpose of Wine's 16 bit headers is to allow 32 bit code to read and write 16 bit structures and thus play nicely with 16 bit binaries. That's why Wine's types and names of 16 bit fields, structures, functions and headers don't always match what Win16 actually used, and why those headers are in a special directory that won't exist on any Windows API (include/wine/). According to Openwatcom's headers, LPSTR is a FAR pointer to char. "FAR" is a 16:16 segmented pointer, which makes sense, because Win16, being 16 bit, had no 32 bit linear pointer type in user-space (only kernel VxDs had it). So the Win16 MCI headers' LP[C]STRs were still 16:16 pointers to char. The SEGPTR type doesn't exist on Win16 at all. It's a Wine invention to conveniently mark a 16:16 pointer and since it's an integer type, it catches attempts to assign directly between 32 bit linear pointers and SEGPTRs with compiler warnings. So it seems Wine's headers should never use LP[C]STR in 16 bit structures, SEGPTR should always be used instead. Damjan Jovanovic
participants (3)
-
Alexandre Julliard -
Damjan Jovanovic -
Joerg-Cyril.Hoehle＠t-systems.com