[MPlayer-cvslog] r32478 - in trunk: DOCS/tech/slave.txt command.c input/input.c input/input.h

[Reimar Döffinger]

On Sun, Oct 10, 2010 at 11:30:42AM +0200, cigaes wrote:
> Author: cigaes
> Date: Sun Oct 10 11:30:42 2010
> New Revision: 32478
> 
> Log:
> Add the overlay_add and overlay_remove commands.

Uh, this was never reviewed.

> +static void overlay_add(char *file, int id, int x, int y, unsigned col)
> +{
> +    FILE *f;
> +    unsigned w, h, nc;
> +    unsigned char *data;
> +    struct mp_eosd_image *img;
> +
> +    if (!(f = fopen(file, "r"))) {

I considert
FILE *f = fopen(file, "r");
if (!f)
...
a lot more readable.
This is in addition to the fact that it at least
partially duplicates code in gl_common.c and is
also wrong due to missing "b" in fopen and does
not support comments (and some applications generate
pnm files with comments by default).

> +    data = malloc(w * h);

Possible integer overflow.

> +    if (!overlay_source_registered) {
> +        eosd_register(&overlay_source);
> +        eosd_image_remove_all(&overlay_source);
> +        overlay_source_registered = 1;
> +    }

Is this certain to work correctly when playing multiple
files?
At least some subtitle stuff gets reset on each new file.
If not, does the ASS stuff get correctly reset on a new file?
In general it might be nicer to have a function that checks
if a particular source is already registered.

> +        if ((int)img->opaque == id) {

This will cause a warning on 64 bit.

> +        case MP_CMD_OVERLAY_ADD:
> +        {
> +            overlay_add(cmd->args[0].v.s, cmd->args[1].v.i,
> +                        cmd->args[2].v.i, cmd->args[3].v.i, cmd->args[4].v.i);
> +            break;
> +        }
> +        case MP_CMD_OVERLAY_REMOVE:
> +        {
> +            overlay_remove(cmd->args[0].v.i);
> +            break;
> +        }

Useless {}