Another video capturing problem

[ojwb]

This seems to be related to #47 - at least git bisect pins the blame on the same commit (1565081),

The tmp.gif animation produced by these commands should be a fairly clean 3 second loop counting from 0 to 14 (and it was before that commit) but now it's mostly stuck on one frame with a brief glitch to some others:

$ ./build_headless_opt.sh
$ perl -e 'print "\r\0\n\x22\xeb2:\xf5\xe3A=0\xb814:\xf1;A:\xe3D=1\xb8360:\xed,:\xfd0\r\xff"'> tmp.bas
$ perl -e 'print "RUN\r"' > keys.bin
$ rm beebjit_frame_*
$ ./beebjit -fast -headless -cycles 43216000000 -opt video:paint-start-cycles=21600000000,video:border-chars=0 -frame-cycles 1 -max-frames 150 -exit-on-max-frames -commands 'breakat 725000;c;loadmem tmp.bas 1900;loadmem keys.bin 03e0;writem 02e1 e4;writem 0000 24;writem 0001 19;writem 0002 24;writem 0003 19;writem 0012 24;writem 0013 19;breakat 21608000000;c'
$ ffmpeg -hide_banner -loglevel panic -y -f image2 -r 50 -s 640x512 -pix_fmt bgra -vcodec rawvideo -i beebjit_frame_%d.bgra tmp.gif

(The program here is: 10MO.2:REP.F.A=0TO14:P.;A:F.D=1TO360:N.,:U.0)

Neither my patch in #47 nor the cleaner fix you applied instead seems to make a difference in this case.

[scarybeasts]

Interesting. This command line uses -fast without -accurate, which uses a different paint path.

I think this might be a simple-ish fix: in video_paint_timer_fired(), if you move the line "p_video->has_paint_timer_triggered = 1;" outside of all "if" statements, does that work without any side-effects?

[scarybeasts]

(Also, it should in theory be possible to remove the -cycles command line parameter and rely on the -exit-on-max-frames to handle the exit.)

[ojwb]

I think this might be a simple-ish fix: in video_paint_timer_fired(), if you move the line "p_video->has_paint_timer_triggered = 1;" outside of all "if" statements, does that work without any side-effects?

It fixes my reduced testcase and my original testcase and it doesn't break BBCMicroBot's testsuite (though that's not all that thorough - for example, it doesn't currently test "clapperboard" mode, so didn't catch this breakage).

It looks like it probably doesn't make a difference exactly where this line gets moved to, but FWIW this is the exact patch I tested with:

diff --git a/video.c b/video.c
index 01c7d7e6..0245b052 100644
--- a/video.c
+++ b/video.c
@@ -1259,12 +1259,12 @@ video_paint_timer_fired(void* p) {
                timing_get_total_timer_ticks(p_video->p_timing));
   }
 
+  p_video->has_paint_timer_triggered = 1;
   if (!p_video->externally_clocked) {
     /* For accurate mode, this triggers the start of painting, and paint will
      * occur at the usual 50Hz virtual time.
      */
     if (timing_timer_is_running(p_timing, timer_id)) {
-      p_video->has_paint_timer_triggered = 1;
       video_wall_time_vsync_hit(p_video);
       /* A bunch of unconsumed framing changes will have built up. Clear the
        * the flag so the screen isn't immediately cleared after the first paint.

[ojwb]

It looks the fix above hasn't been merged.

However trying to verify this, my testcase in the first comment no longer seems to function properly - I just get a mostly static MODE7 screen with the startup message followed by >RUN and >, with a slight vertical glitch in the GIF loop (and with the fix applied it's the same).

If I repeat the test with acf0b96 (which is close to where master was when I reported this) then the testcase does function so this seems to be a beebjit change.