MPlayer cross-platform UNIX movie player, development

KO Myung-Hun | 1 Mar 2009 15:09

Re: [PATCH] vo_kva

Hi/2.

Diego Biurrun wrote:
>> --- libvo/vo_kva.c	(revision 0)
>> +++ libvo/vo_kva.c	(revision 0)
>>  <at>  <at>  -0,0 +1,1193  <at>  <at> 
>> +static void imgDisplay(void)
>> +{
>> +    PVOID       pBuffer;
>> +    ULONG       ulBPL;
>>     
>
> Any particular reason to use so many spaces?  Not that it matters..
>
>   

No. But maybe it's due to my habit of align variables. I think there 
were a variable with long name type and deleted it, then I didn't 
realign them.

Fixed, anyway.

>> +#define MRETURN(ret)  return (MRESULT)(ret)
>>     
>
> This just looks like obfuscation to me.
>
>   

I agree not, but changed.

(Continue reading)

Reimar Döffinger | 7 Mar 2009 15:36

Picon

Picon

Re: [PATCH] vo_kva

On Sun, Mar 01, 2009 at 11:09:49PM +0900, KO Myung-Hun wrote:
> +static vo_info_t info = {

Should be const now.

> +#define SET_ASPECT_RATIO(ratio) {\
> +    m_int.kvas.ulRatio = (ratio);\
> +    kvaSetup(&m_int.kvas);\
> +}

Make it a function (you only need one argument more) and you can avoid
all the syntactic issues that such a macro has.

> +struct tagVOKVAINTERNAL {
> +    HAB         hab;
> +    HMQ         hmq;
> +    HWND        hwndFrame;
> +    HWND        hwndClient;
> +    HWND        hwndSysMenu;
> +    HWND        hwndTitleBar;
> +    HWND        hwndMinMax;
> +    FOURCC      fcc;
> +    INT         iImageFormat;
> +    KVASETUP    kvas;
> +    KVACAPS     kvac;
> +    RECTL       rclDst;
> +    INT         bpp;
> +    LONG        lStride;
> +    PBYTE       pbImage;
> +    BOOL        fFixT23;

(Continue reading)

KO Myung-Hun | 8 Mar 2009 11:23

Re: [PATCH] vo_kva

Hi/2.

Reimar Döffinger wrote:
> On Sun, Mar 01, 2009 at 11:09:49PM +0900, KO Myung-Hun wrote:
>   
>> +static vo_info_t info = {
>>     
>
> Should be const now.
>
>   

Fixed.

>> +#define SET_ASPECT_RATIO(ratio) {\
>> +    m_int.kvas.ulRatio = (ratio);\
>> +    kvaSetup(&m_int.kvas);\
>> +}
>>     
>
> Make it a function (you only need one argument more) and you can avoid
> all the syntactic issues that such a macro has.
>
>   

Ok.

>> +struct tagVOKVAINTERNAL {
>> +    HAB         hab;
>> +    HMQ         hmq;

(Continue reading)

Reimar Döffinger | 8 Mar 2009 12:24

Picon

Picon

Re: [PATCH] vo_kva

>>> +            WinCalcFrameRect(hwnd, &rcl, FALSE);
>>> +
>>> +            if (rcl.xRight - rcl.xLeft > vo_screenwidth) {
>>> +                rcl.xLeft  = 0;
>>> +                rcl.xRight = vo_screenwidth;
>>> +            }
>>> +
>>> +            if (rcl.yTop - rcl.yBottom > vo_screenheight) {
>>> +                rcl.yBottom = 0;
>>> +                rcl.yTop    = vo_screenheight;
>>> +            }
>>
>> Are you really sure you want to limit the Window size to the screen
>> size? The code for Windows specifically disables this limitation because
>> it can be really annoying for HD videos.
>
> It's a workaround for T23 laptop with S3 video card. If a movie window size 
> is larger than a screen size, the image is distorted.

Well, you already have a flag for that hardware, right? You could use it
here, too.
Anyway it is your decision, I just wanted to point out that the vos that
I maintain will specifically try to avoid that behaviour and your vo is
probably going to stand out from the others by behaving differently,
though that is not necessarily a bad thing.

>>> +    // if slave mode, ignore mouse events and deliver them to a parent 
>>> window
>>> +    if (WinID != -1 &&
>>> +        ((msg >= WM_MOUSEFIRST && msg <= WM_MOUSELAST) ||

(Continue reading)

KO Myung-Hun | 8 Mar 2009 15:12

Re: [PATCH] vo_kva

Hi/2.

Reimar Döffinger wrote:
>>>> +            WinCalcFrameRect(hwnd, &rcl, FALSE);
>>>> +
>>>> +            if (rcl.xRight - rcl.xLeft > vo_screenwidth) {
>>>> +                rcl.xLeft  = 0;
>>>> +                rcl.xRight = vo_screenwidth;
>>>> +            }
>>>> +
>>>> +            if (rcl.yTop - rcl.yBottom > vo_screenheight) {
>>>> +                rcl.yBottom = 0;
>>>> +                rcl.yTop    = vo_screenheight;
>>>> +            }
>>>>         
>>> Are you really sure you want to limit the Window size to the screen
>>> size? The code for Windows specifically disables this limitation because
>>> it can be really annoying for HD videos.
>>>       
>> It's a workaround for T23 laptop with S3 video card. If a movie window size 
>> is larger than a screen size, the image is distorted.
>>     
>
> Well, you already have a flag for that hardware, right? You could use it
> here, too.
>   

Of course.

> Anyway it is your decision, I just wanted to point out that the vos that

(Continue reading)

Reimar Döffinger | 14 Mar 2009 15:34

Picon

Picon

Re: [PATCH] vo_kva

On Sun, Mar 08, 2009 at 11:12:13PM +0900, KO Myung-Hun wrote:
> > Anyway it is your decision, I just wanted to point out that the vos that
> > I maintain will specifically try to avoid that behaviour and your vo is
> > probably going to stand out from the others by behaving differently,
> > though that is not necessarily a bad thing.
> 
> But what you missed is that the frame window procedure is subclassed 
> only if fixt23 is enabled.

Ok, but then a different name or at least a comment indicating it only
applies to T23 workarounds would be good so you know what it is there
for without having to have preinit in your head.

> > A disabled window is a window that can not receive any input, on Windows you
> > use EnableWindow(vo_window, 0); to set that state. This is usually what
> > is used to disable buttons (in addition to graying them out) so I'd
> > expect that to exist on OS/2, too - but I don't know.
> 
> I think, it's WinEnableWindow(). But with quick test, a disabled window 
> does not pass any event to other window. Anyway if I find the solution 
> using this approach, I'll submit another patch later.

Just to clarify: on Windows a disabled window simply will not get input
focus, its parent window will instead, so there is no need to pass the
events on, they will be directed to the parent window directly.

> > Whether the click that moves focus/brings a window on top counts as
> > click into the window as well is a matter of OS policy, too (as evident
> > by Linux window-managers allowing to choose this).
> > So I think my point stands, but as said if it is important to you keep

(Continue reading)

KO Myung-Hun | 14 Mar 2009 17:17

Re: [PATCH] vo_kva

Hi/2.

Reimar Döffinger wrote:
> If Diego is fine with the build system stuff I'd say it can be applied, if
> before or after you send a patch that adds/includes the comments I
> suggested that would be great.
>   

Ok. I added some more comments.

-- 
KO Myung-Hun

Using Mozilla SeaMonkey 1.1.14
Under OS/2 Warp 4 for Korean with FixPak #15
On AMD ThunderBird 1 GHz with 512 MB RAM

Korean OS/2 User Community : http://www.ecomstation.co.kr

Index: Makefile
===================================================================
--- Makefile	(revision 28881)
+++ Makefile	(working copy)
 <at>  <at>  -600,6 +600,7  <at>  <at> 
 SRCS_MPLAYER-$(JACK)         += libao2/ao_jack.c
 SRCS_MPLAYER-$(JOYSTICK)     += input/joystick.c
 SRCS_MPLAYER-$(JPEG)         += libvo/vo_jpeg.c
+SRCS_MPLAYER-$(KVA)          += libvo/vo_kva.c

(Continue reading)

Diego Biurrun | 14 Mar 2009 17:37

Picon

Re: [PATCH] vo_kva

On Sun, Mar 15, 2009 at 01:17:08AM +0900, KO Myung-Hun wrote:
>
> Reimar Döffinger wrote:
>> If Diego is fine with the build system stuff I'd say it can be applied, if
>> before or after you send a patch that adds/includes the comments I
>> suggested that would be great.
>
> Ok. I added some more comments.
>
> --- libvo/vo_kva.c	(revision 0)
> +++ libvo/vo_kva.c	(revision 0)
>  <at>  <at>  -0,0 +1,1086  <at>  <at> 
> +// frame window procesure to workaround for T23 laptop with S3 video card

I don't understand this, what is "procesure"?  It's not an English word..

> +        pti->ptlMinTrackSize.x = rcl.xRight - rcl.xLeft;
> +        pti->ptlMinTrackSize.y = rcl.yTop - rcl.yBottom;

nit: The right-hand-side could be aligned.

> +            pswp->cx  = rcl.xRight - rcl.xLeft;
> +            pswp->cy  = rcl.yTop - rcl.yBottom;

ditto

> +    // if slave mode, ignore mouse events and deliver them to a parent window
> +    if (WinID != -1 &&
> +        ((msg >= WM_MOUSEFIRST && msg <= WM_MOUSELAST) ||
> +         (msg >= WM_EXTMOUSEFIRST && msg <= WM_EXTMOUSELAST))) {

(Continue reading)

KO Myung-Hun | 14 Mar 2009 18:22

Re: [PATCH] vo_kva

Hi/2.

Diego Biurrun wrote:
> On Sun, Mar 15, 2009 at 01:17:08AM +0900, KO Myung-Hun wrote:
>   
>> Reimar Döffinger wrote:
>>     
>>> If Diego is fine with the build system stuff I'd say it can be applied, if
>>> before or after you send a patch that adds/includes the comments I
>>> suggested that would be great.
>>>       
>> Ok. I added some more comments.
>>
>> --- libvo/vo_kva.c	(revision 0)
>> +++ libvo/vo_kva.c	(revision 0)
>>  <at>  <at>  -0,0 +1,1086  <at>  <at> 
>> +// frame window procesure to workaround for T23 laptop with S3 video card
>>     
>
> I don't understand this, what is "procesure"?  It's not an English word..
>
>   

It should be 'procedure'.

> Note that I will not torture you any longer after this, but apply an
> updated patch right away :)
>
>

(Continue reading)

Diego Biurrun | 14 Mar 2009 18:31

Picon

Re: [PATCH] vo_kva

On Sun, Mar 15, 2009 at 02:22:59AM +0900, KO Myung-Hun wrote:
> Hi/2.
>
> Diego Biurrun wrote:
>> On Sun, Mar 15, 2009 at 01:17:08AM +0900, KO Myung-Hun wrote:
>>   
>>> Reimar Döffinger wrote:
>>>     
>>>> If Diego is fine with the build system stuff I'd say it can be applied, if
>>>> before or after you send a patch that adds/includes the comments I
>>>> suggested that would be great.
>>>>       
>>> Ok. I added some more comments.
>>>
>>> --- libvo/vo_kva.c	(revision 0)
>>> +++ libvo/vo_kva.c	(revision 0)
>>>  <at>  <at>  -0,0 +1,1086  <at>  <at> 
>>> +// frame window procesure to workaround for T23 laptop with S3 video card
>>
>> I don't understand this, what is "procesure"?  It's not an English word..
>
> It should be 'procedure'.

OK, so now we have

  // frame window procesure to workaround for T23 laptop with S3 video card

First off, it should be ".. work around T23 ..", but this whole comment
does not yet make much sense.  Work around what?  A laptop is obviously
not what is worked around, there must be some sort of quirk that this

(Continue reading)

KO Myung-Hun | 14 Mar 2009 19:11

Re: [PATCH] vo_kva

Hi/2.

Diego Biurrun wrote:
> OK, so now we have
>
>   // frame window procesure to workaround for T23 laptop with S3 video card
>
> First off, it should be ".. work around T23 ..", but this whole comment
> does not yet make much sense.  Work around what?  A laptop is obviously
> not what is worked around, there must be some sort of quirk that this
> machine displays.  Please be more specific.
>
>   

Ok. Added 'which supports upscaling only.'.

>>> Note that I will not torture you any longer after this, but apply an
>>> updated patch right away :)
>>>       
>> Oh~~~ Thanks. ^________^
>>     
>
> I'm afraid we will have to work out what that comment means first,
> please have a bit more patience... 
>
>   

Ok. Let's go. ^^

--

--

(Continue reading)

Dave Yeo | 15 Mar 2009 06:51

Picon

Re: [PATCH] vo_kva

On 03/14/09 11:11 am, KO Myung-Hun wrote:
[...]
> +    kvaDisableScreenSaver();
[...]
Where is this symbol supposed to reside? Here I get

R:\TMP\ldconv_vo_kva_o_78e449bc936d1d7550.obj(ldconv_vo_kva_o_78e449bc936d1d7550.obj) 
: error LNK2029: "_kvaDisableScreenSaver" : unresolved external

Linking against 
http://hobbes.nmsu.edu/download/pub/os2/dev/mm/libkva-1.0.0.zip.
Dave

KO Myung-Hun | 15 Mar 2009 09:17

Re: [PATCH] vo_kva

Hi/2.

Dave Yeo wrote:
> On 03/14/09 11:11 am, KO Myung-Hun wrote:
> [...]
>   
>> +    kvaDisableScreenSaver();
>>     
> [...]
> Where is this symbol supposed to reside? Here I get
>
> R:\TMP\ldconv_vo_kva_o_78e449bc936d1d7550.obj(ldconv_vo_kva_o_78e449bc936d1d7550.obj) 
> : error LNK2029: "_kvaDisableScreenSaver" : unresolved external
>
> Linking against 
> http://hobbes.nmsu.edu/download/pub/os2/dev/mm/libkva-1.0.0.zip.
>   

It's in a new kva. I'll release it soon.

--

-- 
KO Myung-Hun

Using Mozilla SeaMonkey 1.1.14
Under OS/2 Warp 4 for Korean with FixPak #15
On AMD ThunderBird 1 GHz with 512 MB RAM

Korean OS/2 User Community : http://www.ecomstation.co.kr

Diego Biurrun | 14 Mar 2009 19:45

Picon

Re: [PATCH] vo_kva

On Sun, Mar 15, 2009 at 03:11:15AM +0900, KO Myung-Hun wrote:
>
> Diego Biurrun wrote:
>>>> Note that I will not torture you any longer after this, but apply an
>>>> updated patch right away :)
>>>>       
>>> Oh~~~ Thanks. ^________^
>>
>> I'm afraid we will have to work out what that comment means first,
>> please have a bit more patience... 
>
> Ok. Let's go. ^^

Applied, thanks for being patient.

Diego

KO Myung-Hun | 14 Mar 2009 13:46

Re: [PATCH] vo_kva

Hi/2.

KO Myung-Hun wrote:
> Hi/2.
>
> Reimar Döffinger wrote:
>>>>> +            WinCalcFrameRect(hwnd, &rcl, FALSE);
>>>>> +
>>>>> +            if (rcl.xRight - rcl.xLeft > vo_screenwidth) {
>>>>> +                rcl.xLeft  = 0;
>>>>> +                rcl.xRight = vo_screenwidth;
>>>>> +            }
>>>>> +
>>>>> +            if (rcl.yTop - rcl.yBottom > vo_screenheight) {
>>>>> +                rcl.yBottom = 0;
>>>>> +                rcl.yTop    = vo_screenheight;
>>>>> +            }
>>>>>         
>>>> Are you really sure you want to limit the Window size to the screen
>>>> size? The code for Windows specifically disables this limitation 
>>>> because
>>>> it can be really annoying for HD videos.
>>>>       
>>> It's a workaround for T23 laptop with S3 video card. If a movie 
>>> window size is larger than a screen size, the image is distorted.
>>>     
>>
>> Well, you already have a flag for that hardware, right? You could use it
>> here, too.
>>

(Continue reading)

Diego Biurrun | 14 Mar 2009 14:54

Picon

Re: [PATCH] vo_kva

On Sat, Mar 14, 2009 at 09:46:44PM +0900, KO Myung-Hun wrote:
> [.. tons of useless lines ..]
> 
> No more comments ?

May I make a precedural one?  Please do not senselessly bottom-post.
Snip unnecessary context from your replies.  I had to scroll through
many pages of pointless quoted text until I reached your remark.

Diego

KO Myung-Hun | 14 Mar 2009 17:16

Re: [PATCH] vo_kva

Hi/2.

Diego Biurrun wrote:
> On Sat, Mar 14, 2009 at 09:46:44PM +0900, KO Myung-Hun wrote:
>   
>> [.. tons of useless lines ..]
>>
>> No more comments ?
>>     
>
> May I make a precedural one?  Please do not senselessly bottom-post.
> Snip unnecessary context from your replies.  I had to scroll through
> many pages of pointless quoted text until I reached your remark.
>
>   

Ok.

--

-- 
KO Myung-Hun

Using Mozilla SeaMonkey 1.1.14
Under OS/2 Warp 4 for Korean with FixPak #15
On AMD ThunderBird 1 GHz with 512 MB RAM

Korean OS/2 User Community : http://www.ecomstation.co.kr

Diego Biurrun | 1 Mar 2009 15:41

Picon

Re: [PATCH] vo_kva

On Sun, Mar 01, 2009 at 11:09:49PM +0900, KO Myung-Hun wrote:
> [...]

No more comments from me, Reimar?

Diego

KO Myung-Hun | 7 Mar 2009 14:14

Re: [PATCH] vo_kva

Hi/2.

Diego Biurrun wrote:
> On Sun, Mar 01, 2009 at 11:09:49PM +0900, KO Myung-Hun wrote:
>   
>> [...]
>>     
>
> No more comments from me, Reimar?
>
>   

Ping ?

--

-- 
KO Myung-Hun

Using Mozilla SeaMonkey 1.1.14
Under OS/2 Warp 4 for Korean with FixPak #15
On AMD ThunderBird 1 GHz with 512 MB RAM

Korean OS/2 User Community : http://www.ecomstation.co.kr

Return

Return to gmane.comp.video.mplayer.devel.

Search Archive

Language

Change language

Options

Current view: Threads only / Showing only 20 lines / Not hiding cited text.
Change to All messages, whole messages, or hide cited text.

Post a message
NNTP Newsgroup
Classic Gmane web interface
XML

XML

RSS Feed
List Information

About Gmane