[MPlayer-dev-eng] [PATCH] Direct3D libvo driver

[Reimar Döffinger]

Hello,
On Thu, Nov 13, 2008 at 03:59:10PM +0200, Georgi Petrov wrote:
> > Not quite, your patch still shows e.g. the --enable-bl line that has
> > nothing to do with anything you changed. Maybe you used an editor that
> > changes all tabs to spaces or vice versa?
> > Your diff should only contain the minimum number of changes to get the
> > new functionality.
> 
> Is it ok now? I attach the "new version" :)

Yes, I think that's what Diego wanted. If you can do it easily I'd
appreciate if you could test the content type of the attachment to e.g.
text/plain (makes it easier to quote from it).

> +    if (TRUE == gConfig.IsD3DInitialized)
> +        return TRUE;
> +
> +    if (FALSE == D3DRecreateContext())
> +        return FALSE;

Hm, do you actually like having the TRUE/FALSE in front in the
conditions? To me this always reads like Yoda-speak (first the attribute,
then the thing like in "Blue the house is", or nor precisely "Fail the
D3DRecreateContext did" instead of "D3DRecreateContext failed" :-) ).
Note: not asking you to change it, just curious.

> +        /* Copy Y */
> +        for (i = 0; i < mpi->height; i++)
> +        {
> +            fast_memcpy (pScanlineBegin, mpi->planes[0] +
> +                         (i * mpi->stride[0]), mpi->stride[0]);
> +            pScanlineBegin += stLockedRect.Pitch;
> +        }

memcpy_pic should do all of that.

> +        pScanlineBegin = (char *) stLockedRect.pBits;

pBits is void * according to my documentation, so that (char *) is
not necessary.

> +    if (FAILED (IDirect3DDevice9_TestCooperativeLevel(gConfig.pD3DDevice)))
> +    {
> +        mp_msg(MSGT_VO,MSGL_ERR,"<vo_d3d>Video adapter is uncooperative\n");
> +        return VO_ERROR;
> +    }

As I understand the documentation, Present will always fail when this is
wrong, so it seems a bit pointless to check this (if later you need to
differentiate the cases you can still call this when Present failed).

> +    /* FIXME: D3DTEXF_NONE or D3DTEXF_LINEAR */

D3DTEXF_GAUSSIANQUAD of course :-P

> +    if (TRUE == gConfig.IsFullScreen)
> +    {
> +        if (FAILED (IDirect3DDevice9_StretchRect (gConfig.pD3DDevice,
> +                                                  gConfig.pD3DSurface,NULL,
> +                                                  gConfig.pD3DBackBuf,
> +                                                  &gConfig.FullScrMovieRect,
> +                                                  D3DTEXF_NONE)))
> +        {
> +            mp_msg(MSGT_VO,MSGL_ERR,
> +            "<vo_d3d>Unable to copy the frame to the back buffer\n");
> +            return VO_ERROR;
> +        }
> +    }
> +    else
> +    {
> +        if (FAILED (IDirect3DDevice9_StretchRect (gConfig.pD3DDevice,
> +                                                  gConfig.pD3DSurface, NULL,
> +                                                  gConfig.pD3DBackBuf, NULL,
> +                                                  D3DTEXF_NONE)))
> +        {
> +            mp_msg(MSGT_VO,MSGL_ERR,
> +            "<vo_d3d>Unable to copy the frame to the back buffer\n");
> +            return VO_ERROR;
> +        }
> +    }

Code duplication. Just use
"TRUE == gConfig.IsFullScreen ?  &gConfig.FullScrMovieRect : NULL"
as the fifth argument instead of the big if.

> +    if (FAILED (IDirect3DDevice9_Present (gConfig.pD3DDevice, 0, 0, 0, 0)))
> +    {
> +        mp_msg(MSGT_VO,MSGL_ERR,"<vo_d3d>Unable to present the frame\n");
> +        return VO_ERROR;
> +    }

As I understand it, that is wrong, DRAW_IMAGE can be called long before
it is actually time to draw the frame, actually displaying should only
happen in flip_page (in the main mplayer code the timing sleep is
in-between DRAW_IMAGE and flip_page).

> +static int query_format (uint32_t MovieFormat)
> +{
> +    char ColorspaceName[20];
> +
> +    switch (MovieFormat)
> +    {
> +    case IMGFMT_YV12:
> +        gConfig.MovieSrcFmt = MAKEFOURCC('Y','V','1','2');
> +        strncpy (ColorspaceName, "YV12 ", 20);

First, using a table to convert these might be nicer, esp. if even more
formats are added later.
Secondly, I really think you should add the RGB formats.
Lastly,
const char *ColorspaceName;
ColorspaceName = "YV12";
no need for strncpy, also why do you have a space at the end of some
format names?

> +    /* Test conversion from Movie colorspace to display's target colorspace*/
> +    if (FAILED (IDirect3D9_CheckDeviceFormatConversion (gConfig.pD3DHandle,
> +                                                        D3DADAPTER_DEFAULT,
> +                                                        D3DDEVTYPE_HAL,
> +                                                        gConfig.MovieSrcFmt,
> +                                                        gConfig.DesktopFmt)))
> +        return 0;
> +    else
> +    {
> +        mp_msg(MSGT_VO,MSGL_ERR,"<vo_d3d>Accepted Colorspace %s\n",
> +               ColorspaceName);
> +        return (VFCAP_CSP_SUPPORTED | VFCAP_CSP_SUPPORTED_BY_HW | VFCAP_OSD
> +                | VFCAP_HWSCALE_UP | VFCAP_HWSCALE_DOWN);

Would be better not to set VFCAP_OSD as long as you do not really
support it, that will allow vf_expand to draw the OSD.


> +    /* Clear gConfig, the global storage container. */
> +    ZeroMemory (&gConfig, sizeof(gConfig));

I'd prefer the standard C "memset(&gConfig, 0, sizeof(gConfig));"
instead of the Windows-specific "ZeroMemory".

> +
> +    /* Check user arguments "arg" just like this:"-vo d3d:arg"
> +       In the future more options will be added */
> +
> +    /* FIXME: Example only */
> +    if (arg != NULL)
> +    {
> +        if (strstr (arg,"Option1"))
> +        mp_msg(MSGT_VO,MSGL_V,"<vo_d3d>Option1 selected\n");
> +    }

Please use subopt-helper.h for this, see vo_gl.c:preinit for an example
of how to use it.

> +static int control(uint32_t request, void *data, ...)
> +{
> +    switch (request)
> +    {
> +    case VOCTRL_QUERY_FORMAT:
> +        return query_format (*(uint32_t*) data);
> +        break;

The breaks are pointless if you have a return. Since this is already so
much boilerplate code I'd appreciate it if you could remove them.

> +    case VOCTRL_BORDER:
> +        return VO_FALSE;
> +        break;
> +    }
> +    return VO_FALSE;

Here, and for all the stuff you did not implement (yet) VO_FALSE is
wrong.
"return VO_NOTIMPL;" is right.

> +/** @brief libvo Callback: Unused function
> + *  @return N/A
> + */
> +static int draw_slice(uint8_t *src[], int stride[], int w,int h,int x,int y )
> +{
> +    mp_msg(MSGT_VO,MSGL_ERR,"<vo_d3d>draw_slice called\n");
> +    return VO_ERROR;
> +}

This probably is wrong, I think draw_slice is almost always called for any YUV
data, and you must copy the data there since DRAW_FRAME only gets and
mpi with (mpi->flags & MP_IMGFLAG_DRAW_CALLBACK) and no actual image
data.

Greetings,
Reimar Döffinger