[MPlayer-dev-eng] [PATCH] VFCTRL_PERIODIC_UPDATE

[Jason Tackaberry]

Hi Uoti,

On Thu, 2007-01-11 at 05:58 +0200, Uoti Urpala wrote:
> Most of the changes related to this seem quite stupid:

There are a couple mistakes, but that statement is harsh, and if it sets
the tone of the review process (and this patch is by far the smallest
one), I should probably quit now while I'm ahead. :)


> +static int periodic_update(vf_instance_t *vf, vo_functions_t *vo,
> +                            float time_avail) {
> +   int res;
> +   if (!vf) return 0;
> +   if (time_avail < 0.01) return 1;
> 
> So here updates are skipped if there's less than 10 ms free time between
> decoding a frame and showing it. Maybe sensible, but note that this
> disables updates completely if for example 50 FPS video takes more than
> 50% CPU. Also the return value 1 is probably wrong given the rest of the
> code below.

If there's less than 10ms free, the filter will receive the frame soon
enough through the normal mechanism, so there is no need to send it the
periodic update vfctrl.

> 
> -           usec_sleep(1000000 * (time_frame - margin));
> +           int mul = 100 * !periodic_update(sh_video->vfilter, video_out, time_frame);
> +           time_frame-=GetRelativeTime();
> +           usec_sleep(10000 * mul * (time_frame - margin));
> 
> This makes no sense. First there's an obvious bug: mul can become 0
> which is obviously wrong. I assume you meant 1 in this case, though the

Yes, that's wrong.  Obviously mul becoming 0 is not good. 

> code is still stupid even with that change. Why is the interval until
> periodic_update proportional to the free time? And because of the check
> and return value in periodic_update above, when there's 10 ms free this
> wakes up to do nothing in periodic_update, tries to sleep 0.1 ms
> (probably resulting in 1 ms sleep) and when waking up again does nothing
> in periodic_update and so on...

The general idea behind this code is to allow the overlay to update at a
reasonably high framerate (30ish fps or so, when you factor in
overhead).  The sleep_timer is obviously where mplayer sleeps the
longest while waiting to deliver the next frame.  With softsleep
disabled, it sleeps nearly the entire duration.

So if the vfctrl is consumed, it means a filter wants to update the
overlay, and we want to sleep for some fraction of the frame duration
instead.  Although it may be better of to just sleep for some fixed
small amount, say 10ms, in that case.

Another goal was to not affect the sleep times in the case when the
vfctrl isn't consumed.  Even though the code is broken (when the vfctrl
is consumed, mul is 0, and it ends up not sleeping at all), it does
achieve at least this goal. :)

What would you say about:

        if (periodic_update(sh_video->vfilter, video_out, time_frame))
           usec_sleep(10000);
        else
           usec_sleep(1000000 * (time_frame - margin));


> -    while ( (cmd = mp_input_get_cmd(20, 1, 1)) == NULL) {
> +    while ( (cmd = mp_input_get_cmd(3, 1, 1)) == NULL) {
> 
> +       if (sh_video && periodic_update(sh_video->vfilter, video_out, 1))
> +          usec_sleep(1000);
> +       else
>         usec_sleep(20000);
> 
> The original sleeps were quite short already. Do you really have a valid
> reason to make them shorter? 

40ms is too long when the idea is to let the overlay (or generally
speaking any consumer of VFCTRL_PERIODIC_UPDATE) with a reasonable
framerate (30fps or so).

Changing the time passed to mp_input_get_cmd() may not be necessary in
my case, since vf_overlay is controlled via slave commands, so when one
is issued it will wake up.  Although it may be case that some other
filter would want to update the mpi for some other reason, and 20ms is
cutting it a bit close.

3 is admittedly low.  So would it be more acceptable to change
mp_input_get_cmd time to 10, and only sleep (for the current 20ms) if
the periodic update vfctrl isn't consumed?


> If you're trying to show video with good timing in the overlay then
> there should be a proper timing mechanism for it instead;

That is a goal, yes.  What would you suggest as a proper timing
mechanism?


> Also in this change
> 
>         current_module="sleep_rtc";
>          while (time_frame > 0.000) {
>             unsigned long rtc_ts;
> +           periodic_update(sh_video->vfilter, video_out, time_frame);
> +           time_frame-=GetRelativeTime();
>             if (read(rtc_fd, &rtc_ts, sizeof(rtc_ts)) <= 0)
>                 mp_msg(MSGT_CPLAYER, MSGL_ERR, MSGTR_LinuxRTCReadError, strerror(errno));
>             time_frame -= GetRelativeTime();
> 
> there's no reason to add another "time_frame -= GetRelativeTime();" line.

True.  I will remove it.


>From your later email ...

> What's the intended use case for the filter?

For my purposes, the intended use-case of VFCTRL_PERIODIC_UPDATE is
vf_overlay, which I described in my original email.  The idea is to
provide a generic mechanism for filters to update the mpi while paused
and not be locked to the video's framerate.  vf_menu could use this as
well.

But basically any controlling application that wants to overlay an
arbitrary image (with alpha blending) over a video will benefit from
vf_overlay.  Another goal is to provide picture-in-picture.  We are
using it in the Freevo project.

VFCTRL_PERIODIC_UPDATE was suggested by Remar late 2005 after an earlier
attempt to submit vf_overlay.  Originally I was using an approach rather
similar to vf_menu, where I had hardcoded a callback to handle updates.
Remar suggested and coded this more generic approach.


> Well, quite bad for what? For displaying smooth video or low-latency
> interactive content maybe, but if those are desired then IMO the
> implementation should not be polling-based to begin with (and possibly
> the whole idea of using slave mode from another process is
> questionable for those).

Maybe not.  But the idea here is to get this functionality with minimal
disruption to mplayer's core.  It's easy to say that if shouldn't be
polling-based, but I suspect to do it "the right" way would require a
far more intrusive change that have even less chance of getting
merged.  

mplayer's architecture isn't exactly accommodating, and I think the
VFCTRL_PERIODIC_UPDATE approach is a pretty good compromise to be
minimally intrusive while accomplishing the desired results, yet not
measurably affecting the common case (with the single exception of the
sleep time passed to mp_input_get_cmd() which is only called when
mplayer is paused).


>  The values in the patch gave 3+1 ms sleeps in the pause loop, meaning
> 250 loops and 500 wakeups per second (maybe 333 in practice) which is
> definitely way too much.

That's only when the vfctrl is consumed.  When it isn't, it would be 3
+20.  But see my above suggestion, which would make it 10+20.

Thanks for reviewing this code.  Let me know what you think about my
suggestions.

Cheers,
Jason.