[1/2] mmsystem.dll16: fix MCI_STATUS mapping for digitalvideo

[Damjan Jovanovic]

On Fri, May 6, 2011 at 6:23 PM,  <Joerg-Cyril.Hoehle at 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