[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