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

[Ivan Kalvachev]

On 11/18/08, Georgi Petrov <gogothebee at gmail.com> wrote:
>> As far as I am concerned you could leave panscan for later...
>
> Too late :) I wrote 90% of the panscan code.
>
[...]
>
> I attach the improved patch.

Just few nitpickings.

1. The w32_common documentation part should be separate from the vo_direct3d patch and probably committed right away.

2. in w32_common I see that there is two changes of function prototype.
vo_w32_border() and vo_w32_uninit(). This is not acceptable.
In some (standard) C flavors function() is equivalent of function(int).

3. There is something wrong in your draw_slice() implementation.
It looks like you are calling IDirect3DDevice9_StretchRect() for the whole image on each function invocation. But draw_slice() is not used only by D3DRenderFrame(), it is also used to update the screen on every 16 rendered lines. In short for 480p video you call it 30 times more than necessary. 
In theory the adapter may delay the stretch until it is exposition time, and probably this is what happens to you. But it may not be true for the above 90% cpu bugreport. Slice drawing could be disable with -noslices options

4. You have to put "static" in front of function names, unless that function is going to be called from outside the .c file. This keyword says to the compiler that this function is only used inside this file and doesn't export its symbols. 
This is how we have control(), query_format() in each  vo_* drivers and they don't clash at linking. The only exported function in vo_*drivers is the one defined by the LIBVO_EXTERN(), it sets callbacks to the API functions inside the driver.

5. I don't like naming your functions  D3D<something>. The D3D is used by Microsoft (e.g. D3DFORMAT) and we can't say with single look if this is API function or something you wrote. The possibility of name clash is also bigger than zero, even if your functions are static.

You may notice that all function (and variables) in mplayer are in lowercase, with "_" separator between words. It would be welcome if you follow that style too.
Also, I don't like the use of gVariable, I would recommend you to make single struct containing all local to the driver variables and use it like this 
   priv->SrcWidth; priv->DesktopFmt; //or like this:
   priv->src_width; priv->desktop_fmt;
We use "priv" in video filter system, so it would be consistent and would require less changes when/if we start updating the vo.

You are not obligated to comply with #5.