[MPlayer-dev-eng] [PATCH] VFCTRL_PERIODIC_UPDATE

[Uoti Urpala]

On Wed, 2007-01-10 at 20:35 -0500, Jason Tackaberry wrote:
> Because this allows updating the vo during the sleep loops, if the
> vfctrl is consumed, it sleeps for smaller intervals to allow more
> frequent updating.  In the case where the vctrl isn't consumed (the

Most of the changes related to this seem quite stupid:

+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.

-           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
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...

-    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? If you're trying to show video with good
timing in the overlay then there should be a proper timing mechanism for
it instead; if you aren't trying to do that then a couple dozen ms here
shouldn't matter. Your values here are also shorter than the 10 ms
"sanity check" in periodic_update.



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.