[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