[MPlayer-dev-eng] [PATCH] vf_osd updates - fully baked?

[Reimar Döffinger]

Hi,

I'll be asking a lot of questions. That does not meant that I think it
is wrong, it's just the things I think you should look at again to make
sure it actually is right... There is no chance I will understand all the
code in the short time I have looked at it.

On Sun, Sep 11, 2005 at 05:33:51PM -0400, Jason Tackaberry wrote:
> +/// \name Convenience macros.
> +//@{
> +#define min(a, b) ((a<b) ? a : b)
> +#define max(a, b) ((a>b) ? a : b)

I don't like that two much since they usually tend to end up being
declared multiple time. For consistency at least use MIN and MAX I'd
say...
I somehow missed to comment on the place where you used them, it looked
extremely obfuscated to me. Maybe the good old chain of ifs isn't that
bad...

> +#define FALSE 0
> +#define TRUE !FALSE

I really _hate_ it when people use defines for things like that. Why
isn't just using 0 and 1 good enough? I think at least every C programmer
knows that 1 is true and 0 is false...

> +    /** Lockbyte points to the first byte of the shared memory buffer which
> +     *  is used for synchronization. See \a BUFFER_LOCKED and \a
> +     *  BUFFER_UNLOCKED  flags above.
> +     */
> +    unsigned char *lockbyte;

I guess I should have a closer look at this code somewhen. Most of the
"locking" code I have seen just didn't work...

> +    /** Points to the beginning of the BGRA image in shared memory (which
> +     *  is simply (lockbyte+1).

I do not like this. Doesn't this mean that the bgra samples are
unaligned? That can create serious problems, esp. on Sparc...
Preferably any larger data should be 16 byte aligned, since that is the
only values that is sure to create no problems - even when using SSE.

> +/** \name Macros and tables for (internal) colorspace conversion.
> + *  \see convert_bgra_to_yv12a() 
> + */
> +//@{
> +#define FP_BITS 18
> +#define rgb2y(R,G,B) ( (Y_R[ R ] + Y_G[ G ] + Y_B[ B ]) >> FP_BITS )
> +#define rgb2u(R,G,B) ( (U_R[ R ] + U_G[ G ] + U_B[ B ]) >> FP_BITS )
> +#define rgb2v(R,G,B) ( (V_R[ R ] + V_G[ G ] + V_B[ B ]) >> FP_BITS )

This conversion does not fit the ones used in other places in MPlayer
(see DOCS/tech/colorspaces.txt).
I read the reason why you don't use swscaler, but I don't really agree,
since not using it will make it more difficult to support more formats
later on (in case somebody cares).
Also after extracting the alpha to a seperate plane you should be able
to scale it by using the scaler for IMGFMT_Y800.

> +#ifdef STOPWATCH
> +/**
> + * \brief Simple timer for profiling and debugging.

Hmm... at least for x86 libavutil/common.h has the much nocer and
precise START_TIMER and STOP_TIME macros. I think they would be
preferable if they work for you.

> + * Because vf_osd instances must survive a loadfile or loop, vf_uninit is not
> + * specified. Therefore, when the first vf_osd instance is created, this
> + * function is registered with atexit(3), so that the shared memory segment

All I heard indicates that atexit creates a lot of problems. Also I do
not see much point, the OS should free any associated resources on exit
(or is that not the case for shared memory?)

> +    gettimeofday(&curtime, &tz);

You should definitly use the functions from osdep/timer.h

> +    priv->shm_id = shmget(priv->shm_key, bufsize, IPC_CREAT | 0600);

In relation with the atexit problem, it would also be possible to let
another application create the shared memory segment - if it's a good
idea depends of course on what you want to use it for.

> + * put_image), but the BGR32->BGR32 scaler doesn't scale the alpha channel.

As I mentioned before, since you have to seperate out the alpha in an
extra plane anyway, you can first do that and then scale. I think. Btw.
the swscaler can do the conversion and scaling in one step AFAIK.

> +static void
> +invalidate_rect(struct vf_priv_s *priv, int x, int y, int w, int h)
[...]
> +    last = priv->invalid_rects;

Is priv->invalid_rects always != NULL?

> +    while (last->next)
> +        last = last->next;
> +    last->next = r;

Since you don't seem to keep any specific order, why not just add it in
the front with O(1) instead of at the end with O(n)?

> +/// Blends src on top of dst at the given alpha level.
> +#define blend_byte(dst, src, alpha) alpha == 255 ? src : multiply_alpha(dst, 255 - alpha) + src;

If this function is used often, you should consider doing alpha = 255
- alpha; somewhere in the preparation step.

> +        "movl %4, %%eax\n\t"          // %eax = layer alpha
> +        "cmpl $255, %%eax\n\t"        // don't apply layer alpha if it's 100% opaque
"mov %4, %%"REG_a"\n\t"          // %eax = layer alpha
"cmp $255, %%"REG_a"eax\n\t"        // don't apply layer alpha if it's 100% opaque

and include (I think) cpudetect.h. Otherwise it won't work on AMD64.

> +    :  "%eax");
: "%"REG_a);

> +                "movl %4, %%ecx\n\t"

Same here with REG_c

> +static void **
> +grow_array(void **old_array, int old_size, int diff)
> +{
> +    void **tmp = (void **)calloc(1, sizeof(void *) * (old_size + diff));
> +    memcpy(tmp, old_array, sizeof(void *) * old_size);
> +    if (old_array)
> +        free(old_array);
> +    return tmp;
> +}

how about just using realloc??

> +    if (cmd->id == MP_CMD_VF_OSD) {
> +        /* This parsing code is not robust. Passing a malformed argument
> +         * will probably result in a segfault. But this is not atypical
> +         * for MPlayer. :)
> +         */

Maybe you can use the subopt helper code? See subopt-helper.[ch]. Well,
if you don't mind using ':' as seperator instead of ','.
See e.g. vo_gl.c, preinit function for an example of how to use it.

> -      while( (cmd = mp_input_get_cmd(20,1,1)) == NULL) {
> +      while( (cmd = mp_input_get_cmd(3,1,1)) == NULL) {

please keep the value as high as possible, I think esp. with gmplayer a
change here can increase CPU load in paused mode a lot.

> -             usec_sleep(20000);
> +	     if (sh_video && sh_video->vfilter) 
> +	       ((vf_instance_t*)sh_video->vfilter)->control(sh_video->vfilter, VFCTRL_PAUSE_UPDATE, video_out);
> +	     usec_sleep(1000);

same here. Maybe you should check the return value, if it was not uses
(e.g. returns IMPLEM), sleep longer. Not sure if it has a return value
at all though...

> -      if (cmd && cmd->id == MP_CMD_PAUSE) {
> -      cmd = mp_input_get_cmd(0,1,0);
> -      mp_cmd_free(cmd);
> +      switch (cmd->id) {
> +            // Handle commands while paused.
> +            case MP_CMD_LOADFILE:
> +                cmd = mp_input_get_cmd(0,1,0);
> +                mp_input_queue_cmd(cmd);
> +                goto handle_cmd;

I don't like that goto here...

> +            case MP_CMD_PAUSE:
> +                cmd = mp_input_get_cmd(0,1,0);
> +                mp_cmd_free(cmd);
>        }
> +

Shouldn't this be a seperate patch anyway?

> @@ -142,6 +142,11 @@
>    { 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}} } },
> +
> +#ifdef HAVE_SHM
> +  { MP_CMD_VF_OSD, "osdcmd", 1, { {MP_CMD_ARG_STRING,{0}}, {-1,{0}}}},
> +#endif
> +
>    
remove the additional empty lines IMHO.

Greetings,
Reimar Döffinger