[PATCH v2 1/6] mciqtz32: Simplify video window management by creating default window.
Zebediah Figura
zfigura at codeweavers.com
Thu Jul 7 14:19:02 CDT 2022
On 7/5/22 06:34, Akihiro Sagawa wrote:
> From: Akihiro Sagawa <sagawa.aki at gmail.com>
>
> Now the video window by IVideoWindow always has WS_CHILD style regardless
> open parameters.
>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52448
> Signed-off-by: Akihiro Sagawa <sagawa.aki at gmail.com>
What's being simplified, exactly? As far as I can tell we're changing
our behaviour to be more correct, but it's certainly not getting simpler.
> ---
> dlls/mciqtz32/mciqtz.c | 162 +++++++++++++++++++++++++--------
> dlls/mciqtz32/mciqtz_private.h | 1 +
> 2 files changed, 127 insertions(+), 36 deletions(-)
>
> diff --git a/dlls/mciqtz32/mciqtz.c b/dlls/mciqtz32/mciqtz.c
> index a7ec0b78f33..92622d848a9 100644
> --- a/dlls/mciqtz32/mciqtz.c
> +++ b/dlls/mciqtz32/mciqtz.c
> @@ -31,8 +31,11 @@
>
> WINE_DEFAULT_DEBUG_CHANNEL(mciqtz);
>
> +#define MCIQTZ_CLASS L"MCIQTZ_Window"
There's no need for this to be a macro, right? In that case I think it
should be a "static const WCHAR []" string instead.
> +
> static DWORD MCIQTZ_mciClose(UINT, DWORD, LPMCI_GENERIC_PARMS);
> static DWORD MCIQTZ_mciStop(UINT, DWORD, LPMCI_GENERIC_PARMS);
> +static DWORD MCIQTZ_mciPause(UINT, DWORD, LPMCI_GENERIC_PARMS);
>
> /*======================================================================*
> * MCI QTZ implementation *
> @@ -68,6 +71,62 @@ static WINE_MCIQTZ* MCIQTZ_mciGetOpenDev(UINT wDevID)
> return wma;
> }
>
> +static LRESULT WINAPI MCIQTZ_WindowProc(HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
> +{
> + TRACE("hwnd=%p msg=%x wparam=%Ix lparam=%Ix\n", hWnd, uMsg, wParam, lParam);
Please trace hexadecimal parameters with #.
> +
> + switch (uMsg) {
> + case WM_CREATE:
> + {
> + LPARAM wDevId = (LPARAM)((CREATESTRUCTW *)lParam)->lpCreateParams;
> + SetWindowLongW(hWnd, 0, wDevId);
> + return DefWindowProcW(hWnd, uMsg, wParam, lParam);
Perhaps "break" and move the DefWindowProc() call out of the switch?
> + }
> +
> + case WM_CLOSE:
> + {
> + MCIQTZ_mciPause(GetWindowLongW(hWnd, 0), MCI_WAIT, NULL);
> + ShowWindow(hWnd, SW_HIDE);
> + return 0;
> + }
It seems probably easy enough to test this in winmm, but even failing
that, manual tests would be nice.
> +
> + case WM_ERASEBKGND:
> + {
> + RECT rect;
> + GetClientRect(hWnd, &rect);
> + FillRect((HDC)wParam, &rect, GetStockObject(BLACK_BRUSH));
> + return 1;
> + }
Why this instead of setting the background brush to black?
> +
> + default:
> + return DefWindowProcW(hWnd, uMsg, wParam, lParam);
> + }
> +}
> +
> +static BOOL MCIQTZ_UnregisterClass(void)
> +{
> + return UnregisterClassW(MCIQTZ_CLASS, MCIQTZ_hInstance);
> +}
> +
> +static BOOL MCIQTZ_RegisterClass(void)
> +{
> + WNDCLASSW wndClass;
> +
> + ZeroMemory(&wndClass, sizeof(WNDCLASSW));
> + wndClass.style = CS_DBLCLKS;
Why CS_DBLCLKS? We're not using it.
> + wndClass.lpfnWndProc = MCIQTZ_WindowProc;
> + wndClass.cbWndExtra = sizeof(MCIDEVICEID);
> + wndClass.hInstance = MCIQTZ_hInstance;
> + wndClass.hCursor = LoadCursorW(0, (LPCWSTR)IDC_ARROW);
> + wndClass.hbrBackground = (HBRUSH)(COLOR_3DFACE + 1);
> + wndClass.lpszClassName = MCIQTZ_CLASS;
> +
> + if (RegisterClassW(&wndClass)) return TRUE;
> + if (GetLastError() == ERROR_CLASS_ALREADY_EXISTS) return TRUE;
> +
> + return FALSE;
> +}
> +
> /**************************************************************************
> * MCIQTZ_drvOpen [internal]
> */
> @@ -81,6 +140,8 @@ static DWORD MCIQTZ_drvOpen(LPCWSTR str, LPMCI_OPEN_DRIVER_PARMSW modp)
> if (!modp)
> return 0xFFFFFFFF;
>
> + if (!MCIQTZ_RegisterClass()) return 0;
> +
> wma = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(WINE_MCIQTZ));
> if (!wma)
> return 0;
> @@ -109,6 +170,7 @@ static DWORD MCIQTZ_drvClose(DWORD dwDevID)
> /* finish all outstanding things */
> MCIQTZ_mciClose(dwDevID, MCI_WAIT, NULL);
>
> + MCIQTZ_UnregisterClass();
> mciFreeCommandResource(wma->command_table);
> mciSetDriverData(dwDevID, 0);
> CloseHandle(wma->stop_event);
> @@ -139,6 +201,54 @@ static DWORD MCIQTZ_drvConfigure(DWORD dwDevID)
> return 1;
> }
>
> +static BOOL MCIQTZ_CreateWindow(WINE_MCIQTZ* wma, DWORD dwFlags, LPMCI_DGV_OPEN_PARMSW lpParms)
> +{
> + HWND hParent = NULL;
> + DWORD dwStyle = (WS_OVERLAPPEDWINDOW | WS_CLIPCHILDREN | WS_CLIPSIBLINGS) & ~WS_MAXIMIZEBOX;
> + RECT rc;
> + HRESULT hr;
> +
> + /* what should be done ? */
> + if (wma->window) return TRUE;
What should be done?
> +
> + if (dwFlags & MCI_DGV_OPEN_PARENT) hParent = lpParms->hWndParent;
> + if (dwFlags & MCI_DGV_OPEN_WS) dwStyle = lpParms->dwStyle;
> +
> + dwStyle &= ~WS_VISIBLE;
> +
> + rc.left = rc.top = 0;
> + hr = IBasicVideo_GetVideoSize(wma->vidbasic, &rc.right, &rc.bottom);
> + if (FAILED(hr)) return hr == E_NOINTERFACE; /* no video renderer ? */
When can that happen?
> +
> + AdjustWindowRect(&rc, dwStyle, FALSE);
> + if (!(dwStyle & (WS_CHILD|WS_POPUP))) /* overlapped window ? */
Where is this logic coming from? Do you have tests?
This also loses the previous logic that uses GetClientRect() on the
parent. That might be correct, but it lacks tests, and it should
probably be done in a separate patch.
> + {
> + rc.right -= rc.left;
> + rc.bottom -= rc.top;
Aren't "left" and "top" zero?
It seems like it'd be less confusing in general to use separate
width/height (or x/y) variables instead of passing rc.right and
rc.bottom directly to AdjustWindow.
> + rc.left = rc.top = CW_USEDEFAULT;
> + }
> +
> + wma->window = CreateWindowW(MCIQTZ_CLASS, lpParms->lpstrElementName, dwStyle,
> + rc.left, rc.top, rc.right, rc.bottom,
> + hParent, 0, MCIQTZ_hInstance,
> + ULongToPtr(wma->wDevID));
> +
> + TRACE("(%04x, %08lX, %p, style %lx, parent %p, dimensions %ldx%ld, window %p)\n", wma->wDevID,
> + dwFlags, lpParms, dwStyle, hParent, rc.right - (rc.left == CW_USEDEFAULT ? 0 : rc.left), rc.bottom - (rc.top == CW_USEDEFAULT ? 0 : rc.top), wma->window);
> +
> + if (!wma->window)
> + return FALSE;
> +
> + wma->parent = wma->window;
> + IVideoWindow_put_AutoShow(wma->vidwin, OAFALSE);
> + IVideoWindow_put_MessageDrain(wma->vidwin, (OAHWND)wma->parent);
> + IVideoWindow_put_WindowStyle(wma->vidwin, WS_CHILD);
put_Owner() automatically sets WS_CHILD.
> + IVideoWindow_put_Owner(wma->vidwin, (OAHWND)wma->parent);
> + IVideoWindow_put_Visible(wma->vidwin, OATRUE);
> +
> + return TRUE;
> +}
> +
> /**************************************************************************
> * MCIQTZ_mciNotify [internal]
> *
> @@ -161,8 +271,6 @@ static DWORD MCIQTZ_mciOpen(UINT wDevID, DWORD dwFlags,
> {
> WINE_MCIQTZ* wma;
> HRESULT hr;
> - DWORD style = 0;
> - RECT rc = { 0, 0, 0, 0 };
>
> TRACE("(%04x, %08lX, %p)\n", wDevID, dwFlags, lpOpenParms);
>
> @@ -238,22 +346,7 @@ static DWORD MCIQTZ_mciOpen(UINT wDevID, DWORD dwFlags,
> goto err;
> }
>
> - IVideoWindow_put_AutoShow(wma->vidwin, OAFALSE);
> - IVideoWindow_put_Visible(wma->vidwin, OAFALSE);
> - if (dwFlags & MCI_DGV_OPEN_WS)
> - style = lpOpenParms->dwStyle;
> - if (dwFlags & MCI_DGV_OPEN_PARENT) {
> - IVideoWindow_put_MessageDrain(wma->vidwin, (OAHWND)lpOpenParms->hWndParent);
> - IVideoWindow_put_WindowState(wma->vidwin, SW_HIDE);
> - IVideoWindow_put_WindowStyle(wma->vidwin, style|WS_CHILD);
> - IVideoWindow_put_Owner(wma->vidwin, (OAHWND)lpOpenParms->hWndParent);
> - GetClientRect(lpOpenParms->hWndParent, &rc);
> - IVideoWindow_SetWindowPosition(wma->vidwin, rc.left, rc.top, rc.right - rc.top, rc.bottom - rc.top);
> - wma->parent = (HWND)lpOpenParms->hWndParent;
> - }
> - else if (style)
> - IVideoWindow_put_WindowStyle(wma->vidwin, style);
> - IBasicVideo_GetVideoSize(wma->vidbasic, &rc.right, &rc.bottom);
> + MCIQTZ_CreateWindow(wma, dwFlags, lpOpenParms);
This ignores the return value.
> wma->opened = TRUE;
>
> if (dwFlags & MCI_NOTIFY)
> @@ -307,6 +400,13 @@ static DWORD MCIQTZ_mciClose(UINT wDevID, DWORD dwFlags, LPMCI_GENERIC_PARMS lpP
> MCIQTZ_mciStop(wDevID, MCI_WAIT, NULL);
>
> if (wma->opened) {
> + if (wma->window)
Assuming we were to check for failure in MCIQTZ_mciOpen(), which I
imgaine we should do, can this happen?
There are other similar cases below.
> + {
> + IVideoWindow_put_MessageDrain(wma->vidwin, (OAHWND)NULL);
> + IVideoWindow_put_Owner(wma->vidwin, (OAHWND)NULL);
> + DestroyWindow(wma->window);
> + wma->window = NULL;
> + }
> IVideoWindow_Release(wma->vidwin);
> IBasicVideo_Release(wma->vidbasic);
> IBasicAudio_Release(wma->audio);
> @@ -447,7 +547,8 @@ static DWORD MCIQTZ_mciPlay(UINT wDevID, DWORD dwFlags, LPMCI_PLAY_PARMS lpParms
> return MCIERR_INTERNAL;
> }
>
> - IVideoWindow_put_Visible(wma->vidwin, OATRUE);
> + if (wma->parent)
> + ShowWindow(wma->parent, SW_SHOW);
>
> wma->thread = CreateThread(NULL, 0, MCIQTZ_notifyThread, wma, 0, NULL);
> if (!wma->thread) {
> @@ -525,9 +626,6 @@ static DWORD MCIQTZ_mciStop(UINT wDevID, DWORD dwFlags, LPMCI_GENERIC_PARMS lpPa
> wma->thread = NULL;
> }
>
> - if (!wma->parent)
> - IVideoWindow_put_Visible(wma->vidwin, OAFALSE);
> -
The tests bear this out, but it's also a change in behaviour that seems
outside the scope of this patch.
> return 0;
> }
>
> @@ -574,6 +672,9 @@ static DWORD MCIQTZ_mciResume(UINT wDevID, DWORD dwFlags, LPMCI_GENERIC_PARMS lp
> return MCIERR_INTERNAL;
> }
>
> + if (wma->parent)
> + ShowWindow(wma->parent, SW_SHOW);
> +
This doesn't have tests, and it also seems outside the scope of this patch.
> return 0;
> }
>
> @@ -919,21 +1020,10 @@ static DWORD MCIQTZ_mciWindow(UINT wDevID, DWORD dwFlags, LPMCI_DGV_WINDOW_PARMS
> return 0;
>
> if (dwFlags & MCI_DGV_WINDOW_HWND && (IsWindow(lpParms->hWnd) || !lpParms->hWnd)) {
> - LONG visible = OATRUE;
> - LONG style = 0;
> TRACE("Setting hWnd to %p\n", lpParms->hWnd);
> - IVideoWindow_get_Visible(wma->vidwin, &visible);
> - IVideoWindow_put_Visible(wma->vidwin, OAFALSE);
> - IVideoWindow_get_WindowStyle(wma->vidwin, &style);
> - style &= ~WS_CHILD;
> - if (lpParms->hWnd)
> - IVideoWindow_put_WindowStyle(wma->vidwin, style|WS_CHILD);
> - else
> - IVideoWindow_put_WindowStyle(wma->vidwin, style);
> - IVideoWindow_put_Owner(wma->vidwin, (OAHWND)lpParms->hWnd);
> - IVideoWindow_put_MessageDrain(wma->vidwin, (OAHWND)lpParms->hWnd);
> - IVideoWindow_put_Visible(wma->vidwin, visible);
> - wma->parent = lpParms->hWnd;
> + wma->parent = lpParms->hWnd ? lpParms->hWnd : wma->window;
> + IVideoWindow_put_MessageDrain(wma->vidwin, (OAHWND)wma->parent);
> + IVideoWindow_put_Owner(wma->vidwin, (OAHWND)wma->parent);
> }
Should we also hide "wma->window" here?
> if (dwFlags & MCI_DGV_WINDOW_STATE) {
> TRACE("Setting nCmdShow to %d\n", lpParms->nCmdShow);
More information about the wine-devel
mailing list