[PATCH] Direct3D Much better D3D management
Ok, I know this patch may seem bigger than the Reimar's requirements. I can make more smaller patches that accomplish the same in the end if you wish so. The main purpose of this patch is to do a much better at D3D's init/reset/destroy than before and fix 2-3 bugs. Let me start: When D3D is initialized, those things are created: 1. The "D3D" itself. uses priv->d3d_handle This is handled in preinit to support query of supported formats later. 2. The D3D Device uses priv->d3d_device This is initialized in configure_d3d(); 3. The offscreen and backbuffer surfaces. uses priv->d3d_surface uses priv->d3d_backbuf This is initialized in create_d3d_surfaces(); So - we have 3 "levels" of D3D initialization before we can render frames. This is the natural way of initializing D3D. Remember good this order. We have to handle 2 situations once D3D is initialized: 1. Window resize / going to/from fullscreen 2. Adapter becoming uncooperative We handle window resize / going to/from fullscreen in function resize_d3d: * Destroy the offscreen/backbuffer surfaces * Do a D3D Reset (which requires that no surfaces are allocated) * Create both surfaces again. The whole thing is needed because the Backbuffer surface doesn't rescale itself and by reseting the D3D device and creating a new backbuffer surface, it fills the new window's coordinates. About the offscreen surface - we wouldn't need to destroy and recreate it again (as it has the non-changing coordinates of the original movie), if D3D_Reset didn't require that all D3D resources are freed before calling it. So that's it. We handle the adapter becoming uncooperative in function reconfigure_d3d: * This function has new meaning from the previous one. It totally destroys D3D and recreates it again by doing: * Destroy the offscreen/backbuffer surfaces * Destroy the D3D Device created in configure_d3d() * Stop the whole D3D started in preinit * Start the whole D3D * Call configure_d3d() After calling configure_d3d() we've simulated the initialization after preinit. Problems fixed: * Very good D3D initialize / destroy separation * More meaningful functions: create_d3d_surfaces() destroy_d3d_surfaces() configure_d3d() resize_d3d() reconfigure_d3d() uninit_d3d() Now each function does exactly what it says. Before the patch "reconfigure_d3d" used to both destroy and recreate D3D, BUT NOT FROM THE BEGINNING (ignoring to stop the whole D3D and create it again as in preinit). It's more guaranteed to work now and called ONLY when the adapter is uncooperative. Now resize_d3d does "lightweight" resizing by calling Reset instead of destroy/create the adapter. * No need for destroying and recreating the D3D adapter on each resize or going to/from fullscreen. Before this patch we killed the D3D Device and created it again. For fast window resizing this used to happen many times in a second. Now we call Reset, which based on my test is faster and safer. Before the patch if you tried to resiz the window too fast, D3D used to fall into a state, where the D3D Device can't be created again. By calling Reset this doesn't happen any more AND it's faster/safer. * Going fullscreen / coming back should be a little faster (again - we skip D3D Device destroy/create and change it with Reset) * Fixed a bug when after calling the "previous" reconfigure_d3d() we didn't allocate new backbuffer after coming from fullscreen (or something like that, I'm not sure). The problem was that after coming back from fullscreen AND resizing the window, the backbuffer stayed with the old dimensions and the image became aliased.
Just a little updates and fixes to the "mp_msg" logs. Nothing in the logic changed.
Uh, I have additional advancements, but as I do them on an hourly basis, I'll skip until tomorrow, when you can review this patch, so we can discuss it and I can add more. I've begun to test -wid mode and I've found some interesting bugs. For example - when using MPUI as frontend. When I start MPUI, it has some window size. After I load a video inside, the window doesn't resize itself correctly to the size of the video. The problem is somewhere in w32_common.c and I'm investigating. D3D is given WRONG windows dimensions, so it creates backbuffer with WRONG dimensions and everything works just wrong. I'm missing something...
The final (I think) D3D management patch.
On Mon, Dec 01, 2008 at 12:29:20PM +0200, Georgi Petrov wrote:
The final (I think) D3D management patch.
Index: libvo/vo_direct3d.c =================================================================== --- libvo/vo_direct3d.c (revision 28049) +++ libvo/vo_direct3d.c (working copy) @@ -168,12 +168,12 @@ - mp_msg(MSGT_VO, MSGL_V, "<vo_direct3d>destroy_d3d_context called\r\n"); + mp_msg(MSGT_VO,MSGL_V,"<vo_direct3d>destroy_d3d_surfaces called\r\n");
cosmetics Please keep the spaces after the commas separating function arguments that I added. They make things much more readable. Diego
Please keep the spaces after the commas separating function arguments that I added. They make things much more readable.
Fixed.
Reimar - after you look at the patch and confirm that there's nothing wrong, I can split it into smaller logical patches if you would like me to. I'm working on other problems as well and will commit changes very soon.
On Mon, Dec 1, 2008 at 11:22 PM, Diego Biurrun <diego@biurrun.de> wrote:
On Mon, Dec 01, 2008 at 12:48:48PM +0200, Georgi Petrov wrote:
Please keep the spaces after the commas separating function arguments that I added. They make things much more readable.
Fixed.
No.
Huh... The merger didn't make a difference between white spaces. Try this one...
I getting mad. I'm sure I fixed whitespace before the opening braces as well, but it somehow slipped... I attach the updated patch.
On Tue, Dec 02, 2008 at 12:10:22AM +0200, Georgi Petrov wrote:
I getting mad. I'm sure I fixed whitespace before the opening braces as well, but it somehow slipped... I attach the updated patch.
--- libvo/vo_direct3d.c (revision 28056) +++ libvo/vo_direct3d.c (working copy) @@ -185,32 +185,68 @@ + mp_msg(MSGT_VO, MSGL_ERR, + "<vo_direct3d><INFO>IDirect3D9_CreateOffscreenPlainSurface Failed.\n");
random indention Diego
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Georgi Petrov wrote:
I getting mad. I'm sure I fixed whitespace before the opening braces as well, but it somehow slipped... I attach the updated patch.
Isn't there a way to automatically do this using indent rules? Seems like a lot of conversation over whitespace happens with patches. Kevin - -- Get my public GnuPG key from http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x7D0BD5D1 -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEYEARECAAYFAkk0aUwACgkQ6w2kMH0L1dF9pgCfXuOdVqgq8mLGVnW2JXlK/UGP maQAn2rWYCdCUjFtlTfAoy1O3Sr3mE44 =O7/m -----END PGP SIGNATURE-----
Isn't there a way to automatically do this using indent rules? Seems like a lot of conversation over whitespace happens with patches.
Good point. About those indentations - I'll switch to a different merger. This one does a terrible job and 2/3 of the mistakes come from it. BTW - I removed the whitespace before "(" intentionally. That's the coding style, right?
random indention
Not quite. Just trying to stay within this outdated as hell 80 columns madness... Reimer, please apply it. I attach a patch with those issues fixed.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Georgi Petrov wrote:
Isn't there a way to automatically do this using indent rules? Seems like a lot of conversation over whitespace happens with patches.
Good point.
May I suggest that mplayer use something like indent -kr -l100 -i4 -nut Which gives output like this... void dbus_open_next() { gchar *path; DBusMessage *message; if (connection != NULL && control_id != 0) { path = g_strdup_printf("/control/%i", control_id); message = dbus_message_new_signal(path, "com.gecko.mediaplayer", "Next"); dbus_connection_send(connection, message, NULL); dbus_message_unref(message); g_free(path); } } Kevin - -- Get my public GnuPG key from http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x7D0BD5D1 -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEYEARECAAYFAkk0b5cACgkQ6w2kMH0L1dFhzwCfcTs5EPRmKexAmRmnjvX493gR K70AnRAYPB+2V+9BSYU+qWxhYJ6erShm =DAT6 -----END PGP SIGNATURE-----
On Tue, Dec 02, 2008 at 01:06:28AM +0200, Georgi Petrov wrote:
Isn't there a way to automatically do this using indent rules? Seems like a lot of conversation over whitespace happens with patches.
Good point.
About those indentations - I'll switch to a different merger. This one does a terrible job and 2/3 of the mistakes come from it.
You are not using Subversion?
BTW - I removed the whitespace before "(" intentionally. That's the coding style, right?
K & R style is generally the closest to what is preferred around here (with 4 space indentaion instead of tabs) and is what I switched the file to. This means no space between function name and (.
random indention
Not quite. Just trying to stay within this outdated as hell 80 columns madness...
This has nothing to do with outdated terminal widths or anything of the sort. Unfortunately our brains and eyes do not follow the evolution of display hardware. Above a certain length readability gets hurt. You will have noticed that newspapers and magazines use multicolumn layout even though they have plenty of horizontal space available on their pages. Also, nobody ever asked you to sacrifice everything including common sense to stay below 80 characters. Just keep lines reasonably short, preferably below 80 characters. Don't chop off your hands just to cut your fingernails.
--- libvo/vo_direct3d.c (revision 28056) +++ libvo/vo_direct3d.c (working copy) @@ -185,32 +185,68 @@
+ if (FAILED(IDirect3DDevice9_CreateOffscreenPlainSurface( + priv->d3d_device, priv->src_width, priv->src_height, + priv->movie_src_fmt, D3DPOOL_DEFAULT, &priv->d3d_surface, NULL))) { + mp_msg(MSGT_VO, MSGL_ERR, + "<vo_direct3d><INFO>IDirect3D9_CreateOffscreenPlainSurface Failed.\n"); + return 0;
random indentation
@@ -250,30 +273,86 @@ + /* Destroy the D3D Device */ + if (priv->d3d_device != NULL) { + IDirect3DDevice9_Release(priv->d3d_device); + priv->d3d_device = NULL; + }
ditto
@@ -285,9 +364,14 @@
+ /* Destroy the D3D Device */ + if (priv->d3d_device != NULL) { + IDirect3DDevice9_Release(priv->d3d_device); + priv->d3d_device = NULL; + }
ditto
@@ -543,7 +629,20 @@
- if (!reconfigure_d3d()) + /* "config" may be called several times, so if this is not the first + * call, we should destroy Direct3D adapter and surfaces before + * calling configure_d3d, which will create them again. + */ + + destroy_d3d_surfaces();
ditto
+ if (priv->d3d_device != NULL) { + IDirect3DDevice9_Release(priv->d3d_device); + priv->d3d_device = NULL; + }
ditto Sorry for the harsh words, but how hard can it be to fix these issues? Diego
Hello, applied. On Tue, Dec 02, 2008 at 01:06:28AM +0200, Georgi Petrov wrote:
+ if (priv->d3d_backbuf != NULL) { + IDirect3DSurface9_Release(priv->d3d_backbuf); + priv->d3d_backbuf = NULL; }
Btw. have you tried what happens when you call Release on a NULL pointer? All these checks might be unnecessary. Also, if they are needed, what do you think about writing
if (priv->d3d_backbuf) IDirect3DSurface9_Release(priv->d3d_backbuf); priv->d3d_backbuf = NULL;
Instead? I cam to prefer it because 1) It is more obvious that priv->d3d_backbuf is always NULL afterwards 2) you save the {}, and essential it becomes just a "guard-if" for the called function. Greetings, Reimar Döffinger
Hello,
applied.
Thanks.
+ if (priv->d3d_backbuf != NULL) { + IDirect3DSurface9_Release(priv->d3d_backbuf); + priv->d3d_backbuf = NULL; }
Btw. have you tried what happens when you call Release on a NULL pointer?
No...
All these checks might be unnecessary. Also, if they are needed, what do you think about writing
if (priv->d3d_backbuf) IDirect3DSurface9_Release(priv->d3d_backbuf); priv->d3d_backbuf = NULL;
Instead? I cam to prefer it because 1) It is more obvious that priv->d3d_backbuf is always NULL afterwards 2) you save the {}, and essential it becomes just a "guard-if" for the called function.
I understand. There's no difference for me, but I send a patch for all deallocations in destroy_d3d_surfaces. Apply them if you like it better.
Sorry - this check_events (fixed with the latest patch from Reimar) left from my experiments with the current problem I posted about.
Log: Remove resize_d3d call from render_d3d_frame that was made useless by previous commit. If this is necessary e.g. to prevent flicker while resizing, check_events should be called instead or even better the functionality be moved to some higher layer.
Well - my mistake again... This is what happens when I continue implementing/testing new features and they slip into a correction of a patch I've submited 5 days ago.....
On Tue, Dec 02, 2008 at 05:09:21PM +0200, Georgi Petrov wrote:
Log: Remove resize_d3d call from render_d3d_frame that was made useless by previous commit. If this is necessary e.g. to prevent flicker while resizing, check_events should be called instead or even better the functionality be moved to some higher layer.
Well - my mistake again... This is what happens when I continue implementing/testing new features and they slip into a correction of a patch I've submited 5 days ago.....
Well, you know you could have multiple MPlayer checkouts or you could even look at git (+git-svn, http://git.or.cz, windows binaries available there at near the bottom) which allows you to easily create local branches.
On Tue, Dec 2, 2008 at 4:25 PM, Georgi Petrov <gogothebee@gmail.com> wrote:
Hello,
applied.
Thanks.
+ if (priv->d3d_backbuf != NULL) { + IDirect3DSurface9_Release(priv->d3d_backbuf); + priv->d3d_backbuf = NULL; }
Btw. have you tried what happens when you call Release on a NULL pointer?
No...
All these checks might be unnecessary. Also, if they are needed, what do you think about writing
if (priv->d3d_backbuf) IDirect3DSurface9_Release(priv->d3d_backbuf); priv->d3d_backbuf = NULL;
Instead? I cam to prefer it because 1) It is more obvious that priv->d3d_backbuf is always NULL afterwards 2) you save the {}, and essential it becomes just a "guard-if" for the called function.
I understand. There's no difference for me, but I send a patch for all deallocations in destroy_d3d_surfaces. Apply them if you like it better.
Did you forget this patch?
On Mon, Dec 08, 2008 at 12:18:12PM +0200, Georgi Petrov wrote:
All these checks might be unnecessary. Also, if they are needed, what do you think about writing
if (priv->d3d_backbuf) IDirect3DSurface9_Release(priv->d3d_backbuf); priv->d3d_backbuf = NULL;
Instead? I cam to prefer it because 1) It is more obvious that priv->d3d_backbuf is always NULL afterwards 2) you save the {}, and essential it becomes just a "guard-if" for the called function.
I understand. There's no difference for me, but I send a patch for all deallocations in destroy_d3d_surfaces. Apply them if you like it better.
Did you forget this patch?
No, but there is more important stuff. Also there is no need for a patch, making these modifications by hand does not even take one minute. Greetings, Reimar Döffinger
Ok, I was asking because I sent a patch as a replay to this request and I just saw it didn't make it into SVN. It doesn't bother me.
On Mon, 2008-12-01 at 15:46 -0700, Kevin DeKorte wrote:
Georgi Petrov wrote:
I getting mad. I'm sure I fixed whitespace before the opening braces as well, but it somehow slipped... I attach the updated patch.
Isn't there a way to automatically do this using indent rules? Seems like a lot of conversation over whitespace happens with patches.
You can use automatic indent once, but it's not an answer for continuous maintenance. It doesn't always get everything right and you need to fix some things by hand; if you'd run it after every change you'd constantly need to fix the same problems again.
On Mon, 01 Dec 2008 15:46:36 -0700, Kevin DeKorte wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Georgi Petrov wrote:
I getting mad. I'm sure I fixed whitespace before the opening braces as well, but it somehow slipped... I attach the updated patch.
Isn't there a way to automatically do this using indent rules? Seems like a lot of conversation over whitespace happens with patches.
it might just be easier to run indent over it when everything is done no need to worry about every patch indentation, just post diff -w. -compn
On Tue, Dec 02, 2008 at 12:05:59AM +0200, Georgi Petrov wrote:
On Mon, Dec 1, 2008 at 11:22 PM, Diego Biurrun <diego@biurrun.de> wrote:
On Mon, Dec 01, 2008 at 12:48:48PM +0200, Georgi Petrov wrote:
Please keep the spaces after the commas separating function arguments that I added. They make things much more readable.
Fixed.
No.
Huh... The merger didn't make a difference between white spaces. Try this one...
--- libvo/vo_direct3d.c (revision 28056) +++ libvo/vo_direct3d.c (working copy) @@ -250,30 +273,86 @@ + + /* Destroy the D3D Device */ + if (priv->d3d_device != NULL) { + IDirect3DDevice9_Release (priv->d3d_device); + priv->d3d_device = NULL; + }
missing indentation, in other places as well
+ IDirect3D9_Release (priv->d3d_handle);
Now you've messed up the spaces between function name and parenthesis, same in other places. Diego
On Tue, Dec 02, 2008 at 12:05:59AM +0200, Georgi Petrov wrote:
+ if (priv->d3d_device != NULL) { + IDirect3DDevice9_Release (priv->d3d_device); + priv->d3d_device = NULL; + }
I assume this indentation is not intentional. I can fix it myself. Any objections to it? I'd be in favour of applying it as is, it is a bit big but not too complex. Greetings, Reimar Döffinger
Ok, I know this patch may seem bigger than the Reimar's requirements. I can make more smaller patches that accomplish the same in the end if you wish so.
I don't know if someone else has had similar problems but --disable-gl and --enable-direct3d (or letting it autodetect) seems to exclude libvo/w32_common.c from being compiled. The only mention of w32_common.c in configure is inside a _gl (then _gl_win32) conditional: if test "$_gl" = yes ; then _def_gl='#define CONFIG_GL 1' _vosrc="$_vosrc vo_gl.c vo_gl2.c gl_common.c" if test "$_gl_win32" = yes ; then _def_gl_win32='#define GL_WIN32 1' _vosrc="$_vosrc w32_common.c" _res_comment="win32 version" fi _vomodules="opengl $_vomodules" else _def_gl='#undef CONFIG_GL' _def_gl_win32='#undef GL_WIN32' _novomodules="opengl $_novomodules" fi echores "$_gl" -DrD-
I don't know if someone else has had similar problems but --disable-gl and --enable-direct3d (or letting it autodetect) seems to exclude libvo/w32_common.c from being compiled.
The only mention of w32_common.c in configure is inside a _gl (then _gl_win32) conditional:
Yes, I haven't thought about it. I'll post a fix these days.
On Mon, Dec 01, 2008 at 11:54:06AM -0800, David DeHaven wrote:
Ok, I know this patch may seem bigger than the Reimar's requirements. I can make more smaller patches that accomplish the same in the end if you wish so.
I don't know if someone else has had similar problems but --disable-gl and --enable-direct3d (or letting it autodetect) seems to exclude libvo/w32_common.c from being compiled.
Fixed. Diego
participants (7)
-
compn -
David DeHaven -
Diego Biurrun -
Georgi Petrov -
Kevin DeKorte -
Reimar Döffinger -
Uoti Urpala