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

[Laurent Aimar]

Hi,

On Sun, Jul 25, 2010 at 04:03:11PM +0300, David Kaplan wrote:
> Adds support for the D2D API on Win7/Vista SP2 with Platform Update
> Requires d2d1 contrib headers
 I have tested it and it works well (including zoom/ar/autoscale/crop).
Good work.

 I have a last round of small remarks (that I missed the last time, sorry), and
it will be good to commit.

> +vlc_module_begin ()
> +    set_category(CAT_VIDEO)
> +    set_subcategory(SUBCAT_VIDEO_VOUT)
> +    set_help(D2D_HELP)
> +    set_shortname("Direct2D")
> +    set_description(N_("Direct2D video output"))
> +    set_capability("vout display", 60)
 Just a remark for the other devs: that makes it higher than wingdi and
glwin32. With the current feature set of both modules, I don't think there is
any issue.
 But I think that using 65 would be better (to have it higher than SDL but
lower than direct3d).

> +    add_shortcut("direct2d")
> +    set_callbacks(Open, Close)
> +vlc_module_end ()

> +    vout_display_info_t info = vd->info;
> +    info.is_slow              = false;
> +    info.has_double_click     = true;
> +    info.has_hide_mouse       = true;
 It must be false, otherwise the mouse won't be hidden. (Yes, it has
recently changed).

> +    info.has_pictures_invalid = false;
> +    vd->info = info;
> +
> +    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;
 Can't 0x00ff0000 works (now that you don't use RGBA)?

> +/**
> + * 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->d2_render_target && sys->d2_bitmap) {
> +        /* This seems unnecessary
> +        D2D1_RECT_U r_src = {
> +            sys->rect_src_clipped.left,
> +            sys->rect_src_clipped.top,
> +            sys->rect_src_clipped.right,
> +            sys->rect_src_clipped.bottom
> +        };
> +        */
 You are right, it is unecessary and so you could remove it.

> +/**
> + * 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 r_dest = {
> +        sys->rect_dest.left,
> +        sys->rect_dest.top,
> +        sys->rect_dest.right,
> +        sys->rect_dest.bottom
> +    };
> +
> +    if (sys->d2_render_target && sys->d2_bitmap) {
> +        ID2D1HwndRenderTarget_BeginDraw(sys->d2_render_target);
> +
> +        ID2D1HwndRenderTarget_DrawBitmap(sys->d2_render_target,
> +                                         sys->d2_bitmap,
> +                                         &r_dest,
> +                                         1.0f,
> +                                         D2D1_BITMAP_INTERPOLATION_MODE_LINEAR,
> +                                         NULL);
> +
> +        HRESULT hr = ID2D1HwndRenderTarget_EndDraw(sys->d2_render_target,
> +                                                 NULL,
> +                                                 NULL);
 Alignment is a bit off here.
> +        if (hr ==  D2DERR_RECREATE_TARGET) {
> +            D2D_DestroyRenderTarget(vd);
> +            D2D_CreateRenderTarget(vd);
> +        }
> +    }
> +
> +    picture_Release(picture);
> +
> +    CommonDisplay(vd);
> +}

Regards,

-- 
fenrir