[vlc-devel] [PATCH] Direct2D video output module

Laurent Aimar fenrir at elivagar.org
Thu Jul 22 21:07:57 CEST 2010


Hi,

On Thu, Jul 22, 2010 at 05:26:25PM +0300, David Kaplan wrote:
> diff --git a/modules/video_output/msw/common.h b/modules/video_output/msw/common.h
> index e023ef4..db6a2d8 100644
> --- a/modules/video_output/msw/common.h
> +++ b/modules/video_output/msw/common.h
> @@ -157,6 +157,15 @@ struct vout_display_sys_t
>      vout_display_opengl_t vgl;
>  #endif
>  
> +#ifdef MODULE_NAME_IS_direct2d
> +    HINSTANCE              h_d2_dll;            /* handle of the opened d3d9 dll */
> +    FLOAT                  f_d2_dpi_x;                                  /* DPI x */
> +    FLOAT                  f_d2_dpi_y;                                  /* DPI y */
> +    ID2D1Factory           *p_d2_factory;                         /* D2D factory */
> +    ID2D1HwndRenderTarget  *p_d2_render_target;          /* D2D rendering target */
> +    ID2D1Bitmap            *p_d2_bitmap;                            /* D2 bitmap */
 No need for the p_/f_/h_ prefix. I plan to remove the remaining ones from
msw vouts someday.

> +#define GUID_EXT
> +DEFINE_GUID(IID_ID2D1Factory, 102048327, 28496, 18010, 146, 69, 17, 139, 253, 59, 96, 7);
 Using hexa seems to be far more common (unless msdn used decimal here?).

> +void D2D_CreateRenderTarget(vout_display_t *vd);
> +void D2D_ResizeRenderTarget(vout_display_t *vd);
> +void D2D_DestroyRenderTarget(vout_display_t *vd);
 If they are local functions, using static is better. You could also not use
the D2D_ prefix.

> +/**
> + * Initialises Direct2D vout module
> + */
> +static int Open(vlc_object_t *object)
> +{
> +    vout_display_t *vd = (vout_display_t *)object;
> +    vout_display_sys_t *sys;
> +
> +    vd->sys = sys = calloc(1, sizeof(*sys));
> +    if (!sys)
> +        return VLC_ENOMEM;
> +
> +    if (CommonInit(vd))
> +        goto error;
 I think that calling CommonInit after having loaded the dll and retreived the
function D2D1CreateFactory is better. It's probably the easiest way to avoid
too much initialization in case of failures.

> +    sys->h_d2_dll = LoadLibrary(TEXT("D2D1.DLL"));
> +    if (!sys->h_d2_dll) {
> +        if (object->b_force)
> +            msg_Err(vd, "Cannot load D2D1.DLL, aborting");
> +        goto error;
> +    }
> +
> +    D2D1_FACTORY_OPTIONS fo = {
> +        D2D1_DEBUG_LEVEL_NONE
> +    };
> +
> +    HRESULT (WINAPI *_D2D1CreateFactory)( D2D1_FACTORY_TYPE, REFIID,
> +        const D2D1_FACTORY_OPTIONS *, void ** );
> +
> +    _D2D1CreateFactory = (void *)GetProcAddress(sys->h_d2_dll,
> +        TEXT("D2D1CreateFactory"));
 Vertical alignements for function arguments is way easier to read. It applies
to the whole file. Something like
_D2D1CreateFactory = (void *)GetProcAddress(sys->h_d2_dll,
                                            TEXT("D2D1CreateFactory"));
(if you use a fixed font for mails).

 Also, avoiding using _ prefix is prefered.

> +    if (!_D2D1CreateFactory) {
> +        if (object->b_force)
> +            msg_Err(vd,
> +                "Cannot locate reference to a D2D1CreateFactory ABI in D2D1.DLL");
 Dunno if the check on b_force is actually needed here, a d2d1.dll without this
function is probably broken, and so warning about it doesn't seem to be a bad idea
to me.
> +        goto error;
> +    }
> +
> +#ifndef NDEBUG
> +    msg_Dbg(vd, "D2D1.DLL loaded");
> +#endif
> +
> +    HRESULT hr = _D2D1CreateFactory(D2D1_FACTORY_TYPE_SINGLE_THREADED,
> +        (REFIID)&IID_ID2D1Factory, &fo, (void **)&sys->p_d2_factory);
> +    if (hr != S_OK) {
> +        if (object->b_force)
> +            msg_Err(vd, "Cannot create Direct2D factory (hr = 0x%x)!",
> +                (uint32_t)hr);
I think here, not checking on b_force would be better, otherwise there is no
way to know why the loading failed.
> +        goto error;
> +    }
> +
> +    sys->p_d2_render_target = NULL;
It would be better to move that line before any goto.

> +    D2D_CreateRenderTarget(vd);
Shouldn't you check that it succeeds (the vout is unusable if not) ?

> +    vout_display_info_t info = vd->info;
> +    info.is_slow              = false;
> +    info.has_double_click     = true;
> +    info.has_hide_mouse       = true;
> +    info.has_pictures_invalid = true;
> +    vd->info = info;
> +
> +    RECT rc;
> +    GetClientRect(sys->hvideownd, &rc);
> +    D2D1_SIZE_U size = {
> +        rc.right - rc.left,
> +        rc.bottom - rc.top
> +    };
> +
> +    vd->fmt.i_chroma = VLC_CODEC_RGB32; /* masks change this to BGR32 for ID2D1Bitmap */
> +    vd->fmt.i_rmask  = 0x000000ff;
> +    vd->fmt.i_gmask  = 0x0000ff00;
> +    vd->fmt.i_bmask  = 0x00000000; /* todo - fix this (should be 0x00ff0000) */
> +    vd->fmt.i_width  = size.width;
> +    vd->fmt.i_height = size.height;
 D2D does not resize itself ? If it does, then you should not change
the width/height of vd->fmt.

> +    vd->pool    = Pool;
> +    vd->prepare = Prepare;
> +    vd->display = Display;
> +    vd->manage  = Manage;
> +    vd->control = Control;
> +
> +    EventThreadUpdateTitle(sys->event, VOUT_TITLE " (Direct2D output)");
> +
> +#ifndef NDEBUG
> +    msg_Dbg(vd, "Ready");
> +#endif
> +
> +    return VLC_SUCCESS;
> +
> +error:
> +    Close(VLC_OBJECT(vd));
> +    return VLC_EGENERIC;
> +}
> +/**
> + * Performs set up of ID2D1Bitmap memory ready for blitting
> + */
> +static void Prepare(vout_display_t *vd, picture_t *picture)
> +{
> +    vout_display_sys_t *sys = vd->sys;
> +
> +    if (sys->p_d2_render_target && sys->p_d2_bitmap)
> +    {
> +        UINT32 pitch = vd->fmt.i_width * 4;
 If pitch is the size in bytes of a line in picture->p[0].p_pixels, then you
must use picture->p[0].i_pitch which may not be equal to
vd->fmt.i_width * 4. (It may be higher).

> +        HRESULT hr = ID2D1Bitmap_CopyFromMemory(sys->p_d2_bitmap, NULL,
> +            picture->p[0].p_pixels, pitch);
> +        if( hr != S_OK )
> +            msg_Err(vd, "Failed to copy bitmap memory (hr = 0x%x)!",
> +                (uint32_t)hr);
%x takes an unsigned as argument, so (unsigned)hr should be used.

> +#ifndef NDEBUG
> +        /*msg_Dbg(vd, "Bitmap dbg: target = %p, pitch = %d, bitmap = %p",
> +            sys->p_d2_render_target, pitch, sys->p_d2_bitmap);*/
> +#endif
> +    }
> +}
> +
> +/**
> + * Blits a scaled picture_t to the render target
> + */
> +static void Display(vout_display_t *vd, picture_t *picture)
> +{
> +    vout_display_sys_t *sys = vd->sys;
> +
> +    D2D1_RECT_F rc = {
> +        sys->rect_dest.left,
> +        sys->rect_dest.top,
> +        sys->rect_dest.right,
> +        sys->rect_dest.bottom
> +    };
> +
> +    if (sys->p_d2_render_target && sys->p_d2_bitmap) {
> +        ID2D1HwndRenderTarget_BeginDraw(sys->p_d2_render_target);
> +        ID2D1HwndRenderTarget_DrawBitmap(sys->p_d2_render_target,
> +            sys->p_d2_bitmap, &rc, 1.0f,
> +            D2D1_BITMAP_INTERPOLATION_MODE_LINEAR, NULL);
 Just for information, what are the available interpolation modes?

> +        ID2D1HwndRenderTarget_EndDraw(sys->p_d2_render_target, NULL, NULL);
> +    }
> +
> +    picture_Release(picture);
> +
> +    CommonDisplay(vd);
> +}
> +
> +/**
> + * Resizes a ID2D1HWndRenderTarget
> + */
> +void D2D_ResizeRenderTarget(vout_display_t *vd)
> +{
> +    vout_display_sys_t *sys = vd->sys;
> +
> +    D2D1_SIZE_U size = {
> +        sys->rect_dest.right - sys->rect_dest.left,
> +        sys->rect_dest.bottom - sys->rect_dest.top
> +    };
> +
> +    HRESULT hr  = ID2D1HwndRenderTarget_Resize(sys->p_d2_render_target, &size);
> +    if(hr != S_OK) {
> +        msg_Err(vd, "Cannot resize render target (width = %d, height = %d, hr = 0x%x)!",
> +            size.width, size.height, (uint32_t)hr);
> +
> +        D2D_DestroyRenderTarget(vd);
 Is it really needed? What happens when ID2D1HwndRenderTarget_Resize fails? Are
the target/bitmap unusable? Are they still usables but aren't resized?
 If there are still usable, then I think it will simplify the code to ignore the
error and will at least allows to continue displaying video.
> +    }
> +}

> +/**
> + * Creates a ID2D1HwndRenderTarget and associated ID2D1Bitmap
> + */
> +void D2D_CreateRenderTarget(vout_display_t *vd)
> +{
> +    vout_display_sys_t *sys = vd->sys;
> +
> +    D2D1_PIXEL_FORMAT pf = {
> +        DXGI_FORMAT_B8G8R8A8_UNORM,
> +        D2D1_ALPHA_MODE_IGNORE
> +    };
> +
> +    D2D1_RENDER_TARGET_PROPERTIES rtp = {
> +        D2D1_RENDER_TARGET_TYPE_DEFAULT,
> +        pf,
> +        0,
> +        0,
> +        D2D1_RENDER_TARGET_USAGE_NONE,
> +        D2D1_FEATURE_LEVEL_DEFAULT
> +    };
> +
> +    D2D1_SIZE_U size = {
> +        sys->rect_dest.right - sys->rect_dest.left,
> +        sys->rect_dest.bottom - sys->rect_dest.top
> +    };
> +
> +    D2D1_HWND_RENDER_TARGET_PROPERTIES hrtp = {
> +        sys->hvideownd,
> +        size,
> +        D2D1_PRESENT_OPTIONS_IMMEDIATELY /* this might need fiddling */
> +    };
> +
> +    HRESULT hr  = ID2D1Factory_CreateHwndRenderTarget(sys->p_d2_factory,
> +        &rtp, &hrtp, &sys->p_d2_render_target);
> +    if(hr != S_OK) {
> +        msg_Err(vd, "Cannot create render target (hvideownd = 0x%x, width = %d, height = %d, pf.format = %d, hr = 0x%x)!",
> +            (uint32_t)hrtp.hwnd, hrtp.pixelSize.width, hrtp.pixelSize.height,
> +            pf.format, (uint32_t)hr);
> +
> +        D2D_DestroyRenderTarget(vd);
 You can just returns from here, there is nothing to destroy. Besides, if
you don't I fear that trying to create the bitmap will segfault
(p_d2_render_target is NULL).
> +    }
> +
> +    ID2D1Factory_GetDesktopDpi(sys->p_d2_factory, &sys->f_d2_dpi_x,
> +        &sys->f_d2_dpi_y);
> +
> +    D2D1_BITMAP_PROPERTIES bp = {
> +        pf,
> +        sys->f_d2_dpi_x,
> +        sys->f_d2_dpi_y
> +    };
> +
> +    D2D1_SIZE_U bitmap_size = {
> +        vd->fmt.i_width,
> +        vd->fmt.i_height
> +    };
> +
> +    hr = ID2D1HwndRenderTarget_CreateBitmap(sys->p_d2_render_target,
> +        bitmap_size, NULL, 0, &bp, &sys->p_d2_bitmap);
> +    if (hr != S_OK) {
> +        msg_Err(vd, "Failed to create bitmap (hr = 0x%x)!", (uint32_t)hr);
> +    }
> +
> +#ifndef NDEBUG
> +    msg_Dbg(vd, "Render trgt dbg: dpi = %f, render_target = %p, bitmap = %p",
> +        sys->f_d2_dpi_x, sys->p_d2_render_target, sys->p_d2_bitmap);
> +#endif
> +}
 Are you sure that when ID2D1Factory_CreateHwndRenderTarget or
ID2D1HwndRenderTarget_CreateBitmap fails, they don't set an unspecified value
in the pointer argument? If you don't know, it is safer to set them to NULL.

Regards,

-- 
fenrir




More information about the vlc-devel mailing list