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

[Diego Biurrun]

On Mon, Nov 17, 2008 at 12:10:58PM +0200, Georgi Petrov wrote:
> 
> That's it, I attach the updated patch.

Let's see...

> --- AUTHORS	(revision 27943)
> +++ AUTHORS	(working copy)

OK

> --- libvo/video_out.c	(revision 27943)
> +++ libvo/video_out.c	(working copy)

OK

> --- configure	(revision 27943)
> +++ configure	(working copy)

OK
> --- DOCS/tech/MAINTAINERS	(revision 27943)
> +++ DOCS/tech/MAINTAINERS	(working copy)

OK

> --- libvo/vo_direct3d.c	(revision 0)
> +++ libvo/vo_direct3d.c	(revision 0)
> @@ -0,0 +1,682 @@
> +
> +/** @brief Calculate scaled fullscreen movie rectangle with
> + *  preserved aspect ratio.
> + *  @return TRUE on success, FALSE on failure
> + */
> +void CalculateFullscreenRect (void)

A void function returning a value?

> +/** @brief Destroy D3D Context related to the current window.
> + *  @return TRUE on success, FALSE on failure
> + */
> +void D3DDestroyContext (void)

ditto

> +/** @brief (Re)Initialize Direct3D. Kill and recreate context.
> + *  @return TRUE on success, FALSE on failure
> + *  The first function called to init D3D context.
> + */

This TRUE/FALSE thing feels very strange and not C-like.

> +/** @brief Uninitialize Direct3D and close the window.
> + *  @return N/A
> + */
> +void D3DUninit(void)

I don't think you need to fill out @return for void functions, same
below.

> +/** @brief Render a frame on the screen.
> + *  @param mpi  Mpi structure with the decoded frame inside

lowercase mpi

> +/** @brief Query if movie's colorspace is supported by the HW.

the movie colorspace

> +/** @brief libvo Callback: Handle control requests.
> + *  @return VO_TRUE on success, VO_NOTIMPL when not
> + *  implemented

nit: Join these two lines.

> + *  @param title    Windows title

"window title"?  I don't think you are talking about multiple windows..

> + *  @param format   Movie colorspace format (using Mplayer's
> + *                  defines, e.g. IMGFMT_YUY2)

You are still not allowed to misspell MPlayer.

> +static void uninit(void)
> +{
> +    /* Release everything related to D3D, */
> +    D3DUninit();
> +
> +    /* w32_common framework call. Uninitialize all window-handling specific
> +     * stuff.
> +     */
> +    vo_w32_uninit();

Those two comments look pretty redundant.

> +static void check_events(void)
> +{
> +    int Flags;

I think capitalized variable names are ugly, same in many other places.

> --- libvo/w32_common.c	(revision 27943)
> +++ libvo/w32_common.c	(working copy)
> @@ -149,6 +149,24 @@
>  
> +/** @brief Dispatch incoming window events and handle them.
> + *
> + * This function should be placed inside libvo's function "check_events".
> + *
> + *  Global libvo variables changed:
> + *  vo_dwidth:  New window client area width
> + *  vo_dheight: New window client area height

Lowercase after the period.

> + *  VO_EVENT_RESIZE = The window was resized. Please reinit yout driver's

your

> @@ -332,6 +362,17 @@
>  
> +/** @Brief Configure and create window on the screen.
> + *
> + *  This function should be called in libvo's "config" callback.
> + *  It configures a window and shows in on the screen.

it on

> @@ -347,6 +388,22 @@
>  
> +/** @brief Initialize w32_common framework.
> + *
> + * The first function that should be called from the w32_common framework.
> + * It handles window creation on the screen with proper title and attributes.
> + * It also initializes framework's internal variables. The function should be

the framework's

> @@ -422,7 +511,15 @@
>  
> + * Should be called last in video driver's uninit function. First release
> + * anything build ontop of the created window e.g. rendering context inside

built on top

Diego