[MPlayer-dev-eng] [PATCH] vo_kva
Reimar Döffinger
Reimar.Doeffinger at gmx.de
Sat Mar 7 15:36:10 CET 2009
On Sun, Mar 01, 2009 at 11:09:49PM +0900, KO Myung-Hun wrote:
> +static vo_info_t info = {
Should be const now.
> +#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.
> +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).
> +static void imgCreate(void)
> +{
> + int size;
> + int n;
> +
> + size = SRC_HEIGHT * m_int.lStride;
You could merge the declaration and initialization.
> + // 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
> +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.
> + 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.
> + 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.
> + 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.
> + // 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.
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 (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).
> + 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.
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...
> + 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.
> + opt_t subopts[] = {
If you're at it anyway: you can make that one const now...
> + PSZ pszVideoModeStr[3] = {"DIVE", "WarpOverlay!", "SNAP"};
const
> + 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
> + 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, but if the user explicitly specified it so, why add extra
code to second guess them?
> + if (!d_width)
> + d_width = width;
> +
> + if (!d_height)
> + d_height = height;
This should not be necessary.
> +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?).
In particular, since there is only one buffer, you at least can not provide
MP_IMGTYPE_IP or MP_IMGTYPE_IPB
> +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.
> + 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?
> + case VOCTRL_QUERY_FORMAT:
> + return query_format(*((uint32_t *)data));
One () more than necessary.
> +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
> +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...
More information about the MPlayer-dev-eng
mailing list