gmane.comp.video.mplayer.devel

Re: [PATCH] Direct3D Much better D3D management

On Mon, Dec 1, 2008 at 11:22 PM, Diego Biurrun <diego <at> 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...

Index: libvo/vo_direct3d.c
===================================================================
--- libvo/vo_direct3d.c	(revision 28056)
+++ libvo/vo_direct3d.c	(working copy)
@@ -168,12 +168,12 @@
     priv->is_clear_needed = 1;
 }

-/** @brief Destroy D3D Context related to the current window.
+/** @brief Destroy D3D Offscreen and Backbuffer surfaces.
  */
-static void destroy_d3d_context(void)
+static void destroy_d3d_surfaces(void)
 {
-    mp_msg(MSGT_VO, MSGL_V, "<vo_direct3d>destroy_d3d_context called\r\n");
-    /* Let's destroy the old (if any) D3D Content */
+    mp_msg(MSGT_VO, MSGL_V, "<vo_direct3d>destroy_d3d_surfaces called\r\n");
+    /* Let's destroy the old (if any) D3D Surfaces */

(Continue reading)

headers

Georgi Petrov | 1 Dec 23:10

Re: [PATCH] Direct3D Much better D3D management

I getting mad. I'm sure I fixed whitespace before the opening braces
as well, but it somehow slipped... I attach the updated patch.

Index: libvo/vo_direct3d.c
===================================================================
--- libvo/vo_direct3d.c	(revision 28056)
+++ libvo/vo_direct3d.c	(working copy)
@@ -168,12 +168,12 @@
     priv->is_clear_needed = 1;
 }

-/** @brief Destroy D3D Context related to the current window.
+/** @brief Destroy D3D Offscreen and Backbuffer surfaces.
  */
-static void destroy_d3d_context(void)
+static void destroy_d3d_surfaces(void)
 {
-    mp_msg(MSGT_VO, MSGL_V, "<vo_direct3d>destroy_d3d_context called\r\n");
-    /* Let's destroy the old (if any) D3D Content */
+    mp_msg(MSGT_VO, MSGL_V, "<vo_direct3d>destroy_d3d_surfaces called\r\n");
+    /* Let's destroy the old (if any) D3D Surfaces */

     if (priv->locked_rect.pBits) {
         IDirect3DSurface9_UnlockRect(priv->d3d_surface);
@@ -185,32 +185,68 @@
         priv->d3d_surface = NULL;
     }

-    if (priv->d3d_device != NULL) {

(Continue reading)

headers

Diego Biurrun | 1 Dec 23:18

Re: [PATCH] Direct3D Much better D3D management

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

headers

Kevin DeKorte | 1 Dec 23:46

Re: [PATCH] Direct3D Much better D3D management

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

headers

Georgi Petrov | 2 Dec 00:06

Re: [PATCH] Direct3D Much better D3D management

> 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.

Index: libvo/vo_direct3d.c
===================================================================
--- libvo/vo_direct3d.c	(revision 28056)
+++ libvo/vo_direct3d.c	(working copy)
@@ -168,12 +168,12 @@
     priv->is_clear_needed = 1;
 }

-/** @brief Destroy D3D Context related to the current window.
+/** @brief Destroy D3D Offscreen and Backbuffer surfaces.
  */

(Continue reading)

headers

Kevin DeKorte | 2 Dec 00:13

Re: [PATCH] Direct3D Much better D3D management


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

(Continue reading)

headers

Diego Biurrun | 2 Dec 00:46

Re: [PATCH] Direct3D Much better D3D management

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.

(Continue reading)

headers

Reimar Döffinger | 2 Dec 10:43

Re: [PATCH] Direct3D Much better D3D management

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

headers

Georgi Petrov | 2 Dec 15:25

Re: [PATCH] Direct3D Much better D3D management

> 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

(Continue reading)

headers

Georgi Petrov | 2 Dec 16:03

Re: [PATCH] Direct3D Much better D3D management

Sorry - this check_events (fixed with the latest patch from Reimar)
left from my experiments with the current problem I posted about.

headers

Georgi Petrov | 2 Dec 16:09

Re: [PATCH] Direct3D Much better D3D management

>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.....

headers

Reimar Döffinger | 2 Dec 16:28

Re: [PATCH] Direct3D Much better D3D management

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.

headers

Georgi Petrov | 8 Dec 11:18

Re: [PATCH] Direct3D Much better D3D management

On Tue, Dec 2, 2008 at 4:25 PM, Georgi Petrov <gogothebee <at> 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.
>
>

(Continue reading)

headers

Reimar Döffinger | 8 Dec 12:21

Re: [PATCH] Direct3D Much better D3D management

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

headers

Georgi Petrov | 8 Dec 12:28

Re: [PATCH] Direct3D Much better D3D management

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.

headers

Uoti Urpala | 2 Dec 00:19

Re: [PATCH] Direct3D Much better D3D management

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.

headers

compn | 2 Dec 02:52

Re: [PATCH] Direct3D Much better D3D management

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

headers

Diego Biurrun | 1 Dec 23:16

Re: [PATCH] Direct3D Much better D3D management

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 <at> 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

(Continue reading)

headers

Reimar Döffinger | 1 Dec 23:17

Re: [PATCH] Direct3D Much better D3D management

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