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

[Georgi Petrov]

>> +    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.

This is part of the new coding style in the company I work for. I
didn't like it initially and just as you suggested, it seems stupid,
but it has 2 advantages:

1. if you mistype the double == as a single one like:
if (FALSE = D3DRecreateContext())

the compiler will scream at you.

2. If you have a long line, you may not see the end or if you scroll
to the end, and this way it's easier to see if you're comparing a
failure of the function or success. We don't use 80 character
limitation, so on long lines, you don't have to scroll that much.
Before we had to scroll just to see if we're checking GOOD or BAD
condition.

>
>> +        /* 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.
>

Hmm, yes it does.. I'll update it. Isn't it faster that way instead of
function call each time? After all this is called each frame...


>> +        pScanlineBegin = (char *) stLockedRect.pBits;
>
> pBits is void * according to my documentation, so that (char *) is
> not necessary.
>

Ok. Does this casting slow down a litle bit?

>> +    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).
>

Yes, this was left in case I decided to implement error handling
(reinitialize D3D). May be the handler could sit in Present's failure
test. Such state may occur for example in case of standby/resume or
something like that. It will be cool if I reinitialize and just
continue playing :)

>> +    /* 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.
>

You are genius, I will!

>> +    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).
>

Huh... I understand. So - in DRAW_IMAGE I should just copy the frame
to the OffscreenSurface and the Present should happen in flip_page,
right? It's like DRAW_IMAGE is called parts of the second before
flip_page, so the driver has time to copy the frame before flipping?

>> +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?
>

I know about the table - vo_directx.c is like that. I thought it may
complicate the things by adding another global variable. I hate global
variables. Unfortunately libvo's logic is based around them... For
example many callback functions are called without parameters,
counting that the driver has saved them all. It's faster this way, I
know, but I don't like when I lose proximity. Do you really think I
should use a table?

>> +    /* 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.
>

Hah!!! This is great. I will! I will implement subtitles later, of
course. Does vf_expand handle this by default (does it get into the
filter chain), or I should add it like -vf expand?

>> +    /* 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".
>

Ok, you have it :)

>> +
>> +    /* 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.
>

Yes, I'll implement it when I want some options. Thanks.

>> +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.
>

Hmmm, it depends. What is I decide to do something else afterward and
forget to return something... I always think that the right "case
framework" is:

case xxxxxx:
    some code...
break;

Imagine that the next "some code..." doesn't contain a break... That's
why in C# for example the default pass-through policy of C is left
behind and by default new case operator breakes the previous one. Many
bugs arrive exactly from switch case pass-through.

>> +    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.
>

I will correct it.

>> +/** @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.

I don't understand that. With both YV12/YUY2 colorspaces this function
never gets called. What's the difference between it and DRAW_FRAME???
And when (mpi->flags & MP_IMGFLAG_DRAW_CALLBACK) is true and when
false?

>
> Greetings,
> Reimar Döffinger

Thanks for pointing out those things. I'll try to get them fixed as
soon as possible. But first - should I fix them first or the patch
will get accepted? And what will happen after the patch is accepted?
Am I going to post further modifications to vo_direct3d.c here ot I'll
have read-write access to commit whenever I want? I just want to know
in order to prepare "commit technique".

Greetings from me as well!