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

[Reimar Döffinger]

On Sun, Nov 16, 2008 at 10:04:10PM +0200, Georgi Petrov wrote:
> +BOOL gIsPaused;              /**< TRUE  = Movie is paused,
> +                                  FALSE = Movie is not paused */

I'll just mention it once: that whole BOOL TRUE/FALSE stuff is very much
against the concept of C, there is a clear "BOOL" type called int, and 0
is false and everything else is true.
I can't help but feel reminded of that good old Java joke of doing
"if (((a == b) == true) == false)"

> +/** @brief Calculate scaled fullscreen movie rectangle with
> + *  preserved aspect ratio.
> + *  @return TRUE on success, FALSE on failure
> + */
> +void CalculateFullscreenRect (void)
> +{
> +    int ScaledHeight = 0;
> +    int ScaledWidth = 0;
> +    /* If we've created fullscreen context, we should calculate stretched
> +    * movie's RECT, otherwise it will fill the whole fullscreen with
> +    * wrong aspect ratio */
> +
> +    if (gSrcWidth > gSrcHeight)
> +    {
> +        ScaledHeight = (int) ((float) vo_dwidth / gAspectRatio);
> +        gFullScrMovieRect.left = 0;
> +        gFullScrMovieRect.right = vo_dwidth;
> +        gFullScrMovieRect.top =
> +            ((vo_screenheight - ScaledHeight) / 2);
> +        gFullScrMovieRect.bottom =
> +            gFullScrMovieRect.top + ScaledHeight;
> +    } else if (gSrcHeight > gSrcWidth)
> +    {
> +        ScaledWidth = (int) ((float) vo_dheight * gAspectRatio);
> +        gFullScrMovieRect.top = 0;
> +        gFullScrMovieRect.bottom = vo_dheight;
> +        gFullScrMovieRect.left =
> +            ((vo_screenwidth - ScaledWidth) / 2);
> +        gFullScrMovieRect.right =
> +            gFullScrMovieRect.left + ScaledWidth;
> +
> +    } else /* Perfect square */
> +    {
> +        gFullScrMovieRect.top = 0;
> +        gFullScrMovieRect.left = 0;
> +        gFullScrMovieRect.right = vo_dheight;
> +        gFullScrMovieRect.bottom = vo_dwidth;
> +    }
> +    mp_msg(MSGT_VO,MSGL_V,
> +    "<vo_direct3d>Fullscreen Movie Rect: t: %ld, l: %ld, r: %ld, b:%ld\r\n",
> +        gFullScrMovieRect.top, gFullScrMovieRect.left,
> +        gFullScrMovieRect.right, gFullScrMovieRect.bottom);

This supports only a small part of what MPlayer is supposed to handle.
Code adapted from vo_gl.c:
  if (vo_fs) {
    int new_w, new_h;
    GLdouble scale_x, scale_y;
    aspect(&new_w, &new_h, A_ZOOM);
    panscan_calc();
    new_w += vo_panscan_x;
    new_h += vo_panscan_y;
    gFullScrMovieRect.left = (vo_screenwidth - new_w) / 2;
    gFullScrMovieRect.right = gFullScrMovieRect.left + new_w;
    gFullScrMovieRect.top = (vo_screenheight - new_h) / 2;
    gFullScrMovieRect.bottom = gFullScrMovieRect.top + new_h;
  }
Note: for proper panscan support this assumes that StretchRect can
handle left/top to be negative and right/bottom larger than the
destination.
The documentation is useless as always, the people writing MSDN stuff have never
realized the concept of "corner cases" and the usefulness of documenting
them...
Anyways, to test that corner case use "-fs -panscanrange -2" and use the
w/e keys to zoom in/out. -vo gl should probably behave correctly.


> +static uint32_t D3DRenderFrame (mp_image_t *mpi)
> +{
> +    D3DLOCKED_RECT  stLockedRect;   /**< Offscreen surface we lock in order
> +                                         to copy MPlayer's frame inside it.*/
> +
> +    if((mpi->flags & MP_IMGFLAG_DIRECT) ||
> +       (mpi->flags & MP_IMGFLAG_DRAW_CALLBACK))
> +        return VO_TRUE;

The MP_IMGFLAG_DIRECT is no good here, firstly as long as you do not
implement direct rendering it can never happen anyway, and if you do
implement it the code here basically would be the same as in the
"normal" case except you skip the memcpy stuff (since the decoder would
have written directly into stLockedRect.pBits itself).

> +    case VOCTRL_FULLSCREEN:
> +        vo_w32_fullscreen();
> +        D3DConfigure();


> +static void check_events(void)
> +{
> +    int Flags;
> +    /* w32_common framework call. Handles video window events.
> +     * Updates global libvo's vo_dwidth/vo_dheight upon resize
> +     * with the new window width/height. I copy them to gConfig
> +     */
> +    Flags = vo_w32_check_events();
> +    if (Flags & VO_EVENT_RESIZE)
> +        D3DConfigure();

Not sure how slow it is nowadays, but I'd guess it would be nice if the
D3D context wouldn't be reinitialize just because the window was
resized (I hope I understood the code right that you actually do that).
E.g. with -vo gl you can resize the window while the video is paused and
it will immediately display the correctly scaled image.

> +    /* Copy U. Casting to char * is here to suppress a compiler warning. */
> +    Dst = (char *)stLockedRect.pBits + stLockedRect.Pitch * gSrcHeight
> +          + UVstride * y + x;

Personally I wouldn't comment on that cast, but just to make it clear:
stLockedRect.pBits is "void *" and doing pointer arithmetic with that is
_wrong_, so the cast is necessary to even have valid C (I still consider
it bad that the compiler only warns instead of refusing to compile it).

> +w32_common author: Reimar Doeffinger <reimar.doeffinger at stud.uni-karlsruhe.de>

I am not the author, just the maintainer.

> +Known libvo drivers which use this code: vo_gl.c, vo_direct3d.c

vo_gl2.c, too.

Note I did not review all of the patch, just the obvious things that can be
improved.

Greetings,
Reimar Döffinger