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

[Georgi Petrov]

You have some very good points...

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

It got committed already - everything - the driver and the changes to
w32_common.c...

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

I'm sure Reimar wanted to correct the prototypes - they are supposed
to be (void).

> 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

You are right. However, I never really understood one thing:

1. We have a draw_slices() libvo callback, which as you said updates
the screen with the rendered content every 16 lines. I thought it's on
every line though. I never realized that StretchRect is called so
often. This is huge overkill. BUT - it's overkill for for every 16
lines (even if it copies only them) as well!!!

2. We have a VOCTRL_DRAW_IMAGE in "control", which passes mpi
structure with whole frame already rendered.

This is stupid. Or it seems so. This way we have two "paths" for
rendering. I never really understood when is VOCTRL_DRAW_IMAGE used
for the whole frame and when draw_slices() for a 16 by 16 lines... On
my test images, draw_slices() never called directly.

Please somebody explain to me WHY are there those two draw paths and
exactly when is the first and when the second one used? As far as I
understand, this question is valid only for planar modes. In packed
mode, only VOCTRL_DRAW_IMAGE is used, right?

StretchRect is expensive. I don't want to call it each 16 lines even
if I update only 16 lines on the destination (with RECT given as a
parameter to StretchRect). I would want to delay StretchRect until
Present and call it in flip_page() then...

So in conclusion - I the two StretchRects - the one from draw_slice()
and from D3DRender() - to flip_page() just before Present, right?

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

I'll do it.

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

Ok, I'll do it.

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

Is this REALLY the coding style in the whole MPlayer? If so, I'll
change mines as well, no problem.

> 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;

Funny - it was just like that when Diego wanted from me to drop
gConfig struct, which was a container for all globals. I did it. Now
you want me to introduce globals  container again. Please - you both
decide which is preferable, because I will do it only once again.

> We use "priv" in video filter system, so it would be consistent and would require less changes when/if we start updating the vo.

Sounds ok to me. Diego?