[MPlayer-dev-eng] [PATCH] vo_kva
KO Myung-Hun
komh at chollian.net
Sun Mar 8 15:12:13 CET 2009
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.
--
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
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: kva.diff
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090308/f43631a2/attachment.asc>
More information about the MPlayer-dev-eng
mailing list