[MPlayer-dev-eng] [PATCH] vo_kva
KO Myung-Hun
komh at chollian.net
Sat Mar 14 13:46:44 CET 2009
Hi/2.
KO Myung-Hun wrote:
> Hi/2.
>
> Reimar Döffinger wrote:
>>>>> + 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.
>>
>
> Of course.
>
>> 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.
>>
>>
>
> But what you missed is that the frame window procedure is subclassed
> only if fixt23 is enabled.
>
>>>>> + // 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.
>>
>
> d&d has not been tested.
>
>> 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.
>>
>>
>
> I think, it's WinEnableWindow(). But with quick test, a disabled
> window does not pass any event to other window. Anyway if I find the
> solution using this approach, I'll submit another patch later.
>
>>>>> + 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.
>>
>>
>
> I think, if needed, OS policy can be overrided by user. Many enhancers
> do this.
>
> And in case of FFplay, it has a feature to seek to the clicked
> position. If I switched to other window and click to return to
> FFplay, seeking is performed as well as focus changing. It's very
> bothered.
>
> Of course, MPlayer is not FFplay. ^^
>
> I'll keep this way.
>
>>>>> + 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).
>>
>>
>
> First of all, it's very out of my expectation that you allow flickering.
>
> And we should consider colorkey as well. So unless we distinguish
> movie area and black-bar area, movie is not overlaid correctly.
>
>>>>> + 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
>>> }
>>>
>>
>>
>
> Ok.
>
>>>>> + 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?
>>
>>
>
> No.
>
>>>>> +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.
>>
>
> Yes, it's the part I ignored.
>
>> 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
>>
>>
>
> BTW, '-vf format=bgr32' must be specified ?
>
>>> + 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.
>>
>
> Added.
>
>> Are they to distinguish between numlock on/numlock off?
>>
>
> No. It's just to support KEY_KPINS and KEY_KPDEL because they are
> listed in osdep/keycodes.h
>
>> 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 KEY_KPHOME, KEY_KPEND, and so on are there, I'll distinguish them.
> But MPlayer does not support them.
>
>>> + 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...
>>
>>
>
> Do you mean to get m_int.fcc from dstFormat ? If so, fixed.
>
>>> +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...
>>
>>
>
> Ok.
>
>>> +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.
>>
>>
>
> Fixed.
>
>>> +.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...
>>
>>
>
> Ok.
No more comments ?
--
KO Myung-Hun
Using Mozilla SeaMonkey 1.1.14
Under OS/2 Warp 4 for Korean with FixPak #15
On AMD ThunderBird 1 GHz with 512 MB RAM
Korean OS/2 User Community : http://www.ecomstation.co.kr
More information about the MPlayer-dev-eng
mailing list