Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(568)

Issue 5819001: Add frame-by-frame animation. (Closed)

Created:
10 years ago by Luigi Semenzato
Modified:
9 years, 7 months ago
Reviewers:
Daniel Erat
CC:
chromium-os-reviews_chromium.org
Visibility:
Public.

Description

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+168 lines, -39 lines) Patch
M src/Makefile View 1 1 chunk +1 line, -1 line 0 comments Download
M src/ply-frame-buffer.c View 1 chunk +4 lines, -4 lines 0 comments Download
M src/ply-image.c View 1 2 3 4 5 5 chunks +163 lines, -34 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
Luigi Semenzato
This adds animation to our minimal version of ply-image without breaking compatibility with the existing ...
10 years ago (2010-12-13 23:38:07 UTC) #1
Daniel Erat
Ah, I'd assumed that you were doing the blurring in software instead of displaying a ...
10 years ago (2010-12-14 00:41:15 UTC) #2
Luigi Semenzato
On 2010/12/14 00:41:15, Daniel Erat wrote: > Ah, I'd assumed that you were doing the ...
10 years ago (2010-12-14 01:03:06 UTC) #3
Daniel Erat
On 2010/12/14 01:03:06, Luigi Semenzato wrote: > On 2010/12/14 00:41:15, Daniel Erat wrote: > > ...
10 years ago (2010-12-14 01:09:02 UTC) #4
Luigi Semenzato
PTAL. CLOCK_MONOTONIC is good enough, it only can only differ from CLOCK_MONOTONIC_RAW by a small ...
10 years ago (2010-12-14 01:55:15 UTC) #5
Daniel Erat
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 ...
10 years ago (2010-12-14 02:18:05 UTC) #6
Luigi Semenzato
The case of less than 1 frame/second is not plausible---but, as you like. It's not ...
10 years ago (2010-12-14 18:36:17 UTC) #7
Daniel Erat
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. ...
10 years ago (2010-12-14 19:11:02 UTC) #8
Luigi Semenzato
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 ...
10 years ago (2010-12-14 19:30:49 UTC) #9
Daniel Erat
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 ...
10 years ago (2010-12-14 20:10:37 UTC) #10
Daniel Erat
Sorry, I went through the code again with Dave and think that it's correct (I ...
10 years ago (2010-12-14 20:22:53 UTC) #11
Luigi Semenzato
> I still think that this > approach is harder to read than just determining ...
10 years ago (2010-12-14 22:18:20 UTC) #12
Luigi Semenzato
The funny thing is that this was working fine with just nanosleep (BILLION / frame_rate). ...
10 years ago (2010-12-14 22:21:50 UTC) #13
Daniel Erat
10 years ago (2010-12-15 01:24:16 UTC) #14
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/
>

Powered by Google App Engine
This is Rietveld 408576698