[MPlayer-dev-eng] [PATCH] libvo: GGI driver update

[Reimar Döffinger]

Hi,
> Oh, BTW: In case you wonder a bit about the change in the
> draw_slice() function, this is necessary to silence TOOLS/checktree.sh
> about too long lines - they became too long with the functional change.

That's okay (I think), but why did you move it around?

>      switch (format) {
> -        case IMGFMT_RGB4:
> +    case IMGFMT_RGB4:

cosmetic, later...

> -    ggiCheckMode(ggi_conf.vis, &mode);
> +    ggiSetFlags(ggi_conf.vis, GGIFLAG_ASYNC);
>  
> -    if (ggiSetMode(ggi_conf.vis, &mode) < 0) {
> -        mp_msg(MSGT_VO, MSGL_ERR, "[ggi] unable to set video mode\n");
> -        return (-1);
> -    }
> -    if (ggiGetMode(ggi_conf.vis, &mode) < 0) {
> -        mp_msg(MSGT_VO, MSGL_ERR, "[ggi] unable to get video mode\n");
> -        return (-1);
> +    if (GT_SCHEME(mode.graphtype) == GT_PALETTE)
> +        ggiSetColorfulPalette(ggi_conf.vis);
> +

> -    ggiSetFlags(ggi_conf.vis, GGIFLAG_ASYNC);
> -
> -    if (GT_SCHEME(mode.graphtype) == GT_PALETTE)
> -	ggiSetColorfulPalette(ggi_conf.vis);
> -
> -    if (ggiGetFlags(ggi_conf.vis) & GGIFLAG_ASYNC)
> -	ggi_conf.async = 1;
> -

Why were these moved? and does ggi_conf.async still get set somewhere?

> +    if ((IMGFMT_IS_BGR(mpi->imgfmt) &&
> +        (IMGFMT_BGR_DEPTH(mpi->imgfmt) == vo_dbpp)) ||
> +	(IMGFMT_IS_RGB(mpi->imgfmt) &&
> +        (IMGFMT_RGB_DEPTH(mpi->imgfmt) == vo_dbpp)) &&
> +	((mpi->width == ggi_conf.srcwidth) &&
> +	 (mpi->height == ggi_conf.srcheight))
> +	)
>      {
> -	return (VO_FALSE);
> +	cando_fmt = 1;
>      }
>  
> +    if (!cando_fmt) return (VO_FALSE);
> +

I think you are missing a () in the if above, and anyway it is ugly.
Why not
if (IMGFMT_IS_BGR(mpi->imgfmt) && IMGFMT_BGR_DEPTH(mpi->imgfmt) !=
vo_dbpp)
  return VO_FALSE;
if (IMGFMT_IS_RGB(mpi->imgfmt) && IMGFMT_RGB_DEPTH(mpi->imgfmt) !=
vo_dbpp)
  return VO_FALSE;
etc. ?

> -    if ((char *) arg) {
> +    if (arg != NULL) {
>          int i = 0;
>          ggi_conf.driver = strdup(arg);
> -        while (ggi_conf.driver[i]) {
> +        for (i = 0; ggi_conf.driver[i] != '\0'; i++) {
>              if (ggi_conf.driver[i] == '.')
>                  ggi_conf.driver[i] = ',';
> -            i++;
>          }

ouch. cosmetics. And esp. the while -> for change is not for the better
IMHO.

>      switch (request) {
> -        case VOCTRL_QUERY_FORMAT:
> +    case VOCTRL_QUERY_FORMAT:
same.

That's all for now...

Greetings,
Reimar Döffinger