|
|
Created:
10 years ago by Luigi Semenzato Modified:
9 years, 7 months ago Reviewers:
Daniel Erat CC:
chromium-os-reviews_chromium.org Visibility:
Public. |
DescriptionAdd frame-by-frame animation.
Change-Id: Ia4c409a75e923c7f4e549a2ce6d9d8e95835e84b
BUG=chromium-os:10292
TEST=ran on CR-48
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=1a56e28
Patch Set 1 #
Total comments: 1
Patch Set 2 : Use clock_gettime instead of gettimeofday #
Total comments: 3
Patch Set 3 : Use int64 and simplify timing calculation #
Total comments: 2
Patch Set 4 : . #Patch Set 5 : Add comment #
Total comments: 2
Patch Set 6 : Fix units in comment. #
Messages
Total messages: 14 (0 generated)
This adds animation to our minimal version of ply-image without breaking compatibility with the existing usage. (Dan: I know you don't know this code much, but you are the only other person who's ever looked at it---and it's no rocket science.)
Ah, I'd assumed that you were doing the blurring in software instead of displaying a sequence of pre-blurred images. Is the animated region small enough that reading the images from disk doesn't slow things down noticeably? I assume that we're more disk-bound than CPU-bound during boot. http://codereview.chromium.org/5819001/diff/1/src/ply-image.c File src/ply-image.c (right): http://codereview.chromium.org/5819001/diff/1/src/ply-image.c#newcode507 src/ply-image.c:507: int r = gettimeofday(tv, NULL); If there's any danger of the system clock being changed while this program is running, it'd be safer to use clock_gettime(CLOCK_MONOTONIC, ...)
On 2010/12/14 00:41:15, Daniel Erat wrote: > Ah, I'd assumed that you were doing the blurring in software instead of > displaying a sequence of pre-blurred images. Is the animated region small > enough that reading the images from disk doesn't slow things down noticeably? I > assume that we're more disk-bound than CPU-bound during boot. I tried measuring the boot time change but the measurements were too noisy. When running standalone with the "time" command I get about 110ms vs about 80ms without the animation (just to put up the splash screen). I am putting up 5 frames at 15 frames/second. The images are 276 x 102 pixels. They could be smaller if I don't blur. The total image size is still only a fraction of the image. Blurring with the CPU could be expensive but we can try. > http://codereview.chromium.org/5819001/diff/1/src/ply-image.c > File src/ply-image.c (right): > > http://codereview.chromium.org/5819001/diff/1/src/ply-image.c#newcode507 > src/ply-image.c:507: int r = gettimeofday(tv, NULL); > If there's any danger of the system clock being changed while this program is > running, it'd be safer to use clock_gettime(CLOCK_MONOTONIC, ...) Good idea, thanks. Should it be CLOCK_MONOTONIC or CLOCK_MONOTONIC_RAW or it doesn't matter? CLOCK_MONOTONIC can be corrected by ntp. (Not that it has had any chance at running yet.)
On 2010/12/14 01:03:06, Luigi Semenzato wrote: > On 2010/12/14 00:41:15, Daniel Erat wrote: > > Ah, I'd assumed that you were doing the blurring in software instead of > > displaying a sequence of pre-blurred images. Is the animated region small > > enough that reading the images from disk doesn't slow things down noticeably? > I > > assume that we're more disk-bound than CPU-bound during boot. > > I tried measuring the boot time change but the measurements were too noisy. > When running standalone with the "time" command I get about 110ms vs about 80ms > without the animation (just to put up the splash screen). I am putting up 5 > frames at 15 frames/second. The images are 276 x 102 pixels. They could be > smaller if I don't blur. The total image size is still only a fraction of the > image. > > Blurring with the CPU could be expensive but we can try. Okay, keeping it as-is sounds okay to me, then. > > http://codereview.chromium.org/5819001/diff/1/src/ply-image.c > > File src/ply-image.c (right): > > > > http://codereview.chromium.org/5819001/diff/1/src/ply-image.c#newcode507 > > src/ply-image.c:507: int r = gettimeofday(tv, NULL); > > If there's any danger of the system clock being changed while this program is > > running, it'd be safer to use clock_gettime(CLOCK_MONOTONIC, ...) > > Good idea, thanks. Should it be CLOCK_MONOTONIC or CLOCK_MONOTONIC_RAW or it > doesn't matter? CLOCK_MONOTONIC can be corrected by ntp. (Not that it has had > any chance at running yet.) I didn't know about CLOCK_MONOTONIC_RAW, but that sounds like a better choice. Chrome is using CLOCK_MONOTONIC, but they may need to support pre-2.6.28 kernels. :-)
PTAL. CLOCK_MONOTONIC is good enough, it only can only differ from CLOCK_MONOTONIC_RAW by a small frequency adjustment.
Mostly looks good; I just have a few nit-picky suggestions. http://codereview.chromium.org/5819001/diff/7001/src/ply-image.c File src/ply-image.c (right): http://codereview.chromium.org/5819001/diff/7001/src/ply-image.c#newcode494 src/ply-image.c:494: int difference_nsec(struct timespec x, struct timespec y) why not return int64_t (and add some casts in the expression) so that overflows won't happen? http://codereview.chromium.org/5819001/diff/7001/src/ply-image.c#newcode601 src/ply-image.c:601: req.tv_nsec = (1000 * 1000 * 1000) / frame_rate - elapsed_nsec; could you pull a variable like: int64_t nsec_between_frames; ... nsec_between_frames = (1000 * 1000 * 1000) / frame_rate; outside of the loop, and then do: int64_t nsec_to_sleep = nsec_between_frames - difference_nsec (now, then); if (nsec_to_sleep > 0) { struct timespec req; req.tv_sec = nsec_to_sleep / (1000 * 1000 * 1000); req.tv_nsec = nsec_to_sleep % (1000 * 1000 * 1000); nanosleep (&req, NULL); } here? i think it's a bit easier to read and is more correct if we get changed to a less-than-one-frame-per-second framerate for some reason. http://codereview.chromium.org/5819001/diff/7001/src/ply-image.c#newcode606 src/ply-image.c:606: gettime_check (&then); it'd be cleaner/more accurate to copy 'now' into 'then' instead of fetching a new timestamp here; you can accumulate extra time between the two gettime_check() calls otherwise
The case of less than 1 frame/second is not plausible---but, as you like. It's not going to be a performance issue. Thank you for the time calculation suggestion, PTAL!
http://codereview.chromium.org/5819001/diff/12001/src/ply-image.c File src/ply-image.c (right): http://codereview.chromium.org/5819001/diff/12001/src/ply-image.c#newcode616 src/ply-image.c:616: if (req.tv_nsec > 0) this is wrong if e.g. tv_sec is 1 and tv_nsec is 0. mind doing this instead? if (interval_nsec - error_nsec > 0) { req.tv_sec = ... req.tv_nsec = ... nanosleep (&req, NULL); } http://codereview.chromium.org/5819001/diff/12001/src/ply-image.c#newcode623 src/ply-image.c:623: error_nsec = difference_nsec (now, then) - interval_nsec; i'm not sure i understand this. say that 'frame_rate' is set to 20, so 'interval_nsec' is BILLION / 20 = 50_000_000. if the call to gettime_check() before this loop sets 'now' to 40_000_000, we take 10 ms to draw, and the call to gettime_check() in the first iteration of the loop sets 'now' to 50_000_000, then 'error_nsec' is set to (50_000_000 - 40_000_000) - 50_000_000 = -40_000_000. the next time through, we'll sleep for 50_000_000 - -40_000_000 = 90_000_000 nsec, when we should actually just be sleeping for 40_000_000 nsec. it's entirely possible that i made a mistake in the above.
On 2010/12/14 19:11:02, Daniel Erat wrote: > http://codereview.chromium.org/5819001/diff/12001/src/ply-image.c > File src/ply-image.c (right): > > http://codereview.chromium.org/5819001/diff/12001/src/ply-image.c#newcode616 > src/ply-image.c:616: if (req.tv_nsec > 0) > this is wrong if e.g. tv_sec is 1 and tv_nsec is 0. mind doing this instead? > > if (interval_nsec - error_nsec > 0) { > req.tv_sec = ... > req.tv_nsec = ... > nanosleep (&req, NULL); > } Sorry, yes, I fixed this in parallel. > http://codereview.chromium.org/5819001/diff/12001/src/ply-image.c#newcode623 > src/ply-image.c:623: error_nsec = difference_nsec (now, then) - interval_nsec; > i'm not sure i understand this. say that 'frame_rate' is set to 20, so > 'interval_nsec' is BILLION / 20 = 50_000_000. if the call to gettime_check() > before this loop sets 'now' to 40_000_000, we take 10 ms to draw, and the call > to gettime_check() in the first iteration of the loop sets 'now' to 50_000_000, > then 'error_nsec' is set to (50_000_000 - 40_000_000) - 50_000_000 = > -40_000_000. the next time through, we'll sleep for 50_000_000 - -40_000_000 = > 90_000_000 nsec, when we should actually just be sleeping for 40_000_000 nsec. > > it's entirely possible that i made a mistake in the above. Maybe you're thinking of the way it was before. Now there is only one call to gettime, so the difference between NOW and THEN includes both the drawing and the sleeping, not the drawing only. The first time around we "guess" that drawing takes no time (error = 0) and sleep accordingly. The other times we compensate for the previous error, measured as the expected time vs. the achieved time for an entire cycle. I will add a comment.
http://codereview.chromium.org/5819001/diff/20001/src/ply-image.c File src/ply-image.c (right): http://codereview.chromium.org/5819001/diff/20001/src/ply-image.c#newcode632 src/ply-image.c:632: error_nsec = difference_nsec (now, then) - interval_nsec; i still don't get it. this code would make sense to me if this line didn't have "- interval_nsec" at the end of it. in the example that i gave in the last comment, this would result in us sleeping the expected 40 ms instead of 90 ms.
Sorry, I went through the code again with Dave and think that it's correct (I was missing the sleep the first time through the loop). I still think that this approach is harder to read than just determining the time at which you want to draw the next frame and then sleeping as much as is needed, but LGTM. :-) http://codereview.chromium.org/5819001/diff/20001/src/ply-image.c File src/ply-image.c (right): http://codereview.chromium.org/5819001/diff/20001/src/ply-image.c#newcode493 src/ply-image.c:493: * Compute the difference in microseconds between two timevals. Return 0 if s/microseconds/nanoseconds/
> I still think that this > approach is harder to read than just determining the time at > which you want to draw the next frame and then sleeping as > much as is needed You just described what the code does. Is it confusing because I measure the time at the end of the loop instead of the beginning? I do that to make the first time through the loop work without having to fake some value for THEN. The only other possibility is to get the time between the sleep and the draw, but I don't see how that makes it easier to understand. You still have to reason in terms of the total cycle time.
The funny thing is that this was working fine with just nanosleep (BILLION / frame_rate). I thought this way would be more robust against possible variations in CPU usage, but maybe it's overkill.
On Tue, Dec 14, 2010 at 2:18 PM, <semenzato@chromium.org> wrote: >> I still think that this >> approach is harder to read than just determining the time at >> which you want to draw the next frame and then sleeping as >> much as is needed > > You just described what the code does. Is it confusing because I measure > the > time at the end of the loop instead of the beginning? I do that to make the > first time through the loop work without having to fake some value for THEN. > > The only other possibility is to get the time between the sleep and the > draw, > but I don't see how that makes it easier to understand. You still have to > reason in terms of the total cycle time. I think that I found it confusing due to the (somewhat vaguely-named) error_nsec variable that gets computed at the end of the loop, and the fact that you're saving an interval that includes the sleep instead of just saving a timestamp and using that in the calculation. I expected the general algorithm to be more like this: time_between_frames = 1 sec / framerate last_draw_time = get_current_time() draw_frame() for each frame: elapsed_time = get_current_time() - last_draw_time time_to_sleep = time_between_frames - elapsed_time if (time_to_sleep > 0) sleep(time_to_sleep) last_draw_time = get_current_time() draw_frame() Are you concerned about the sleep not actually taking the expected amount of time? > http://codereview.chromium.org/5819001/ > |