GNOME Bugzilla – Bug 740557
videotestsrc: add ball motion based on system clock
Last modified: 2017-04-12 13:33:07 UTC
video_test_src_ball moves around in a flowy pattern that is nice to look at. (personally I don't want to get rid of that, see below.) in a nutshell: gettimeofday (&tv, NULL); rad = 1 * G_PI * tv.tv_usec/1000000 ; x = w/2 + sin(rad) * (radius); The ball revolves around the center of the frame, one revolution per second, or 1/2 a revolution then jumps back to the start as the second increases. Adding clock seconds: clockoverlay time-format="%S" font-desc="Sans 240" makes a very nice way of testing sync that I need for a gstreamer app I am working on. I stepped on the original ball behaviour code because much of the function was unchanged and I didn't want to cut/paste big hunks of code, nor get tangled up in how to abstract out the common code into it's own function. I would like some guidance on how to address this, both what direction to go (ignore previous behaviour, copy the common code, put the common code in a function (where does that go?) add a parameter that defines what ball behaviour to use...) or if someone else wants to just do it, that would be fine with me.
Created attachment 291277 [details] [review] working linted patch
Review of attachment 291277 [details] [review]: I'm not sure it's such a great idea to change an existing test effect. Creating new one would be better from my point of view. The second thing is that this is test patterns, most of them are taken from standards or are designed to test specific things. They are not designed to please human eye. What are you testing with this ? ::: gst/videotestsrc/videotestsrc.c @@ +1073,3 @@ + struct timeval tv; + + gettimeofday (&tv, NULL); First, gettimeofday() should never be used for animation on a computer. Think of what will hapen if you switch timezone, or the jittery cause by NTP deamon adjusting the clock. Second, in GStreamer we use GStreamer clock, in this case I think time information is already in GstVideoFrame somewhere.
I can agree with adding. Now I need advice on implementation: Should I add another pattern # to the end of the list: (18): ball - Moving ball, ... (21): pinwheel - Pinwheel (22): sweep - 1 cycle per second or make it an additional property that effects -pattern 18 behaviour? videotestsrc pattern=18 motion=0 (0=original wavy, 1=sweep) (I like additional property better) Use case: observing lag between different displays of what should be the same video stream. gst-switch has multiple paths of the same source to the same UI window: thumb nail and mixed. being able to watch predictable movement that should be mostly in sync is desirable over the wavy ball that is hard to tell how far behind (or ahead) the two streams are. btw, this is what I am currently using - seconds rendered on top of the ball: gst-launch-1.0 videotestsrc pattern=18 ! clockoverlay time-format="%S" font-desc="Sans 240" ! ximagesink Which is good for multi second problem. (kinda getting off topic, but I wanted to give some context of what I am doing)
Comment on attachment 291277 [details] [review] working linted patch New patch replaces this.
Added two new properties: motion : For Pattern=Ball, what the ball does flags: readable, writable Enum "GstVideoTestSrcMotion" Default: 0, "wavy" (0): wavy - Ball waves back and forth, up and down (1): sweep - 1 revoltion per seccond (2): hsweep - 1/2 revoltion per seccond, then reset to top flip : For Pattern=Ball, invert colors every second. flags: readable, writable Boolean. Default: false wavy is the original pattern. wavy is the best I could to to describe it. I am not sure it matters to describe any of these more accurately, they will be reviewed visually. sweep is my attempt at creating a sweep hand that rotates 1 revolution per second. half-sweep may end up being more useful for my purposes. flip swaps foreground/background every second. > I think time information is already in GstVideoFrame somewhere. I tried to find this and gave up. If you can give me a pointer to an example I'll be happy to use it.
Created attachment 291692 [details] [review] working linted patch
Review of attachment 291692 [details] [review]: Looks good, good work :) ::: gst/videotestsrc/videotestsrc.c @@ +1077,3 @@ + x = radius + (0.5 + 0.5 * sin (2 * G_PI * t / 200)) * (w - 2 * radius); + y = radius + (0.5 + 0.5 * sin (2 * G_PI * sqrt (2) * t / 200)) * (h - + 2 * radius); Shouldn't there be a mode for doing this based on real time instead of frames too? @@ +1089,3 @@ + radius = MIN (h, w) / 4 - 0; + + ts = gst_util_get_timestamp (); And one for doing this based on frames? @@ +1096,3 @@ + rad = 2 * G_PI * ts / 1000000000; + } else { /* motion_type GST_VIDEO_TEST_SRC_HSWEEP */ + rad = G_PI * ts / 1000000000; Use GST_SECOND here instead of the large literals :)
Created attachment 291743 [details] [review] s/10000.../GST_SECOND
adding a property for "base on time or frame" - I think that just clutters the docs/code with an option that may never get used; because I can't think of how it would be used. prolly cuz right now I don't need it ;) note: 99% of my use is live source and streaming over lan and wan, so that's all I think about. If someone tells me it would help debug a codec or something, I'll be happy to add it when someone says "I need this." Including if you think it is needed so that no permutations of options are unavailable which might be confusing to someone. Although scope creep alert: If that's needed for that, then it is needed for this too: horizontal-speed : Scroll image number of pixels per frame (positive is scroll to the left) Whoever wants this should write the docs and I'll implement it. documenting what I did so far was harder than writing the code.
Created attachment 291761 [details] [review] videotestsrc - fixed bug The change of clock source function changed the behaviour of hsweep. this puts it back the way I want it.
hold off. ts = gst_util_get_timestamp (); is giving me a number that doesn't make sense. it's like it is 1/2 second off. i'm investigating.
1/2 off compared to what? You should basically handle it like some random number and only look at differences between those numbers (like the documentation says). Do you want meaningful absolute values?
compared to the system clock displayed in human readable time: the h:m:s display by my desktop's clock display, and the clockoverlay in: gst-launch-1.0 videotestsrc pattern=ball motion=sweep flip=1 ! clockoverlay time-format="%S" When %S flips from 2 to 3, I want my sweep to be at top of it's rotation (like 12 oclock on an analog clock) and the background/foreground colors to flip. I am curious what gst_util_get_timestamp does, but it seems like it isn't what I need.
So you want the wallclock time? No, gst_util_get_timestamp() does not give you that... I'm surprised it's only 1/2s off for you and not a completely different number. Why do you want the wallclock time here? For the sweep it might make sense to use the current running time of the element maybe. You can get that by using the normal synchronization formula: gst_clock_get_time(gst_element_get_clock(element)) - gst_element_get_base_time(element) (plus correct reference counting, handling of no clock or base time)
why wallclock: as a human observing and comparing things I have some easy reference about what correct is. Like when my flip was happening different from clockoverlay time-format="%S" incrementing, I put the gstreamer ximagesink window next to my desktop clock display and quickly verified that they were both clicking seconds at the same time. I can't use current running time because I have more than one os process generating frames, possibly on different machines (and yes, I understand keeping clocks in synk is an issue). I can use the clock on my cell phone to verify the 2 OS clocks are about right. I opened with "as a human" because I am bumping up against too many things for my brain to reconcile: like when my sweep was upside down, I tried to think "it doesn't matter where 0 is, I can watch the 2 displays and see if they were hitting 0 at the same time." It was distracting because I am used to it being at the top of the circle, so it was like both were broken by the same amount. given the point of this code is testing, it is going to be used when things may not be working right, and it is up to a human to get to the bottom of what is broken. adding something to think about is bad.
I've been trying to understand youw use case fro a while now. So far, I understand that you would like videotestsrc to generate an animation for you base on the wallclock so you would see the same animation (or similar) synchronized across processes and even across different computers. This use case exist in GStreamer, though it's not done this way. It's done through controlling the base time and clock being used in your pipeline. A special clock, called the GstNetworkClock can be used also. You could even think of a providing a clock to your pipeline that maps to the wall clock. I think the problem with a test pattern that does not obey to the pipeline clock is that it won't behave correctly when being seeked. Two recording of the same pattern, will not result in the same movement. Also, it won't obey to the requested playback speed. We need to keep in mind that videotestsrc is not only a live test src, it's also an infinit non-live source which can be seeked, and can playback at different rates.
Can you show me the pipelines you mentioned? I am testing with: gst-launch-1.0 -v videotestsrc pattern=ball ! clockoverlay ! ximagesink If I run two of those, the balls are out of sync. Is it really a problem that my test doesn't behave the same as the other tests with regard to time and such? I would think people using it can understand what it does, and expect it to behave as described. maybe I have to move my code to the clockoverlay plugin? seems absurd, but so does a rule saying a test pattern can't be based on wall clock.
Created attachment 291817 [details] [review] uses gettimeofday
I think it's unexpected to use any other time here than the running time. You disagree so maybe there should be an enum property "animation-mode", which can take the following values: frames (current behaviour that should stay the default), running time, wall clock time. Also instead of gettimeofday() you probably want to use g_get_current_time() for portability. Also note that clockoverlay uses localtime() or localtime_r(), but that shouldn't make a difference for the seconds and sub-seconds.
Created attachment 291856 [details] [review] videotestsrc patch - incomplete added option: animation-mode : For Pattern=Ball, what counter defines the position of the ball. flags: readable, writable Enum "GstVideoTestSrcAnimationMode" Default: 0, "frames" (0): frames - frame count (1): running-time - running time (2): wall-time - wall clock time running-time is not implemented, see comments in bug report.
I am not sure where to get running-time from. I took a look at the timeovelay and saw gst_time_overlay_get_text (GstBaseTextOverlay * overlay, GstBuffer * video_frame) GstClockTime time = GST_BUFFER_TIMESTAMP (video_frame); I am guessing video_frame is gst_video_test_src_ball (GstVideoTestSrc * v, GstVideoFrame * frame) I tried to use frame, but that didn't seem to work: time = GST_BUFFER_TIMESTAMP (frame); if (GST_CLOCK_TIME_IS_VALID (time)) { secs = (guint) ((time / GST_SECOND) % 60); msecs = (guint) ((time % GST_SECOND) / (1000 * 1000)); } else { GST_DEBUG ("buffer without valid timestamp"); secs = 0; msecs = 0; } rad = 2 * G_PI * msecs;
Created attachment 292023 [details] [review] videotestsrc patch Adds 3 properties: animation-mode, motion, flip: animation-mode : For Pattern=Ball, what counter defines the position of the ball. flags: readable, writable Enum "GstVideoTestSrcAnimationMode" Default: 0, "frames" (0): frames - frame count (1): wall-time - wall clock time (2): running-time (Throws: NOT IMPLEMENTED) - running time motion : For Pattern=Ball, what the ball does flags: readable, writable Enum "GstVideoTestSrcMotionType" Default: 0, "wavy" (0): wavy - Ball waves back and forth, up and down (1): sweep - 1 revoltion per seccond (2): hsweep - 1/2 revoltion per seccond, then reset to top flip : For Pattern=Ball, invert colors every second. flags: readable, writable Boolean. Default: false Given the amount of chatter around running-time, I left the enum and case in place to document that it has been thought of but not implemented so don't look elsewhere for it. Also if someone wants it they can do the R&D and drop the few lines into the switch statement.
bump. (waiting for review)
Review of attachment 292023 [details] [review]: ::: gst/videotestsrc/gstvideotestsrc.c @@ +185,3 @@ + {GST_VIDEO_TEST_SRC_WALL_TIME, "wall clock time", "wall-time"}, + {GST_VIDEO_TEST_SRC_RUNING_TIME, "running time", + "running-time (Throws: NOT IMPLEMENTED)"}, This needs to be implemented first before merging, or be commented out @@ +207,3 @@ + "wavy"}, + {GST_VIDEO_TEST_SRC_SWEEP, "1 revoltion per seccond", "sweep"}, + {GST_VIDEO_TEST_SRC_HSWEEP, "1/2 revoltion per seccond, then reset to top", typo: second, not seccond @@ +243,3 @@ + g_object_class_install_property (gobject_class, PROP_ANIMATION_MODE, + g_param_spec_enum ("animation-mode", "Animation mode", + "For Pattern=Ball, what counter defines the position of the ball.", pattern=ball (lowercase) @@ +249,3 @@ + g_object_class_install_property (gobject_class, PROP_MOTION_TYPE, + g_param_spec_enum ("motion", "Motion", + "For Pattern=Ball, what the ball does", and here @@ +255,3 @@ + g_object_class_install_property (gobject_class, PROP_FLIP, + g_param_spec_boolean ("flip", "Flip", + "For Pattern=Ball, invert colors every second.", and here ::: gst/videotestsrc/videotestsrc.c @@ +1060,1 @@ gst_video_test_src_ball (GstVideoTestSrc * v, GstVideoFrame * frame) For getting the running time here, the trick would be to pass it as a parameter to the rendering functions. In non-live mode it would be the value that is set to GST_BUFFER_TIMESTAMP/PTS. In live-mode it would be gst_clock_get_time(GST_ELEMENT_CLOCK(src)) - gst_element_get_base_time(src). Nothing complicated really. @@ +1088,3 @@ + /* Not sure where to get parameters from. */ + g_error_new (GST_CORE_ERROR, GST_CORE_ERROR_NOT_IMPLEMENTED, + "Running Time based animation not coded."); This does not do what you think it does :) GError is just a object you can pass around that contains error information, it does not do anything by itself. You could use GST_ELEMENT_ERROR() here for example. But either comment out this running time code or just implement it as above.
Thanks for the review Sebastian! :)
Created attachment 343617 [details] [review] videotestsrc: Add options to make Ball pattern based on system time, and invert each second.
I think this version can finally go upstream :)
Created attachment 343632 [details] [review] videotestsrc: Add options to make Ball pattern based on system time, and invert each second.
Updated to fix a small floating point error that sometimes takes the sqrt of a very small negative number.
Committed as commit 99f489949127d0b4d720d5f6313b8635d0d96e7d Author: Carl Karsten <carl@personnelware.com> Date: Thu Nov 27 18:02:49 2014 -0600 videotestsrc: Add options to make ball pattern based on system time, and invert each second. This adds some extra options that affect pattern=ball mode, allowing the animation to be synced to running time or wall-time clock for comparing sync across different instances / pipelines / machines. Also added is the ability to invert the rendering colours every second, and some different ball motion patterns. https://bugzilla.gnome.org/show_bug.cgi?id=740557
I wonder if "flip" is really the best name for this property, in case someone wants to come up with something better :)
"cycle-colors" maybe?
> "cycle-colors" maybe? I like that. Or perhaps even make it an interval instead of a boolean and then "color-invert-interval" or "cycle-colors-interval" or such?
the current behaviour (that I want) is to flip in sync with when the rotating ball does a cycle. the position of the ball gives you an idea of the possession of the cycle so you see as it approaches the top of the cycle. the flip makes it easy to see when it hit that point. This is all for watching two displays that should be in sync and deciding how close they are.
Would the range work exactly the same as the foreground-color and background-color properties? (also of this videotestsrc element) Carl, Are you interest in taking that task over or do you want me to look into it? Thanks! Luis
I'm not sure how something called "range" would work. not the implementation, but what the vision of how it would be used. I also don't see how foreground/background is relevant. foreground-color : Foreground color to use (big-endian ARGB) flags: readable, writable, controllable Unsigned Integer. Range: 0 - 4294967295 Default: 4294967295 background-color : Background color to use (big-endian ARGB) flags: readable, writable, controllable Unsigned Integer. Range: 0 - 4294967295 Default: 4278190080 So.. sure, look into it.