[MPlayer-dev-eng] [PATCH] vo_kva
Reimar Döffinger
Reimar.Doeffinger at gmx.de
Sun Mar 8 12:24:54 CET 2009
>>> + WinCalcFrameRect(hwnd, &rcl, FALSE);
>>> +
>>> + if (rcl.xRight - rcl.xLeft > vo_screenwidth) {
>>> + rcl.xLeft = 0;
>>> + rcl.xRight = vo_screenwidth;
>>> + }
>>> +
>>> + if (rcl.yTop - rcl.yBottom > vo_screenheight) {
>>> + rcl.yBottom = 0;
>>> + rcl.yTop = vo_screenheight;
>>> + }
>>
>> Are you really sure you want to limit the Window size to the screen
>> size? The code for Windows specifically disables this limitation because
>> it can be really annoying for HD videos.
>
> It's a workaround for T23 laptop with S3 video card. If a movie window size
> is larger than a screen size, the image is distorted.
Well, you already have a flag for that hardware, right? You could use it
here, too.
Anyway it is your decision, I just wanted to point out that the vos that
I maintain will specifically try to avoid that behaviour and your vo is
probably going to stand out from the others by behaving differently,
though that is not necessarily a bad thing.
>>> + // if slave mode, ignore mouse events and deliver them to a parent
>>> window
>>> + if (WinID != -1 &&
>>> + ((msg >= WM_MOUSEFIRST && msg <= WM_MOUSELAST) ||
>>> + (msg >= WM_EXTMOUSEFIRST && msg <= WM_EXTMOUSELAST))) {
>>> + WinPostMsg(HWNDFROMWINID(WinID), msg, mp1, mp2);
>>> +
>>> + return (MRESULT)TRUE;
>>> + }
>>>
>>
>> Note that on Windows the approach has a few issue (e.g. some messages
>> simply can't be passed on), simply creating/using a disabled Window
>> works much better.
>
> Fortunately, it works fine on OS/2. BTW, what is a disabled window ?
Just to clarify, what did not work on Windows specifically was
drag-and-drop, if the -wid window supported drag and drop you still
could not use that as soon as the MPlayer window was above it.
A disabled window is a window that can not receive any input, on Windows you
use EnableWindow(vo_window, 0); to set that state. This is usually what
is used to disable buttons (in addition to graying them out) so I'd
expect that to exist on OS/2, too - but I don't know.
>> There is also the question why you even create a Window in -wid mode
>> instead of using the -wid one, the only reason it is done on Windows
>> is that NVidia OpenGL drivers are buggy...
>
> If I understand what you are saying correctly, because it's not possible to
> access the window created by other process on OS/2.
Well, I forgot that that applies only to OpenGL, GDI (2D drawing
functions) can't even be used on a window created by a different thread
even on Windows.
>>> + case WM_BUTTON1DOWN:
>>> + if (WinQueryFocus(HWND_DESKTOP) != hwnd)
>>> + WinSetFocus(HWND_DESKTOP, hwnd);
>>> + else if (!vo_nomouse_input)
>>> + mplayer_put_key(MOUSE_BTN0);
>>> +
>>> + return (MRESULT)TRUE;
>>> +
>>> + case WM_BUTTON3DOWN:
>>> + if (WinQueryFocus(HWND_DESKTOP) != hwnd)
>>> + WinSetFocus(HWND_DESKTOP, hwnd);
>>> + else if (!vo_nomouse_input)
>>> + mplayer_put_key(MOUSE_BTN1);
>>> +
>>> + return (MRESULT)TRUE;
>>> +
>>> + case WM_BUTTON2DOWN:
>>> + if (WinQueryFocus(HWND_DESKTOP) != hwnd)
>>> + WinSetFocus(HWND_DESKTOP, hwnd);
>>> + else if (!vo_nomouse_input)
>>> + mplayer_put_key(MOUSE_BTN2);
>>> +
>>> + return (MRESULT)TRUE;
>>>
>>
>> It seems to me that that WinQueryFocus/WinSetFocus is overriding the
>> behaviour intended by the OS. Don't do it unless that behaviour is
>> important to you.
>>
>
> It's to separate focus changing and clicking. It can prevent from mistaken
> clicking
Whether the click that moves focus/brings a window on top counts as
click into the window as well is a matter of OS policy, too (as evident
by Linux window-managers allowing to choose this).
So I think my point stands, but as said if it is important to you keep
it.
>>> + case WM_PAINT:
>>> + {
>>> + HPS hps;
>>> + RECTL rcl, rclDst;
>>> + PRECTL prcl = NULL;
>>> + HRGN hrgn, hrgnDst;
>>> + RGNRECT rgnCtl;
>>> +
>>> + kvaAdjustDstRect(&m_int.kvas.rclSrcRect, &rclDst);
>>> +
>>> + hps = WinBeginPaint(hwnd, NULLHANDLE, &rcl);
>>> +
>>> + hrgn = GpiCreateRegion(hps, 1, &rcl);
>>> + hrgnDst = GpiCreateRegion(hps, 1, &rclDst);
>>> +
>>> + GpiCombineRegion(hps, hrgn, hrgn, hrgnDst, CRGN_DIFF);
>>> +
>>> + rgnCtl.ircStart = 1;
>>> + rgnCtl.ulDirection = RECTDIR_LFRT_TOPBOT;
>>> + GpiQueryRegionRects(hps, hrgn, NULL, &rgnCtl, NULL);
>>> +
>>> + if (rgnCtl.crcReturned > 0) {
>>> + rgnCtl.crc = rgnCtl.crcReturned;
>>> + prcl = malloc(sizeof(RECTL) * rgnCtl.crcReturned);
>>> + }
>>> +
>>> + if (prcl && GpiQueryRegionRects(hps, hrgn, NULL, &rgnCtl, prcl))
>>> {
>>> + int i;
>>> +
>>> + for (i = 0; i < rgnCtl.crcReturned; i++)
>>> + WinFillRect(hps, &prcl[i], CLR_BLACK);
>>> + }
>>> +
>>> + free(prcl);
>>> +
>>> + GpiDestroyRegion(hps, hrgnDst);
>>> + GpiDestroyRegion(hps, hrgn);
>>> +
>>> + WinEndPaint(hps);
>>
>> Can't you just redraw the whole Window? Given that you need to draw into
>> it about 25 times per second anyway it seems like partial redrawing is
>> not worth the effort.
>
> It's for black-bar. And it is redrawn only if the area is invalidated.
Sure, I am just asking if _only_ redrawing the invalidated area is worth
the code or if it wouldn't make sense to just draw the whole Window
(i.e. something like
hps = WinBeginPaint(hwnd, NULLHANDLE, NULL);
WinQueryWindowRect(hwnd, &rcl);
WinFillRect(hps, &rcl, CLR_BLACK);
WinEndPaint(hps);
which is much simpler, though e.g. it might cause flicker).
>>> + if (fUseSnap)
>>> + kvaMode = KVAM_SNAP;
>>> + else if (fUseWO)
>>> + kvaMode = KVAM_WO;
>>> + else if (fUseDive)
>>> + kvaMode = KVAM_DIVE;
>>> + else
>>> + kvaMode = KVAM_AUTO;
>>
>> I usually would use a int flag to choose between different options,
>> though I admit it is not the most user-friendly approach.
>> If you prefer it like this you should at least catch e.g.
>> -vo kva:wo:dive
>
> Do you mean 'dive' should be selected ? If so, it does not. As you know, if
> multiple mode is specified, snap > wo > dive is selected. Should I use
> another approach ?
At least document that specifying more than one does not make sense, or
even check it, e.g.
> if (!!fUseSnap + !!fUseWO + !!fUseDive > 1) {
> print warning/error message
> }
>>> + if (kvaInit(kvaMode, m_int.hwndClient, vo_colorkey & 0xFFFFFF)) {
>>>
>>
>> Is there a reason to apply a mask to vo_colorkey? Sure they probably
>> make no sense,
>
> Ok if bit mask has nonsense. I just followed the way which the other vo
> drivers do.
Well... the difference is those other drivers support -nocolorkey, the highest
bits are used for that. Or does kva support disabling colorkey?
>> but if the user explicitly specified it so, why add extra
>> code to second guess them?
>
> What is 'extra code' ?
Well, the "& 0xFFFFFF".
>>> +static uint32_t get_image(mp_image_t *mpi)
>>> +{
>>> + if (mpi->flags & MP_IMGFLAG_PLANAR) {
>>> + mpi->planes[1] = m_int.planes[1];
>>> + mpi->planes[2] = m_int.planes[2];
>>> +
>>> + mpi->stride[1] = m_int.stride[1];
>>> + mpi->stride[2] = m_int.stride[2];
>>> + }
>>> +
>>> + mpi->planes[0] = m_int.planes[0];
>>> + mpi->stride[0] = m_int.stride[0];
>>> + mpi->flags |= MP_IMGFLAG_DIRECT;
>>> +
>>> + return VO_TRUE;
>>>
>>
>> That's not going to work (did you actually try it with -dr?).
>
> No.
>
>> In particular, since there is only one buffer, you at least can not
>> provide
>> MP_IMGTYPE_IP or MP_IMGTYPE_IPB
>
> I just refered to the way other vo drivers such as vo_directx.c and
> vo_sdl.c do.
Not quite, you missed the
if(priv->format != mpi->imgfmt) return VO_FALSE;
if(mpi->type == MP_IMGTYPE_STATIC || mpi->type == MP_IMGTYPE_TEMP) {
part of vo_sdl.
That's probably enough to make it work. Without IPB support it will be
hard to test though, you could try
mplayer -vo kva -dr -vf format=bgr32
> + if (mpkey == KEY_KP0 && usCh != '0')
> + mpkey = KEY_KPINS;
> +
> + if (mpkey == KEY_KPDEC && usCh != '.')
> + mpkey = KEY_KPDEL;
Please add a comment why they are here.
Are they to distinguish between numlock on/numlock off?
If so, what about KP1 - KP9?
(I am also asking because there are still some issue with supporting the
keypad on Windows, maybe your solutions work there, too).
> + if (m_int.kvac.ulInputFormatFlags & KVAF_YV12) {
> + m_int.fcc = FOURCC_YV12;
> + dstFormat = IMGFMT_YV12;
> + } else if (m_int.kvac.ulInputFormatFlags & KVAF_YUY2) {
> + m_int.fcc = FOURCC_Y422;
> + dstFormat = IMGFMT_YUY2;
> + } else if (m_int.kvac.ulInputFormatFlags & KVAF_YVU9) {
> + m_int.fcc = FOURCC_YVU9;
> + dstFormat = IMGFMT_YVU9;
> + } else if (m_int.kvac.ulInputFormatFlags & KVAF_BGR24) {
> + m_int.fcc = FOURCC_BGR3;
> + dstFormat = IMGFMT_BGR24;
> + } else if (m_int.kvac.ulInputFormatFlags & KVAF_BGR16) {
> + m_int.fcc = FOURCC_R565;
> + dstFormat = IMGFMT_BGR16;
> + } else if (m_int.kvac.ulInputFormatFlags & KVAF_BGR15) {
> + m_int.fcc = FOURCC_R555;
> + dstFormat = IMGFMT_BGR15;
> + }
you could use query_format_info to get the fcc from the IMGFMT...
> +static uint32_t draw_image(mp_image_t *mpi)
> +{
> + // if -dr or -slices then do nothing:
> + if (mpi->flags & (MP_IMGFLAG_DIRECT | MP_IMGFLAG_DRAW_CALLBACK))
> + return VO_TRUE;
> +
> + if (mpi->flags & MP_IMGFLAG_PLANAR)
> + draw_slice(mpi->planes, mpi->stride, mpi->w, mpi->h, mpi->x, mpi->y);
> + else
> + mem2agpcpy_pic(m_int.planes[0], mpi->planes[0],
> + SRC_WIDTH * m_int.bpp, SRC_HEIGHT,
> + m_int.stride[0], mpi->stride[0]);
Since you fixed draw_slice, you can now always call draw_slice I
think...
> +static int draw_slice(uint8_t *src[], int stride[], int w, int h, int x, int y)
> +{
> + uint8_t *s;
> + uint8_t *d;
> +
> + // copy packed or Y
> + d = m_int.planes[0] + m_int.stride[0] * y + x;
> + s = src[0];
> + mem2agpcpy_pic(d, s, w, h, m_int.stride[0], stride[0]);
Almost right, you must use something like w*m_int.bpp for it to work for
packed formats.
> +.IPs snap
> +Use SNAP mode.
> +.IPs wo
> +Use WarpOverlay! mode.
> +.IPs dive
> +Use DIVE mode.
"Force" is better that "Use" here, since if you do not specify these it
will use "auto" mode, which will probably end up using one of these, too...
I think it is ok apart from these issues...
Greetings,
Reimar Döffinger
More information about the MPlayer-dev-eng
mailing list