[MPlayer-dev-eng] [PATCH] Refreshed OS/2 Patches

[Reimar Döffinger]

Hello,
On Tue, Jul 17, 2007 at 10:41:00PM +0900, KO Myung-Hun wrote:
> I've splitted patches into 3 parts. This is 3/3, which are newly created
> files.

I happen to have a OS/2 VM lying around, do you happen to have a link to
a development environment lying around? I guess I could help speed this
up a bit by testing, but esp. with ffmpeg I have the problem there I
can't even find a single POSIX-compatible shell to run configure.

> +            ao_control_vol_t *vol = ( ao_control_vol_t * )arg;

Useless cast.

> +    BOOL    fNoShare = FALSE;
> +    int     n;
> +
> +    opt_t subopts[] = {
> +        {"noshare", OPT_ARG_BOOL, &fNoShare, NULL },

OPT_ARG_BOOL takes an int as parameter. Most likely BOOL will happen to
be the same, but it is still the wrong type.

[...]
> +#define COLOR_OVERLAY   0x000008

I don't know if it has the right format, but MPlayer already has
vo_colorkey that allows to specify which colour to use for the overlay.

> +#define SRC_WIDTH   m_int.kvas.szlSrcSize.cx
> +#define SRC_HEIGHT  m_int.kvas.szlSrcSize.cy
> +
> +#define SCREEN_WIDTH    m_int.kvac.cxScreen
> +#define SCREEN_HEIGHT   m_int.kvac.cyScreen

I think the other vos use vo_screenwidth/vo_screenheight and vo_dwidth
and vo_dheight for this.

> +typedef struct tagVOKVAINTERNAL
> +{
> +    HAB         hab;
> +    HMQ         hmq;
> +    HWND        hwndFrame;
> +    HWND        hwndClient;
> +    HWND        hwndSysMenu;
> +    HWND        hwndTitleBar;
> +    HWND        hwndMinMax;
> +    BOOL        fFullScreen;
> +    FOURCC      fcc;
> +    INT         iImageFormat;
> +    KVASETUP    kvas;
> +    KVACAPS     kvac;
> +    RECTL       rclDst;
> +    INT         bpp;
> +    LONG        lStride;
> +    PBYTE       pbImage;
> +    BOOL        fFixT23;
> +    PFNWP       pfnwpOldFrame;
> +    uint8_t    *yv12Planes[ 3 ];    // use as YUV420P format unlike name
> +    int         yv12Stride[ 3 ];
> +    struct SwsContext *sws;
> +} VOKVAINTERNAL, *PVOKVAINTERNAL;

PVOKVAINTERNAL seems to be unused, so remove it. I personally also think
that all-uppercase struct names aren't a good idea, but your decision.

> +static VOID imgFree( PBYTE pbImage )
> +{
> +    if( pbImage )
> +        free( pbImage );
> +}

Seems like overkill to have a whole function for that, even more so
since the if should not really be needed, free is supposed to handle
NULL just fine.

> +static VOID imgDisplay( PBYTE pbImage )
> +{
> +    PVOID       pBuffer;
> +    ULONG       ulBPL;
> +
> +    if( !kvaLockBuffer( &pBuffer, &ulBPL ))
> +    {
> +        if( m_int.iImageFormat == IMGFMT_YV12 )
> +        {
> +            sws_scale( m_int.sws, m_int.yv12Planes, m_int.yv12Stride, 0, SRC_HEIGHT,
> +                       &pBuffer, &ulBPL );
> +
> +            if( gCpuCaps.hasMMX )
> +                asm volatile ("emms\n\t");

Is this really needed? I see that vf_scale does something like this, but
only init, and even there it is a bug IMO.

> +#define MRETURN( ret )  return ( MRESULT )( ret )

I find that really obfuscated, I think it would be better to write it
out.
If you want to avoid the cast, you could also do it by e.g.
#define MTRUE ((MRESULT)TRUE)

and then return MTRUE.

> +        case WM_CLOSE :
> +            mplayer_put_key( KEY_CLOSE_WIN );
> +
> +            MRETURN( 0 );

This is the only place where 0 and not TRUE or FALSE is used. Is that
intentional?

> +static int preinit( const char *arg )
> +{
> +    ULONG   flFrameFlags;
> +
> +    BOOL    fUseDive = FALSE;
> +    BOOL    fFixT23 = FALSE;
> +
> +    opt_t subopts[] = {
> +        {"dive", OPT_ARG_BOOL,  &fUseDive, NULL, 0 },
> +        {"t23", OPT_ARG_BOOL, &fFixT23, NULL, 0 },
> +        { NULL, 0, NULL, NULL, 0 }
> +    };

Same problem with BOOL type as for the ao.

> +    // add 1 to adjust range
> +    *value = (( ulValue + 1 ) * 200 / 255 ) - 100;

*value = ( ulValue + 1 ) * 200 / 255 - 100;

The outer () is pointless.

> +            m_int.fFullScreen = !m_int.fFullScreen;

I think the other vos use the vo_fs variable for this.
I haven't yet checked if the code handles -fixed-vo, -fs and -dr
commandline options in a sane way.

> +// FIXME : assume time == 0
> +int getch2( int time )

Not handling time can cause massive CPU use due to constantly checking
for input.
Maybe you can fix it like this, instead of:

> +    if( !getch2_status )
> +        return -1;
> +
> +    if( KbdCharIn( &kki, IO_NOWAIT, 0 ))
> +        return -1;

Do

if( !getch2_status ) {
  DosSleep(time);
  return -1;
}

i = time / 10;
while (KbdCharIn( &kki, IO_NOWAIT, 0 )) {
  if (i-- <= 0) return -1;
  DosSleep(10);
}

Greetings,
Reimar Döffinger