[MPlayer-dev-eng] [PATCH] vo_kva
KO Myung-Hun
komh at chollian.net
Sun Mar 8 11:23:01 CET 2009
Hi/2.
Reimar Döffinger wrote:
> On Sun, Mar 01, 2009 at 11:09:49PM +0900, KO Myung-Hun wrote:
>
>> +static vo_info_t info = {
>>
>
> Should be const now.
>
>
Fixed.
>> +#define SET_ASPECT_RATIO(ratio) {\
>> + m_int.kvas.ulRatio = (ratio);\
>> + kvaSetup(&m_int.kvas);\
>> +}
>>
>
> Make it a function (you only need one argument more) and you can avoid
> all the syntactic issues that such a macro has.
>
>
Ok.
>> +struct tagVOKVAINTERNAL {
>> + HAB hab;
>> + HMQ hmq;
>> + HWND hwndFrame;
>> + HWND hwndClient;
>> + HWND hwndSysMenu;
>> + HWND hwndTitleBar;
>> + HWND hwndMinMax;
>> + FOURCC fcc;
>> + INT iImageFormat;
>> + KVASETUP kvas;
>> + KVACAPS kvac;
>> + RECTL rclDst;
>> + INT bpp;
>> + LONG lStride;
>> + PBYTE pbImage;
>> + BOOL fFixT23;
>> + PFNWP pfnwpOldFrame;
>> + uint8_t *planes[3]; // y = 0, u = 1, v = 2
>> + int stride[3];
>> + BOOL fHWAccel;
>> + RECTL rclParent;
>> + struct SwsContext *sws;
>> +};
>> +
>> +static struct tagVOKVAINTERNAL m_int;
>>
>
> you could merge the type and the variable declaration. There is also no
> real need to name the struct since its type is not used anywhere (yet).
>
>
Ok.
>> +static void imgCreate(void)
>> +{
>> + int size;
>> + int n;
>> +
>> + size = SRC_HEIGHT * m_int.lStride;
>>
>
> You could merge the declaration and initialization.
>
>
Ok.
>> + // YV12 or YVU9 ?
>> + if (n) {
>> + m_int.planes[1] = m_int.planes[0] + SRC_HEIGHT * m_int.stride[0];
>> + m_int.stride[1] = m_int.stride[0] / n;
>>
>
> usually "n" is called "chroma_shift" and then m_int.stride[1] = m_int.stride[0] >> chroma_shift;
> Given how often you use it, it might make sense to store it in m_int
>
>
Ok.
>> +static void imgFree(void)
>> +{
>> + if (m_int.pbImage)
>> + free(m_int.pbImage);
>>
>
> The "if" is not necessary, remove it unless you think it has some
> documentation value.
>
>
Fixed.
>> + uint8_t *dst[3] = {NULL,};
>> + int dstStride[3] = {0,};
>>
>
> Then ',' are not necessary, and actually the whole initialization to
> 0/NULL is a bit pointless.
>
>
Ok.
>> + sws_scale(m_int.sws, m_int.planes, m_int.stride, 0, SRC_HEIGHT,
>> + dst, dstStride);
>> +
>> + if (gCpuCaps.hasMMX)
>> + asm volatile ("emms\n\t");
>>
>
> The emms should not (no longer?) be necessary.
>
>
Ok. I also want that.
>> + 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.
>> + // 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 ?
> 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.
>
>> + if (fsFlags & KC_SCANCODE) {
>> + switch (uchScan) {
>>
> [...]
>
>> + if (fsFlags & KC_VIRTUALKEY) {
>> + switch (usVk) {
>>
>
> Please use lookup_keymap_table for these, it greatly reduced the amount
> of code (even if the code is quite trivial).
> You might even consider sharing and reusing the translation tables from
> w32_common.c (or if they are not suitable copy-paste-modify to save you
> some time).
>
>
Ok. Thanks for the info.
>> + 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
> Also you should not duplicate the code, you could maybe do something
> like
> case WM_BUTTON1DOWN:
> case WM_BUTTON2DOWN:
> case WM_BUTTON3DOWN:
> case WM_BUTTON1DBLCLK:
> case WM_BUTTON2DBLCLK:
> case WM_BUTTON3DBLCLK:
> if (WinQueryFocus(HWND_DESKTOP) != hwnd) {
> WinSetFocus(HWND_DESKTOP, hwnd);
> return TRUE;
> }
> if (!vo_nomouse_input)
> mplayer_put_key(lookup_keymap_table(mousemap, ...);
>
> Feel free to improve on that suggestion...
>
>
Ok.
>> + 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.
>> + opt_t subopts[] = {
>>
>
> If you're at it anyway: you can make that one const now...
>
>
Anyway, const. Ok.
>> + PSZ pszVideoModeStr[3] = {"DIVE", "WarpOverlay!", "SNAP"};
>>
>
> const
>
>
Ok.
>> + 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 ?
>> + 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.
> but if the user explicitly specified it so, why add extra
> code to second guess them?
>
>
What is 'extra code' ?
>> + if (!d_width)
>> + d_width = width;
>> +
>> + if (!d_height)
>> + d_height = height;
>>
>
> This should not be necessary.
>
>
Ok.
>> +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.
How should I do to make it work ?
>> +static uint32_t put_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
>> + fast_memcpy(m_int.planes[0], mpi->planes[0], SRC_HEIGHT * m_int.stride[0]);
>>
>
> That's not going to work if mpi->stride[0] != m_int.stride[0]. In
> particular it does not handle the negative-stride case (bottom-first
> data) that is very common for raw RGB in AVI.
>
>
I think, fixed.
>> + switch (format) {
>> + case IMGFMT_YV12:
>> + fHWAccel = m_int.kvac.ulInputFormatFlags & KVAF_YV12;
>> + break;
>> +
>> + case IMGFMT_YUY2:
>> + fHWAccel = m_int.kvac.ulInputFormatFlags & KVAF_YUY2;
>> + break;
>> +
>> + case IMGFMT_YVU9:
>> + fHWAccel = m_int.kvac.ulInputFormatFlags & KVAF_YVU9;
>> + break;
>> +
>> + case IMGFMT_BGR24:
>> + fHWAccel = m_int.kvac.ulInputFormatFlags & KVAF_BGR24;
>> + break;
>> +
>> + case IMGFMT_BGR16:
>> + fHWAccel = m_int.kvac.ulInputFormatFlags & KVAF_BGR16;
>> + break;
>> +
>> + case IMGFMT_BGR15:
>> + fHWAccel = m_int.kvac.ulInputFormatFlags & KVAF_BGR15;
>> + break;
>> +
>> + default:
>> + return 0;
>> + }
>>
>
> I think I've seen that code twice now. Maybe it should be a function?
>
>
Ok.
>> + case VOCTRL_QUERY_FORMAT:
>> + return query_format(*((uint32_t *)data));
>>
>
> One () more than necessary.
>
>
Fixed.
>> +static int draw_frame(uint8_t *src[])
>> +{
>> + fast_memcpy(m_int.planes[0], src[0], SRC_HEIGHT * m_int.stride[0]);
>> +
>> + return 0;
>> +}
>>
>
> You don't actually need to implement this one if you provide draw_image
>
>
Ok.
>> +static int draw_slice(uint8_t *src[], int stride[], int w, int h, int x, int y)
>> +{
>> + uint8_t *s;
>> + uint8_t *d;
>> + int n;
>> +
>> + n = m_int.iImageFormat == IMGFMT_YV12 ? 2 : 4;
>> +
>> + // copy 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]);
>> +
>> + w /= n; h /= n; x /= n; y /= n;
>> +
>> + // copy U
>> + d = m_int.planes[1] + m_int.stride[1] * y + x;
>> + s = src[1];
>> + mem2agpcpy_pic(d, s, w, h, m_int.stride[1], stride[1]);
>> +
>> + // copy V
>> + d = m_int.planes[2] + m_int.stride[2] * y + x;
>> + s = src[2];
>> + mem2agpcpy_pic(d, s, w, h, m_int.stride[2], stride[2]);
>> +
>> + return 0;
>> +}
>>
>
> draw_slice could also be called for RGB/BGR videos...
>
I think, fixed.
--
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/dc6ba0f0/attachment.txt>
More information about the MPlayer-dev-eng
mailing list