[MPlayer-dev-eng] [PATCH] set_geometry command in slave mode

[Oded Shimon]

On Sun, Sep 18, 2005 at 02:50:39PM +0530, Ajith Peter wrote:
> Hi,
> 
> Attaching herewith is a patch that enables the -geometry option to be
> used with slave mode as a command set_geometry.  Coded and Tested only
> on X11/Linux.

Well, it's half good...
First comment - try to make your mailer send simple text attachments, not 
base64, it makes mailers not include the patch in the reply.

> Index: mplayer.c
> ===================================================================
> RCS file: /cvsroot/mplayer/main/mplayer.c,v
> retrieving revision 1.871
> diff -u -r1.871 mplayer.c
> --- mplayer.c	15 Sep 2005 03:08:35 -0000	1.871
> +++ mplayer.c	18 Sep 2005 09:14:04 -0000
> @@ -303,6 +303,11 @@
>  extern char *vo_subdevice;
>  extern char *ao_subdevice;
>  
> +// geometry information
> +
> +int geom_width = 0;
> +int geom_height = 0;

These global variables (or semi global, main() is pretty huge) are useless.
Better just put them in the switch as nice temporary vars.

>  // codec outfmt flags (defined in libmpcodecs/vd.c)
>  extern int vo_flags;
>  
> @@ -2885,6 +2890,12 @@
>  	movie_aspect = cmd->args[0].v.f;
>        mpcodecs_config_vo (sh_video, sh_video->disp_w, sh_video->disp_h, 0);
>      } break;
> +    case MP_CMD_SET_GEOMETRY : {

...As in, over here...

> +      vo_geometry = cmd->args[0].v.s;       
> +      //FIX-ME:better screen width and height
> +      if(geometry(&vo_dx, &vo_dy, &geom_width, &geom_height, 1000, 1000))
> +       vo_x11_nofs_sizepos(vo_dx, vo_dy, geom_width, geom_height);

You should use a voctrl, not depend on vo_x11 command, think of MPlayer 
compiled on windows with x11 support, this would fail to compile. Besides, 
not all vo's are x11. the video_out 'control' function allows each video to 
implement it's own method of doing a certain action, like switching to full 
screen or in this case, changing window size.
Have a look in this switch statement at how sitching to full screen is 
done. it should be using a VOCTRL.
The way you implement this is by, in libvo/vo_xv.c, only there you put the 
'vo_x11_nofs_sizepos', that way this "per vo" code is right in its proper 
place - per vo. you should have no trouble implementing this function for 
vo_xv, vo_x11, vo_xvidix, vo_gl and vo_gl2.. other might be trickier, but 
that's optional, for now just vo_xv would be enough if you're particularly 
lazy.

> +    } break;
>      case MP_CMD_AUDIO_DELAY : {
>        float v = cmd->args[0].v.f;
>        audio_delay += v;
> Index: DOCS/tech/slave.txt
> ===================================================================
> RCS file: /cvsroot/mplayer/main/DOCS/tech/slave.txt,v
> retrieving revision 1.46
> diff -u -r1.46 slave.txt
> --- DOCS/tech/slave.txt	13 Sep 2005 18:48:57 -0000	1.46
> +++ DOCS/tech/slave.txt	18 Sep 2005 09:14:07 -0000
> @@ -159,6 +159,10 @@
>          1 is a seek to <value> % in the movie.
>          2 is a seek to an absolute position of <value> seconds.
>  
> +set_geometry <geometry string>
> +    Set the geometry of the window. Refer to man page for information
> +    on the geometry string.  Currently supported for x11 only.
> +
>  speed_incr <value>
>      Add <value> to the current playback speed.
>  
> Index: input/input.c
> ===================================================================
> RCS file: /cvsroot/mplayer/main/input/input.c,v
> retrieving revision 1.122
> diff -u -r1.122 input.c
> --- input/input.c	15 Sep 2005 10:25:43 -0000	1.122
> +++ input/input.c	18 Sep 2005 09:14:15 -0000
> @@ -142,6 +142,7 @@
>    { MP_CMD_GET_VO_FULLSCREEN, "get_vo_fullscreen", 0, { {-1,{0}} } },
>    { MP_CMD_GET_SUB_VISIBILITY, "get_sub_visibility", 0, { {-1,{0}} } },
>    { MP_CMD_KEYDOWN_EVENTS, "key_down_event", 1, { {MP_CMD_ARG_INT,{0}}, {-1,{0}} } },
> +  { MP_CMD_SET_GEOMETRY, "set_geometry", 1, {{MP_CMD_ARG_STRING, {0}}, {-1,{0}} } },
>    
>    { 0, NULL, 0, {} }
>  };
> Index: input/input.h
> ===================================================================
> RCS file: /cvsroot/mplayer/main/input/input.h,v
> retrieving revision 1.58
> diff -u -r1.58 input.h
> --- input/input.h	31 Aug 2005 02:15:02 -0000	1.58
> +++ input/input.h	18 Sep 2005 09:14:16 -0000
> @@ -93,6 +93,9 @@
>  #define MP_CMD_DVDNAV_MENU      5
>  #define MP_CMD_DVDNAV_SELECT    6
>  
> +// Command for setting geometry by slave mode
> +#define MP_CMD_SET_GEOMETRY 150001

Is there any reason for such a high number? this is a pretty "basic" 
command, it should use the number 65 or so, which is very next cmd number 
if i'm not mistaken.

Don't be discouraged, I told you, it's a process, you send patch, we flame, 
you fix. :)

- ods15