[MPlayer-dev-eng] [PATCH] vf_osd take two

[Michael Niedermayer]

Hi

a very quick and partial review ...

On Tue, Sep 06, 2005 at 04:54:20PM -0400, Jason Tackaberry wrote:
[...]
> +Internally, vf_osd maintains two buffers.  The first buffer is the shared
> +BGRA buffer, which the application will render to.  The second buffer is a
> +planar YUVA (YV12 plus alpha channel).  The "YV12A" buffer is what is read
> +by vf_osd when compositing.  Therefore an application writing to the shared
> +BGRA buffer isn't sufficient for the OSD to be updated.  The application
> +must follow up with an "osdcmd invalidate" slave command to cause vf_osd to
> +synchronize its internal YV12A buffer with the BGRA buffer.

if i understand that correctly the app sends BGRA to vf_osd which converts it
to YV12A, isnt it (optionally) possible to send YV12A directly?


[...]

> --- main.orig/libmpcodecs/vf_osd.c	1969-12-31 19:00:00.000000000 -0500
> +++ main/libmpcodecs/vf_osd.c	2005-09-06 15:42:09.000000000 -0400
> @@ -0,0 +1,1361 @@
> +/* Copyright 2005 Jason Tackaberry <tack at sault.org>
> + *
> + * Licenced under the GNU General Public License

i dont want to be picky but which version? :) ==v2 or >=v2?


[...]
> +
> +
> +/// Per-instance private data.
> +struct vf_priv_s {
> +    /** Memory allocated by alloc_osd_data() for the YV12 converted state of
> +     *  the OSD image.  The base pointer is stored in 'buffer' and the other
> +     *  variables point to locations within the single buffer.  y, u, and v
> +     *  hold the luma and chroma planes, and a and uva hold the alpha planes
> +     *  (for luma and chroma respectively).  The pre_* variables hold the
> +     *  pre-alpha-multiplied versions of the above.
> +     */
> +    unsigned char *buffer, *y, *u, *v, *a, *uva,
> +                  *pre_y, *pre_u, *pre_v, *pre_a, *pre_uva;
> +    /** Lockbyte points to the first byte of the shared memory buffer which
> +      * is used for synchronization.  See BUFFER_* flags above.  bgra_imgbuf
> +      * points to the beginning of the BGRA image in shared memory (which is
> +      * simply (lockbyte+1).
> +      */
> +    unsigned char *lockbyte, *bgra_imgbuf, *bgra_imgbuf_scaled;

hmm, does doxygen support such comments for several variables at once?


[...]
> +    /** Respectively: the width (w) and height (h) of the OSD BGRA buffer;
> +        whether the size of the OSD is fixed between loadfiles (osd_fixed);
> +        the top (slice_y) and height (slice_h) indicating the region of the
> +        OSD which will ultimately be composited with the video; the
> +        "global" alpha level of the OSD (alpha, 0 <= alpha <= 256); whether
> +        or not the OSD is visible (visible, 0=hidden 1=visible); whether or
> +        not the OSD has changed (dirty).
> +      */
> +    int w, h, osd_fixed, slice_y, slice_h, alpha, visible, dirty;

i really think this should be split in a few comments+variables, even if
doxygen supports this which iam not sure if it does


[...]
> +
> +/** Keep track of filter instances for two reasons:
> + *   1. OSD buffer should be able to survive a loadfile or loop, 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 pause_update() needs to be able to find
> + *      the osd instances.
> + *
> + * As a result of the above, vf_osd instances are "persistent" (i.e. they
> + * don't get uninitialized).  Consequently, the global variables below apply
> + * to all vf_osd instances.
> + */
> +
> +vf_instance_t** vf_osd = NULL;
> +static struct vf_priv_s **vf_osd_priv = NULL;
> +/// The last mpi that was given to put_image().
> +static mp_image_t *last_mpi = NULL;
> +static int is_paused = 0,   ///< Video pause state: 0=playing, 1=paused.
> +           num_instances = 0; ///< number of vf_osd instances.

i still dislike these globals somehow but iam not objecting against applying
it if the other devels think its ok


[...]
> +            asm volatile(
> +                ".balign 16 \n\t"
> +                "movl %4, %%ecx\n\t"
> +
> +                "1: \n\t"
> +                "movq (%1), %%mm0\n\t"        // %mm0 = mpi
[...]
> +            : "=r" (dst),
> +              "=r" (src),
> +              "=r" (osd),
> +              "=r" (alpha)
> +            : "m" (c) 
> +            : "%ecx", "memory");

%1 is a output operand so you can read & write to it but its initial value
doesnt have to be src
"+r" (src) would be input+output
"=r"(src) : "1"(src) should work too

[...]
-- 
Michael