[MPlayer-dev-eng] [PATCH] vf_osd: OSD filter for MPlayer

[Oded Shimon]

On Sun, Jul 17, 2005 at 01:16:39PM -0400, Jason Tackaberry wrote:
> Hi,
> 
> The attached patch implements a filter that provides an OSD buffer to an
> application controlling MPlayer in slave mode.  The features and
> interface is similar to the OSD of the Hauppauge PVR-350 and likely
> other hardware.  Taken from the documentation (included in the patch),
> here is a brief feature overview:
> 
> <snip>
> 
> I and the Freevo developers (and users, no doubt!) would quite like to
> see this in MPlayer.

I personally would really like to see this patch applied... It might even 
some day help for DVD Menu!

Some questions/comments:

> +.IPs <width>
> +The width of the OSD buffer in pixels.
> +.IPs <height>
> +The height of the OSD buffer in pixels.

Why don't you just use a buffer the size of the video as given by vf_config?

> +// OSD buffer is available for writing.  vf_osd sets this flag when it is
> +// finished reading from the shared buffer.
> +#define BUFFER_UNLOCKED 0x10

As long as you already made such wonderful comments, consider using 
doxygen. (DOCS/tech/code-documentation.txt)

> +/* Keep track of filter instances for two reasons:
> + *   1. OSD buffer should be able to survive a loadfile, so when the
> + *      filter is initialized, we first check to see if we have an 
> + *      existing filter associated with the specified shared memory key
> + *      and use that instead.
> + *   2. When the movie is paused, we still want to be able to update the
> + *      overlays, so vf_osd_pause_update() needs to be able to find
> + *      the osd instances.
> +*/
> +
> +vf_instance_t** vf_osd = NULL;
> +static struct vf_priv_s **vf_osd_priv = NULL;
> +static mp_image_t *last_mpi = NULL;
> +static int pause_state = 0, num_instances = 0;

I've had this same problem with MEncoder a few months ago - I wanted the 
encoder to stay alive even after a file switch. I did this completely 
differently though - I yanked out the vf (or, actually, ve) before 
vf_uninit() - and re-added it later... (and fail if resolution has changed)
I think this method is better, unfourtunately it would not work as 
gracefully for this. It made sense for MEncoder to remember the ve_, as it 
was the one that added it anyway. For here it's a filter in the middle of 
the vf..

Even though there should be no reason to ever use more than one vf_osd, I 
think it would be best to change this somehow. Also note that nowhere in 
the entire program do you free the mem alloced... This is probably 
intentional, but it is bad...

> +inline void 
> +vf_osd_pause_update(vo_functions_t *video_out) 

Why is this inline? It's not even used in this C file!

> +static int 
> +cmd_filter(mp_cmd_t* cmd, int paused, struct vf_priv_s * priv) 

Regarding these 2 functions - have you seen vf->control? I think it's a 
much better way of doing this.

For every pause, mplayer.c could do
if (sh_video && sh_video->vfilter) ((vf_instance_t*)sh_video->vfilter)->control(VFCTRL_PAUSE_UPDATE);

So then any future filter which will need this "hack" can also use this 
vf->control. (vf_menu should probably do so too)

vf->control could/should also be used for the cmd handling, instead of this 
cmd_filter which I wasn't even aware exsisted...

> +     // Track the pause state.  When the video gets paused, the next
> +     // invocation of put_image will grab the mpi and save it.
> +     if (cmd->id == MP_CMD_PAUSE)
> +             pause_state = !paused;

Why not just keep the last mpi ALWAYS? It's not expensive, it's not even a 
memcpy. It's what vf_harddup does.
Also, I saw on occassion you used the original mpi as the dmpi! AFAIK this 
is incredibly broken, and should never be done in vf_. Even if you have 
nothing to do, you should get_image() and copy the pointers. Have a look at 
vf_harddup.c, it's a good example of a filter that does nothing.

> +            case MP_CMD_VF_OSD:
> +                goto handle_cmd;

Why do you break out of pause mode just because an OSD command was 
passed?... Since OSD still lives even in pause mode, it should not break 
out of it...

Overall very impressive patch.. Like I already said, I'd like to see it 
committed someday...

- ods15