Currently, when we want to animate something in Java-side fennec, we use Timer and schedule it to repeat at 60Hz. This is inefficient, as it uses threads (which inevitably mean we have to do more locking) and there's no guarantee that the callback will happen at the ideal time for rendering. What we could do with is a way of scheduling tasks to happen before or after rendering, and to happen blocking on that thread. Patches incoming...

This patch adds a new abstract class, RenderTask, with the necessary bits to perform time-based tasks locked into the rendering function. LayerRenderer maintains a list of tasks and processes them as part of its frame-drawing function. LayerView has accessor functions so that other objects have access to render tasks.

This patch replaces TimerTask with RenderTask usage in PanZoomController. It's a very basic replacement, so other than lowering thread usage, it has no other advantages. Ideally, we'd rewrite (somewhat) the animations code in PanZoomController so that it was time-based instead of fixed at 60Hz, though I'm pretty sure all of our target devices have a 60Hz screen update, so this isn't really necessary for now.

Whoops, the original patch, ironically, had concurrency issues - fixed in this one.

And a type error in this patch meant this felt like it was performing really badly. Still get some oddness when running the bounce animation on overscroll, not sure if this is skipping lots of frames or I've messed something up somewhere...

Comment on attachment 582863 [details] [diff] [review] 1- Add RenderTask and the necessary mechanisms to use it (add synchronization) >diff --git a/mobile/android/base/gfx/LayerRenderer.java b/mobile/android/base/gfx/LayerRenderer.java >+ public void postRenderTask(RenderTask aTask) { >+ synchronized(mTasks) { >+ mTasks.add(aTask); >+ } Why do you need this synchronized block here? mTasks is already a synchronized list, so it shouldn't be needed. >+ public void removeRenderTask(RenderTask aTask) { >+ synchronized(mTasks) { >+ mTasks.remove(aTask); >+ } Ditto. >+ } >+ >+ private void runRenderTasks(List<RenderTask> tasks, boolean after, long frameStartTime) { >+ for (ListIterator<RenderTask> i = tasks.listIterator(); i.hasNext();) { >+ RenderTask task = i.next(); >+ if (task.runAfter != after) >+ continue; >+ >+ boolean stillRunning; >+ if (task.startTime > mLastFrameTime) >+ stillRunning = task.run(frameStartTime - task.startTime); >+ else >+ stillRunning = task.run(frameStartTime - mLastFrameTime); The timeDelta passed in here contradicts the comment in RenderTask - that says the timeDelta is the time since the last frame, not the time since the task was scheduled. >+ // Make a copy of the current render tasks, to avoid concurrent >+ // modification exceptions. >+ List<RenderTask> renderTasks; >+ synchronized(mTasks) { >+ renderTasks = new LinkedList<RenderTask>(mTasks); >+ } Making a copy of the list also breaks things because now i.remove() removes it from this copy, not from mTasks. So the tasks will just accumulate in mTasks unless you call removeRenderTask for it. >diff --git a/mobile/android/base/gfx/RenderTask.java b/mobile/android/base/gfx/RenderTask.java >+ /** >+ * The system time, in nanoseconds, when the task was scheduled. >+ */ >+ public final long startTime; [...] >+ >+ public RenderTask(boolean aRunAfter) { >+ startTime = System.nanoTime(); >+ runAfter = aRunAfter; This sets startTime to the creation time, not the scheduling time. I'm not sure why you're making this startTime part of the API of RenderTask anyway. It can be a private variable in subclasses if they need it, set the first time run() is called. That way you can always call task.run(frameStartTime - mLastFrameTime) and not have that branch two comments up^

Comment on attachment 582872 [details] [diff] [review] Use RenderTask in PanZoomController Another thing I realized after investigating bug 709817 further is that this patch moves the animation code from the UI thread to the GL renderer thread. This means things in PanZoomController will now need to be synchronized, because it's not all accessed from the UI thread. I feel like this might introduce more race conditions.

(In reply to Kartikaya Gupta (:kats) from comment #6) > Comment on attachment 582872 [details] [diff] [review] > Use RenderTask in PanZoomController > > Another thing I realized after investigating bug 709817 further is that this > patch moves the animation code from the UI thread to the GL renderer thread. > This means things in PanZoomController will now need to be synchronized, > because it's not all accessed from the UI thread. I feel like this might > introduce more race conditions. The animations are performed via a Timer, which will use a separate thread, so this is already an issue - not to say this patch shouldn't fix it at the same time, but it's not a new thing?

(In reply to Chris Lord [:cwiiis] from comment #7) > (In reply to Kartikaya Gupta (:kats) from comment #6) > The animations are performed via a Timer, which will use a separate thread, > so this is already an issue - not to say this patch shouldn't fix it at the > same time, but it's not a new thing? The timer just posts the Runnable back to the UI thread using mController.post: - mAnimationTimer = new Timer("Animation Timer"); - mAnimationTimer.scheduleAtFixedRate(new TimerTask() { - @Override - public void run() { mController.post(runnable); } - }, 0, 1000L/60L); (I keep forgetting this too, and then keep trying to fix it before I realize it's actually correct already)

tsk, I missed this - thanks for pointing it out :)

This addresses the review comments. Although I fixed the bugs in the second patch too, I don't think it's worth applying as it is, as it will just complicate things like that. Instead, I think the animations should be altered to properly take advantage of RenderTask and be time-based. I'm putting this up for review, but I don't intend to commit it until something actually uses it.

Comment on attachment 583516 [details] [diff] [review] 1- Add RenderTask and the necessary mechanisms to use it (address review) >diff --git a/mobile/android/base/gfx/LayerRenderer.java b/mobile/android/base/gfx/LayerRenderer.java >+ private void runRenderTasks(List<RenderTask> tasks, boolean after, long frameStartTime, List<RenderTask> toRemove) { >+ for (ListIterator<RenderTask> i = tasks.listIterator(); i.hasNext();) { >+ RenderTask task = i.next(); The above can be simplified into "for (RenderTask task : tasks) {" since you're not doing anything else with the iterator. >+ // Request another render if there are render tasks still pending >+ if (!renderTasks.isEmpty()) >+ mView.requestRender(); The above code should be moved below the hunk below, and changed s/renderTasks/mTasks/. The way it is now doesn't do the desired thing because nothing is taken out of renderTasks. >+ >+ // Remove finished tasks from the master list >+ synchronized(mTasks) { >+ for (RenderTask task : toRemove) { >+ mTasks.remove(task); >+ } >+ } >+ Rest looks good, so r+ with above changes.

Chris, what are you planning on doing here?

(In reply to Brad Lassey [:blassey] from comment #12) > Chris, what are you planning on doing here? This is still a bug, and I still think something along the lines of the patches I've written here are the best way to fix it, but I'm not sure how high priority this really is at the moment, vs other things. I'll unassign it for now and add the mentored flag, maybe a volunteer could take it (or I will if I get time again).

hello Chris I would be glad to work on this bug. Would you please guide me.

(In reply to cheta.gup from comment #14) > hello Chris > I would be glad to work on this bug. Would you please guide me. Hi Cheta, Great to hear you'd like to work on this! The patch as it is has bit-rotted a bit, so the part that interfaces with LayerRenderer probably needs to be rewritten. The way it worked can likely still be applied to things now, just they've been moved around. There also needs to be follow-up after the RenderTask patch that actually makes use of it - Primarily, the JavaPanZoomController needs to use it, as it currently just uses a Java timer for animation and it ought to be synchronised to redrawing. I think also fading out the scroll indicators could make use of it too. It might be best to continue this over e-mail, to avoid cluttering this bug - could you e-mail me with any questions you have? It'd be good to know what your current setup is with regards to building Firefox/Android and where you'd need guidance. Thanks, and looking forward to hearing from you!

Here

(please ignore previous comment ;-) ) Hi Chris ! So I'm nearly done updating you patch, and I'm trying to apply the new mechanism to LayerMarginsAnimator. Can you explain to me what the timeDelta argument of RenderTask.run is supposed to do? The only way I managed to make LayerMarginsAnimator running properly is by ignoring it completely in LegacyAnimationTask. So I think I misunderstood something. Is it a safe-check somehow? Thank you!

(In reply to Augustin Trancart [:autra] from comment #17) > (please ignore previous comment ;-) ) > > Hi Chris ! > > So I'm nearly done updating you patch, and I'm trying to apply the new > mechanism to LayerMarginsAnimator. > > Can you explain to me what the timeDelta argument of RenderTask.run is > supposed to do? The only way I managed to make LayerMarginsAnimator running > properly is by ignoring it completely in LegacyAnimationTask. So I think I > misunderstood something. Is it a safe-check somehow? > > Thank you! timeDelta is the number of ms elapsed since the last call of the run function - it should be used by the callback to determine how far to animate (so at 60fps, timeDelta should almost always be 16 or 17 (which now makes me think the parameter should be nanoseconds...)) It's basically the equivalent of the MS_PER_FRAME static variable, except because RenderTask is tied to actual rendering, it'll update with the real value based on the current rendering performance/refresh-rate/etc.

Here is my first work on it. I'm ready for reviews! Concerning Collections.synchronisedList, I'm wondering if we really gain something by using it? We gain on the add and remove operations which are already synchronised, but we still have to synchronize the foreach anyway. Wouldn't it be better to use a non-synchronised collections and synch only when needed? I need a reliable way to measure the effect of each solutions, I guess...

Comment on attachment 749893 [details] [diff] [review] Add RenderTask mechanism (updated) and make LakerMarginsAnimator use the new mechanism Review of attachment 749893 [details] [diff] [review]: ----------------------------------------------------------------- I won't do the final review, but here's an interim review before we request review from someone else; This all looks fine - there's one code comment that needs addressing, but the rest is just about whitespace and indentation. The LayerMarginsAnimator changes should be split into a separate patch as well. ::: .hgignore @@ +63,5 @@ > +^mobile/bin/* > + > +#metadata workspace > +^\.metadata/* > +\.classpath Remove the .hgignore changes in the patch - fine to have these locally, but we don't want to check these in :) ::: mobile/android/base/gfx/LayerMarginsAnimator.java @@ +24,5 @@ > import java.util.TimerTask; > > public class LayerMarginsAnimator implements TouchEventInterceptor { > private static final String LOGTAG = "GeckoLayerMarginsAnimator"; > + Trailing whitespace, please remove. @@ +76,5 @@ > } > > private void animateMargins(final float left, final float top, final float right, final float bottom, boolean immediately) { > + if (mAnimationTask != null) { > + mAnimationTask.continueAnimation = false; Indentation on this line is wrong - make sure to always use spaces. @@ +116,5 @@ > newOffset.y - oldOffset.y); > > if (progress >= 1.0f) { > + if (mAnimationTask != null) { > + mAnimationTask.continueAnimation = false; Indentation. @@ +211,5 @@ > ImmutableViewportMetrics scrollBy(ImmutableViewportMetrics aMetrics, float aDx, float aDy) { > // Make sure to cancel any margin animations when scrolling begins > + if (mAnimationTask != null) { > + mAnimationTask.continueAnimation = false; > + mAnimationTask = null; Indentation ::: mobile/android/base/gfx/LayerRenderer.java @@ +4,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > package org.mozilla.gecko.gfx; > > +import org.mozilla.gecko.GeckoAppShell; Trailing whitespace. @@ +66,5 @@ > private int mMaxTextureSize; > private int mBackgroundColor; > private int mOverscrollColor; > + > + private long mLastFrameTime; Trailing whitespace on these two lines. @@ +148,5 @@ > + > + mTasks = Collections.synchronizedList(new LinkedList<RenderTask>()); > + mLastFrameTime = System.nanoTime(); > + > + Trailing whitespace and an extra unnecessary blank line (just one is enough). @@ +251,5 @@ > > public int getMaxTextureSize() { > return mMaxTextureSize; > } > + Trailing whitespace. @@ +256,5 @@ > + public void postRenderTask(RenderTask aTask) { > + mTasks.add(aTask); > + mView.requestRender(); > + } > + Trailing whitespace. @@ +277,5 @@ > + if (!stillRunning) > + toRemove.add(task); > + } > + } > + Trailing whitespace. @@ +513,5 @@ > + List<RenderTask> renderTasks; > + synchronized(mTasks) { > + renderTasks = new LinkedList<RenderTask>(mTasks); > + } > + Trailing whitespace. @@ +520,5 @@ > + runRenderTasks(renderTasks, false, mFrameStartTime, toRemove); > + //remove rendered tasks > + mTasks.removeAll(toRemove); > + > + Trailing whitespace. @@ +660,5 @@ > > mCompleteFramesRendered += 1.0f - checkerboard; > mFramesRendered ++; > > + if (mFrameStartTime - mProfileOutputTime > 1000000000) { I guess for the sake of consistency, there should be an 'L' after the number on the right (does Java warn about this?) @@ +665,5 @@ > mProfileOutputTime = mFrameStartTime; > printCheckerboardStats(); > } > } > + Trailing whitespace. @@ +672,5 @@ > + List<RenderTask> renderTasks; > + synchronized(mTasks) { > + renderTasks = new LinkedList<RenderTask>(mTasks); > + } > + Trailing whitespace. @@ +674,5 @@ > + renderTasks = new LinkedList<RenderTask>(mTasks); > + } > + > + List<RenderTask> toRemove = new LinkedList<RenderTask>(); > + //Run through post-render tasks Space after '//' @@ +676,5 @@ > + > + List<RenderTask> toRemove = new LinkedList<RenderTask>(); > + //Run through post-render tasks > + runRenderTasks(renderTasks, true, mFrameStartTime, toRemove); > + mTasks.removeAll(toRemove); Does this need to be wrapped in a synchronized(mTasks) block? (I don't know, but best check the docs if you don't know off-hand) @@ +681,4 @@ > > /* Draw the FPS. */ > if (mFrameRateLayer != null) { > updateDroppedFrames(mFrameStartTime); You've changed mFrameStartTime to nanoseconds, but I'm guessing this function still expects milliseconds - best audit all uses of this. ::: mobile/android/base/gfx/LayerView.java @@ +353,5 @@ > > + public void postRenderTask(RenderTask task) { > + mRenderer.postRenderTask(task); > + } > + Trailing whitespace. ::: mobile/android/base/gfx/LegacyAnimationTask.java @@ +41,5 @@ > + > +/** > + * LegacyAnimationTask - made to quickly migrate old rendering made with a Timer to the RenderTask mechanism. > + * This class suppose a 60 fps rendering > + * @author Augustin Trancart This class uses tabs for indentation and contains a lot of trailing whitespace. Also, we don't use author tags like this - this file does (again, unusually) have a contributors block above that you can add your name to, but I'd consider just stripping it entirely. These blocks tend to be pretty misleading and we have version control to keep track of this :) ::: mobile/android/base/gfx/RenderTask.java @@ +39,5 @@ > + > +/** > + * A class used to schedule a callback to occur when the next frame is drawn. > + */ > +public abstract class RenderTask { Same again about indentation and whitespace.

Wow, it seems my config for indentation is all wrong :-/ Everything is messed up. Really sorry for that, I will fix this as soon as possible. Concerning the other details : - we need the synchronized block for the for loop, this is explicitly stated in the javadoc. I'll double check this anyway. - The L is not necessary in java (as numbers are always promoted, not like in C), but that is always good for the reader to have it, so I'll add it. Thank you for taking time to give me feedbacks !

Here is the new patch for RenderTask (without the adaptation of LayerMarginsAnimator) with hopefully a correct indentation. I've audited the updateDroppedFrame, and I've added the conversion. Feel free to comment on that (and on anything else of course). I'll upload a first version of my LayerMarginsAnimator.patch if you want, but it does not take into account the real framerate atm.

Here is the change for LayerMarginsAnimator. I start working on JavaPanZoomController.

Comment on attachment 754418 [details] [diff] [review] Add RenderTask mechanism (correct indent, and whitespaces) Review of attachment 754418 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, some small comments below - I can't review this as it's based on my original patch (though ftr, it'd be an r+ ;)). I've set kats as the reviewer. ::: mobile/android/base/gfx/LayerRenderer.java @@ +266,5 @@ > + if (task.runAfter != after) > + continue; > + > + boolean stillRunning; > + stillRunning = task.run(frameStartTime - mLastFrameTime); nit, might as well put the declaration and assignment on the same line. ::: mobile/android/base/gfx/LegacyAnimationTask.java @@ +1,1 @@ > +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-*/ I think we need a licence header here (just copy and paste from another file, but make sure the dates are up-to-date) @@ +4,5 @@ > + > +import android.util.Log; > + > +/** > + * LegacyAnimationTask - made to quickly migrate old rendering made with a Timer to the RenderTask mechanism. s/rendering/animations/ @@ +5,5 @@ > +import android.util.Log; > + > +/** > + * LegacyAnimationTask - made to quickly migrate old rendering made with a Timer to the RenderTask mechanism. > + * This class suppose a 60 fps rendering s/suppose a/assumes/ @@ +25,5 @@ > + } > + > + @Override > + public boolean run(long timeDelta) { > + //timeDelta is ignored because this LegacyAnimationTask is made to run at 60fps Add a space at the start of the comment.

Comment on attachment 755265 [details] [diff] [review] Adapt the LayerMarginsAnimator to use the new RenderTask mechanism Review of attachment 755265 [details] [diff] [review]: ----------------------------------------------------------------- This looks good with the comments addressed. One more round I think :) ::: mobile/android/base/gfx/LayerMarginsAnimator.java @@ +76,5 @@ > } > > private void animateMargins(final float left, final float top, final float right, final float bottom, boolean immediately) { > + if (mAnimationTask != null) { > + mAnimationTask.continueAnimation = false; Instead of setting this to false, you could call removeRenderTask. I don't feel strongly about this. @@ +179,5 @@ > ImmutableViewportMetrics scrollBy(ImmutableViewportMetrics aMetrics, float aDx, float aDy) { > // Make sure to cancel any margin animations when scrolling begins > + if (mAnimationTask != null) { > + mAnimationTask.continueAnimation = false; > + mAnimationTask = null; Same here. @@ +235,5 @@ > + //time of creation in millis > + private long startTime; > + private float startLeft, startTop, startRight, startBottom; > + private float top, bottom, left, right; > + public boolean continueAnimation; Doesn't RenderTask already have an equivalent of this variable? @@ +254,5 @@ > + } > + > + @Override > + public boolean run(long timeDelta) { > + if(!continueAnimation) Taking into account the previous comment, you wouldn't need this check. Also, space between if and bracket. @@ +257,5 @@ > + public boolean run(long timeDelta) { > + if(!continueAnimation) > + return false; > + float progress = mInterpolator.getInterpolation( > + Math.min(1.0f, (SystemClock.uptimeMillis() - startTime) You should use timeDelta here, rather than uptimeMillis() - startTime. You wouldn't need to store startTime then.

Comment on attachment 754418 [details] [diff] [review] Add RenderTask mechanism (correct indent, and whitespaces) Review of attachment 754418 [details] [diff] [review]: ----------------------------------------------------------------- I think making so many copies of the list is unnecessary, and would like to see that cleaned up. This is particularly important because this code sits on a hot code path that gets run a lot and object creation should be kept to a minimum. ::: mobile/android/base/gfx/LayerRenderer.java @@ +13,1 @@ > import org.mozilla.gecko.gfx.Layer.RenderContext; Move the RenderTask import down one so it's alphabetized @@ +67,5 @@ > private int mBackgroundColor; > private int mOverscrollColor; > > + private long mLastFrameTime; > + private List<RenderTask> mTasks; Make mTasks final. @@ +510,5 @@ > + // modification exceptions. > + List<RenderTask> renderTasks; > + synchronized(mTasks) { > + renderTasks = new LinkedList<RenderTask>(mTasks); > + } Making a copy of the list here is unnecessary because you're using the toRemove list to track items to remove. Alternatively you could just make mTasks a CopyOnWriteArrayList and not need any of this. I would prefer that approach since it minimizes the creation of new objects on a hot code path. ::: mobile/android/base/gfx/RenderTask.java @@ +1,1 @@ > +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- */ License here too

Hey thank you very much for the comments! I've seen your message on irc also. I will address these before the end of the week (hopefully). Concerning your last comment, I think this is two different things. timeDelta is the interval between two frames (the last two actually, including the current one). StartTime is the time you scheduled the animation, and we keep it to make the animation lasts 250ms at most. So SystemClock.uptimeMillis() - startTime goes from 0 to 250 ms (that's why we divide by 250 and compare it to 1). So the number of time it runs actually depends on how many frames there is in 250ms. So I think we should keep it like that. It seems to me that this animation is actually totally fps-independant, and rely on time only, which is fine for the new mechanism I guess.

Here is the new RenderTask mechanism with comments from Chris and kats addressed. By the way, how can I efficiently measure the effect of my modifications? Is there already a mechanism to have, like, the mean fps? LayerMarginsAnimation patch coming shortly.

Btw Chris, concerning continueAnimation, it is LegacyAnimationTask that have this variable, not RenderTask. I'll see if LayerMarginsAnimationTask should inherit RenderTask or instead if I should use LegacyAnimationTask...

Comment on attachment 755874 [details] [diff] [review] Add RenderTask mechanism (add Licence block, replace synchronised list by CopyOnWriteArrayList) Review of attachment 755874 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the back and forth on this - there's only one minor issue left, but I've also had a change of heart about RenderTask (detailed below). ::: mobile/android/base/gfx/LayerRenderer.java @@ +504,5 @@ > mUpdated = true; > > Layer rootLayer = mView.getLayerClient().getRoot(); > > + List<RenderTask> toRemove = new LinkedList<RenderTask>(); We could probably save on performance by making this a member variable and an ArrayList or similar, rather than a LinkedList. @@ +656,5 @@ > } > } > > + List<RenderTask> toRemove = new LinkedList<RenderTask>(); > + //Run through post-render tasks Space at the start of the comment - taking previous comment into account, we wouldn't need to allocate a new list here either (just clear it). ::: mobile/android/base/gfx/RenderTask.java @@ +22,5 @@ > + * > + * Note, implementers must take care that this method will be called in the > + * rendering thread. > + */ > + public abstract boolean run(long timeDelta); I'm starting to think that perhaps the animation start time and current time would be useful here, in addition to the delta. These aren't the same as using System.nanotime, as you really want the time at the start of the frame to match for all animations and you want it to start on the first call rather than when it gets added to the task-list. How would you feel about changing this patch to do that? That would simplify your LayerMarginsAnimator patch as well.

(In reply to Augustin Trancart [:autra] from comment #28) > Created attachment 755874 [details] [diff] [review] > Add RenderTask mechanism (add Licence block, replace synchronised list by > CopyOnWriteArrayList) > > Here is the new RenderTask mechanism with comments from Chris and kats > addressed. > > By the way, how can I efficiently measure the effect of my modifications? Is > there already a mechanism to have, like, the mean fps? > > LayerMarginsAnimation patch coming shortly. We don't really have a good way of measuring as small an effect as this at the moment - I doubt you'd see any discernable difference in measured values. What's more important is that the animation is synchronised to redrawing, so it should 'feel' better. If you recorded the time between frames, I'd expect this to provide a more consistent output, assuming that compositing isn't taking too long. Certainly, you should see values that synchronise better with a 60hz refresh, so frame times that are multiples of 16.67ms.

You shouldn't need the toRemove list at all. The CopyOnWriteArrayList will handle doing a remove from inside the iteration loop directly. So you can call mTasks.remove in runRenderTasks instead of adding it to the toRemove list. Everything else looks fine to me.

Here is another version. The remove mechanism is now in the loop, thanks to CopyOnWriteArrayList. I've removed LegacyAnimationTask, as we probably won't use it. RenderTask is now more complete. It has a firstRunTime. Subclasses will now have to implement runInternal and not run. run delegates to runInternal after having initialized the firstRunTime.

Comment on attachment 756008 [details] [diff] [review] Add RenderTask mechanism (rendertask has now startTime, addresses kats comments) Review of attachment 756008 [details] [diff] [review]: ----------------------------------------------------------------- We're basically here now. We just need to recombine run/internalRun and rename firstRunTime to startTime and I think we're done - Everything else looks good to me. ::: mobile/android/base/gfx/LayerRenderer.java @@ +262,5 @@ > + } > + > + private void runRenderTasks(CopyOnWriteArrayList<RenderTask> tasks, boolean after, long frameStartTime) { > + for (RenderTask task : tasks) { > + if (task.runAfter != after) I missed this in previous reviews, but the general consensus is to always use curly braces with if's. @@ +268,5 @@ > + > + boolean stillRunning = task.run(frameStartTime - mLastFrameTime, frameStartTime); > + > + // Remove the task from the list if its finished > + if (!stillRunning) Same here. ::: mobile/android/base/gfx/RenderTask.java @@ +17,5 @@ > + > + /** > + * time when this task has first run, in ms. Useful for tasks which run for a specific duration. > + */ > + private long firstRunTime; I think 'startTime' would be a better name for this. Also, nit, capital letter to start in the comment. @@ +47,5 @@ > + * @param timeDelta the last frame duration > + * @param currentFrameStartTime the startTime of the current frame, in ms. > + * @return true if animation should be run at the next frame, false otherwise > + */ > + public abstract boolean internalRun(long timeDelta, long currentFrameStartTime); I don't like the idea of run/internalRun split. I think we should just document that you must call super.run first when overriding run - that's reasonably common already. @@ +58,5 @@ > + * get the first frame start time of this task. > + * By first frame, we mean the first frame in which it has run. > + * @return creationTime in ms > + */ > + public long getFirstRunTime() { similarly, this would just be getStartTime.

Comment on attachment 756008 [details] [diff] [review] Add RenderTask mechanism (rendertask has now startTime, addresses kats comments) Review of attachment 756008 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/gfx/RenderTask.java @@ +17,5 @@ > + > + /** > + * time when this task has first run, in ms. Useful for tasks which run for a specific duration. > + */ > + private long firstRunTime; For private variables, make sure they start with the "m" prefix. so mFirstRunTime (or mStartTime, if you rename it). @@ +19,5 @@ > + * time when this task has first run, in ms. Useful for tasks which run for a specific duration. > + */ > + private long firstRunTime; > + > + private boolean firstRun = false; Same here - mFirstRun. Also you don't need to initialize it to false; member variables are initialized to 0 or false by default. @@ +34,5 @@ > + * @param currentFrameStartTime the startTime of the current frame, in ms. > + * @return true if animation should be run at the next frame, false otherwise > + */ > + public boolean run(long timeDelta, long currentFrameStartTime) { > + if(firstRun) { nit: space before "(" @@ +47,5 @@ > + * @param timeDelta the last frame duration > + * @param currentFrameStartTime the startTime of the current frame, in ms. > + * @return true if animation should be run at the next frame, false otherwise > + */ > + public abstract boolean internalRun(long timeDelta, long currentFrameStartTime); Unlike Chris, I *do* like the idea of the run/internalRun split, and it's a pretty common Java idiom to do that. However the run method should be marked as final so it can't be overridden. Chris, I prefer this approach because it reduces the possibility that subclasses screw things up. If they miss calling super.run() it introduces a subtle breakage that may not be easily detected. Doing it this way means they are forced to implement internalRun (since it's abstract) and they can't override run() (because it would be final) and so the only way to do it is the right way.

Thank you for the review! Concerning run/internalRun, I do need to add final to fullfill the design pattern, that's right. I omitted it thinking it would give more flexibility, but it would remove all the benefit of it to do so I guess. And I made another mistake in the pattern : internalRun should be protected, to prevent other classes to call it directly. So imho there is still two possibilities: - Either we put startTime in RenderTask, and in this case, as we can manage it there, I see no reason not to do so. And I think we'd better secure it like that. - or we decide that it is not something general, we remove it completely from RenderTask, and let the subclasses who need it handle it. What do you think? (I prefer the first way, but I don't know fennec good enough to know) I address other comments asap. By the way, what does "nit" mean? è_ô

Ok, here is the updated version of the patch with your comments. This still includes startTime in RenderTask. I also mark the LayerMarginsAnimator as obsolete, because this won't work anymore with that I guess. Hope everything will be ok this time! (no nit maybe ? ;-) )

Mhh just realised that there are mix between ms and ns in my last patch. I correct that asap. I think timeDelta will be in ns for precision, but startTime should be kept in ms, shouldn't it? Or is it too confusing to have several units of time?

I post another one, but this time I decided that everything would be in ns. So I just changed the javadoc to indicate that startTime is also in ns.

Another update: - Expose a method to schedule a reset of the startTime - address errors in comments: I let some "ms" in javadocs in the previous patch

Comment on attachment 756609 [details] [diff] [review] Add RenderTask mechanism (expose resetStartTime, and address errors in comments) Review of attachment 756609 [details] [diff] [review]: ----------------------------------------------------------------- This is an r- from me, only because of the run/internalRun split - the rest are just nits. With either the suggest change, or just getting rid of internalRun and documenting that you must chain up, this would be an r+. ::: mobile/android/base/gfx/RenderTask.java @@ +19,5 @@ > + * Time when this task has first run, in ns. Useful for tasks which run for a specific duration. > + */ > + private long mStartTime; > + > + private boolean mResetStartTime = true; nit, needs documentation. @@ +29,5 @@ > + * true implicitly schedules a redraw. > + * > + * Note : subclasses should overwrite internalRun. > + * > + * @param timeDelta the last frame duration I don't think this description is obvious enough - something like "The time between this frame and the last frame, in ns" would make it extra clear. @@ +38,5 @@ > + public final boolean run(long timeDelta, long currentFrameStartTime) { > + if (mResetStartTime) { > + mStartTime = currentFrameStartTime; > + mResetStartTime = false; > + } I still don't like the run/internalRun split - I think we should just have run and this block can happen in runRenderTasks in LayerRenderer - so immediately before calling run, it can do if (task.resetStartTime) { task.startTime = currentTime; task.resetStartTime = false; }, or something along those lines, and we could make those fields package-scope. @@ +55,5 @@ > + runAfter = aRunAfter; > + } > + > + /** > + * Get the start time of this task. nit, double space after 'the'. I empathise, my keyboard is half-broken and does this all the time :) @@ +64,5 @@ > + return mStartTime; > + } > + > + /** > + * Schedule a reset of startTime next time {@link RenderTask#run(long, long)} is run. nit, s/startTime/the recorded start time/ - seeing as startTime is private (or package scope after the above suggested change), I guess we shouldn't reference it in documentation.

If you happen to work on this on the weekend, I'll try to keep up, but my connectivity is going to be patchy while I'm on holiday so I'll leave kats to help finish up :)

Oh, let's also mark this as assigned, seeing as you've basically done all the work for it already!

Thanks for the comments Chris. Sorry, I didn't have time to work on it this week-end unfortunately. Ok for all the nits, I'll correct that asap. Concerning the run/internalRun split, I would like to understand better why you don't like it. Can you state your reason more precisely? Here is why I think we should do it this way : It is a common design pattern when we want to template in a abstract superclass any behavior for subclasses, called the "Template Method Pattern". Here run is the template method, and internalRun is a step. It is used by folks from Springframework, for example here https://github.com/SpringSource/spring-mobile/blob/master/spring-mobile-device/src/main/java/org/springframework/mobile/device/view/AbstractDeviceDelegatingViewResolver.java or on Android (android.app.Activity, with onCreate and performCreate for exemple, the first one is the step and the second one is the template). In this case, I prefer this than one of the other alternatives: - The super.run alternative rely on the subclass to do things correctly. It forces another developper to be careful, it let the possibility to screw things up. If the developper forgets to call super.run, it will still work, which is not good, because it'll never get corrected. - In my opinion, the alternative of doing this in LayerRenderer violates the separation of concern principle because this logic belongs to RenderTask and not to LayerRenderer. It also allows to bypass it very easily. It also add to the complexity of this class, which does not need it frankly. And it is confusing for devs, which won't expect to find this here. - On the other hand, with the run/internalRun split, we force subclasses to implement internalRun (and not run, final method) and we force user classes to call run (and not internalRun, protected). So we basically enforce exactly what we want. The names run/internalRun may be wrong though. Of course, I am ready to hear any criticism of this and there is still the possibility to let the subclasses handle this...

Comment on attachment 756609 [details] [diff] [review] Add RenderTask mechanism (expose resetStartTime, and address errors in comments) Review of attachment 756609 [details] [diff] [review]: ----------------------------------------------------------------- Found a few small things in addition to what Chris said in his review. However I am still in favour of the run/internalRun split the way you did it, for exactly the reasons you described in your above comment. Since Chris is on PTO now I'm going to say you can leave the run/internalRun as-is, and upload a new patch with the documentation and other nits fixed. If Chris has a strong argument for changing this we can change it when he gets back. Thanks! ::: mobile/android/base/gfx/LayerRenderer.java @@ +32,5 @@ > import java.nio.FloatBuffer; > import java.nio.IntBuffer; > +import java.util.Collections; > +import java.util.LinkedList; > +import java.util.List; You can remove these three imports as they are not needed any more. @@ +645,5 @@ > > mCompleteFramesRendered += 1.0f - checkerboard; > mFramesRendered ++; > > + if (mFrameStartTime - mProfileOutputTime > 1000000000) { Please define a private static final long NANOS_PER_SECOND at the top of the file and use that here. It will be easier to read than having to count the zeros and figure out what this magic number is. ::: mobile/android/base/gfx/RenderTask.java @@ +19,5 @@ > + * Time when this task has first run, in ns. Useful for tasks which run for a specific duration. > + */ > + private long mStartTime; > + > + private boolean mResetStartTime = true; Agreed, this variable needs documentation. @@ +27,5 @@ > + * the last call, in nanoseconds. Returns true if it should continue > + * running, or false if it should be removed from the task queue. Returning > + * true implicitly schedules a redraw. > + * > + * Note : subclasses should overwrite internalRun. s/overwrite/override/

I addressed the nits and added some javadoc. I also described what the run method does. As is it a template, it should be documented. As you advised to me kats, I keep run/internalRun for now.

Comment on attachment 757454 [details] [diff] [review] Add RenderTask mechanism (javadoc and nits) Review of attachment 757454 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks! Clearing review for Cwiiis since he's on PTO.

Are there other patches that are supposed to go in this bug (i.e. patches to use the RenderTask in various places)? I'm not sure of what you discussed with Chris before.

Yes, every animation that use a timer to be rendered should be migrated. As far as I know, there are at least LayerMarginsAnimation (I just need to update my patch), and JavaPanZoomController. You will have the former today for sure. Not the latter, I think.

This patch updates LayerMarginsAnimator to use the new mechanism. I have a question about a possible improvement though. I think the synchronized block on mTarget can be done only on mTarget.forceViewportMetrics, as getViewportMetrics is just a getter on some properties and that oldMetrics.setMargins does not actually modify oldMetrics. Am I right on that? I'm not sure whether it is really useful or not, but we'd better synchronize the least we can right?

Comment on attachment 758007 [details] [diff] [review] Adapt the LayerMarginsAnimator to use the new RenderTask mechanism Review of attachment 758007 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty good! Mostly some cosmetic changes (some of which existed in the old code too, but since you're touching the code you might as well clean it up). I'd like to see a new version of the patch with the comments addressed, and also with a commit message ready for landing (should include bug number, and without things like "first work"). ::: mobile/android/base/gfx/LayerMarginsAnimator.java @@ +70,5 @@ > } > > private void animateMargins(final float left, final float top, final float right, final float bottom, boolean immediately) { > + if (mAnimationTask != null) { > + mTarget.getView().removeRenderTask(mAnimationTask); Also set mAnimationTask to null here. @@ +85,5 @@ > final long startTime = SystemClock.uptimeMillis(); > final float startLeft = metrics.marginLeft; > final float startTop = metrics.marginTop; > final float startRight = metrics.marginRight; > final float startBottom = metrics.marginBottom; You can get rid of these startXXX variables here. @@ +171,5 @@ > */ > ImmutableViewportMetrics scrollBy(ImmutableViewportMetrics aMetrics, float aDx, float aDy) { > // Make sure to cancel any margin animations when scrolling begins > + if (mAnimationTask != null) { > + mTarget.getView().removeRenderTask(mAnimationTask); Set mAnimationTask to null here. @@ +222,5 @@ > return false; > } > + > + class LayerMarginsAnimationTask extends RenderTask { > + Remove empty line here. @@ +225,5 @@ > + class LayerMarginsAnimationTask extends RenderTask { > + > + private float startLeft, startTop, startRight, startBottom; > + private float top, bottom, left, right; > + public boolean continueAnimation; Please prefix all the variables with "m", and make continueAnimation private as well. @@ +243,5 @@ > + } > + > + @Override > + public boolean internalRun(long timeDelta, long currentFrameStartTime) { > + if(!continueAnimation) { Space before "(" @@ +250,5 @@ > + > + // Calculate the progress (between 0 and 1) > + float progress = mInterpolator.getInterpolation( > + Math.min(1.0f, (System.nanoTime() - getStartTime()) > + / (float)MARGIN_ANIMATION_DURATION)); Line up the "/" under the "(System.nanoTime()" @@ +253,5 @@ > + Math.min(1.0f, (System.nanoTime() - getStartTime()) > + / (float)MARGIN_ANIMATION_DURATION)); > + > + // Calculate the new metrics accordingly > + synchronized(mTarget.getLock()) { Space before "(" @@ +264,5 @@ > + PointF oldOffset = oldMetrics.getMarginOffset(); > + PointF newOffset = newMetrics.getMarginOffset(); > + newMetrics = > + newMetrics.offsetViewportByAndClamp(newOffset.x - oldOffset.x, > + newOffset.y - oldOffset.y); Line this up under newOffset.x

Your patch also looks like it will fix some NPEs that exist in the current code. \o/

(In reply to Augustin Trancart [:autra] from comment #50) > I have a question about a possible improvement though. I think the > synchronized block on mTarget can be done only on > mTarget.forceViewportMetrics, as getViewportMetrics is just a getter on some > properties and that oldMetrics.setMargins does not actually modify > oldMetrics. Am I right on that? I'm not sure whether it is really useful or > not, but we'd better synchronize the least we can right? Actually the entire block needs to be synchronized, because the block looks roughly link this: synchronized (lock) { metrics = getmetrics() do_stuff_with_metrics setmetrics(metrics) } What you're suggesting is to just lock around the setmetrics call, which means that other code could mutate the metrics on the GeckoLayerClient between the getmetrics and the setmetrics. If that happened, then those mutations from the other code would get clobbered because we would set a metrics object based on the original metrics that we got. By keeping the lock around the whole thing, other code cannot mutate the metrics concurrently and so both sets of changes will applied. Does that make sense?

Yes :-) I was fooled by the fact these metrics are immutables... But that doesn't mean that the resulting instance of metrics does not need to reflect all the actions on metrics. I'll address all of these asap, and will start working on PZC.

Here is RenderTask patch, with a updated commit message.

I upload it again because two nits you reported were not corrected on the previous one (alignments of the second line of two 2-lines statement). I missed them when I merged with incoming revs.

Comment on attachment 765957 [details] [diff] [review] JavaPanZoomController adaptation to RenderTask A little comment about this patch : I don't use BOUNCE_ANIMATION_DURATION, because this was very misleading : it was compared against mBounceFrame * the frame duration in ms, which is not the duration of the animation (it is so in the very special case when we draw at 60 fps perfectly, and we don't drop any frame). When I tried to rely on pure duration, the animation was very snappy and not really beautiful (on my emulator, the bounce never had time to bounce back and jumped quite violently). That's why I decided to switch back to number of runs. The result is closer to the original one, though its speed depends on framerate, which was not the case previously. What do you think about that?

(In reply to Augustin Trancart [:autra] from comment #59) > Comment on attachment 765957 [details] [diff] [review] > JavaPanZoomController adaptation to RenderTask > > A little comment about this patch : I don't use BOUNCE_ANIMATION_DURATION, > because this was very misleading : it was compared against mBounceFrame * > the frame duration in ms, which is not the duration of the animation (it is > so in the very special case when we draw at 60 fps perfectly, and we don't > drop any frame). > When I tried to rely on pure duration, the animation was very snappy and not > really beautiful (on my emulator, the bounce never had time to bounce back > and jumped quite violently). That's why I decided to switch back to number > of runs. The result is closer to the original one, though its speed depends > on framerate, which was not the case previously. What do you think about > that? I don't think we should have any speed that depends on the frame-rate, it'll accentuate any instances of jank that occur and might feel odd when we can't quite hit 60fps and our framerate fluctuates a lot. If the previous code relied on number of runs, could we make it a duration based on the overscroll distance? I'll review this patch regardless of this, we can fix this in another revision.

Comment on attachment 765957 [details] [diff] [review] JavaPanZoomController adaptation to RenderTask Review of attachment 765957 [details] [diff] [review]: ----------------------------------------------------------------- This is an excellent first run - perhaps kats will have further comment, but hopefully I've covered enough below for you to prepare another patch. ::: mobile/android/base/gfx/GeckoLayerClient.java @@ +901,5 @@ > + > + @Override > + public void removeRenderTask(RenderTask task) { > + mView.removeRenderTask(task); > + } I don't think there's a need or these two functions, you can just call getView().postRenderTask() instead. ::: mobile/android/base/gfx/JavaPanZoomController.java @@ +74,5 @@ > // The maximum zoom factor adjustment per frame of the AUTONAV animation > private static final float MAX_ZOOM_DELTA = 0.125f; > > + // The number of time a bounce animation should run to look nice. > + private static final int BOUNCE_FRAME_NUMBER = 15; So given my previous comment, we could have something like BOUNCE_ACCELERATION, or similar (does it accelerate? s/acceleration/velocity/ if not). @@ +112,5 @@ > private final TouchEventHandler mTouchEventHandler; > private final EventDispatcher mEventDispatcher; > > + /* The task that handles flings, autonav or bounces. */ > + private UiRenderTask mUiRenderTask; I think this variable could be named better - perhaps mAnimationRenderTask? @@ +748,5 @@ > bounce(getValidViewportMetrics(), PanZoomState.BOUNCE); > } > > /* Starts the fling or bounce animation. */ > + private void startUiRenderTask(final UiRenderTask task) { Similarly here, startAnimationRenderTask. @@ +801,5 @@ > } > } > > + /** > + * This class is an implementation of RenderTask which enforces its implementor to run in the ui thread. nit: capitalise 'ui'. @@ +804,5 @@ > + /** > + * This class is an implementation of RenderTask which enforces its implementor to run in the ui thread. > + * > + */ > + private abstract class UiRenderTask extends RenderTask { I'm not keen on calling this UiRenderTask either - PanZoomRenderTask instead? @@ +818,5 @@ > + if (!mContinueAnimation) { > + return false; > + } > + > + mTarget.post(new Runnable() { The fact that we're posting this means it won't be synchronous, which, to some extent, defeats the purpose. Perhaps we could synchronize on the PanZoomController and run this method directly? I'm not sure if that's correct, but do check that out if you can. @@ +829,5 @@ > + return mContinueAnimation; > + } > + > + /** > + * The method suclasses override. This method is run on the ui thread thanks to internalRun nit, capitalise 'ui'. @@ +835,4 @@ > protected abstract void animateFrame(); > > + /** > + * Terminate an animation. nit, s/an/the/ @@ +842,4 @@ > } > } > > + private class AutonavUiTask extends UiRenderTask { If UiRenderTaskis renamed, perhaps we should call this AutonavRenderTask. @@ +861,5 @@ > } > } > > + /* The task that performs the bounce animation. */ > + private class BounceUiTask extends UiRenderTask { ditto, BounceRenderTask. @@ +904,5 @@ > > /* Performs one frame of a bounce animation. */ > private void advanceBounce() { > synchronized (mTarget.getLock()) { > + float t = easeOut( ((float)mBounceFrame) / BOUNCE_FRAME_NUMBER); nits, remove the space after the first bracket and remove the braces around ((float)mBounceFrame). @@ +921,5 @@ > } > } > > // The callback that performs the fling animation. > + private class FlingUiTask extends UiRenderTask { FlingRenderTask

Comment on attachment 765957 [details] [diff] [review] JavaPanZoomController adaptation to RenderTask Review of attachment 765957 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/gfx/GeckoLayerClient.java @@ +895,5 @@ > } > > + @Override > + public void postRenderTask(RenderTask task) { > + mView.postRenderTask(task); Please add the "Implementation of PanZoomTarget" comment above these two methods as well. ::: mobile/android/base/gfx/JavaPanZoomController.java @@ +822,5 @@ > + mTarget.post(new Runnable() { > + > + @Override > + public final void run() { > + animateFrame(); Because this runnable is always doing exactly the same thing, it would be better to allocate it once as a class variable and then just post it multiple times. Right now it's allocated once for every frame of animation, which will result in a lot of GC pressure (particularly since this will run on the compositor thread which we want to avoid from janking). @@ +837,5 @@ > + /** > + * Terminate an animation. > + */ > + public void terminate() { > + mContinueAnimation = false; This terminate() function is called on the UI thread, which means that mContinueAnimation must be checked on the UI thread as well. With the code as you've written it, there is a possible race where mContinueAnimation is set to false while the compositor thread is in the middle of the internalRun function, such that the animateFrame() runnable still gets posted. That means that animateFrame() will run once on the UI thread after terminate() has already been called on the UI thread, which is bad. I would suggest (1) changing internalRun so that the only thing it does is post the runnable, (2) providing a protected final implementation of animateFrame in this base class that does the mContinueAnimation check/return, and (3) having an animateFrameInternal or some such abstract method that is abstract for subclasses to override. In effect you want to push the mContinueAnimation check inside the UI-thread runnable.

(In reply to Chris Lord [:cwiiis] from comment #61) > @@ +818,5 @@ > > + if (!mContinueAnimation) { > > + return false; > > + } > > + > > + mTarget.post(new Runnable() { > > The fact that we're posting this means it won't be synchronous, which, to > some extent, defeats the purpose. Perhaps we could synchronize on the > PanZoomController and run this method directly? I'm not sure if that's > correct, but do check that out if you can. :autra and I discussed this on IRC last week (see discussion starting at [1]). The gist of it was that making it run directly on the compositor thread is hard because we'd have to add synchronization to pretty much everything in PZC. I don't think it's worth doing that at this point considering we're planning on axing PZC in favour of APZC at some point anyway. [1] http://irclog.gr/#show/irc.mozilla.org/mobile/289140

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #63) > (In reply to Chris Lord [:cwiiis] from comment #61) > > @@ +818,5 @@ > > > + if (!mContinueAnimation) { > > > + return false; > > > + } > > > + > > > + mTarget.post(new Runnable() { > > > > The fact that we're posting this means it won't be synchronous, which, to > > some extent, defeats the purpose. Perhaps we could synchronize on the > > PanZoomController and run this method directly? I'm not sure if that's > > correct, but do check that out if you can. > > :autra and I discussed this on IRC last week (see discussion starting at > [1]). The gist of it was that making it run directly on the compositor > thread is hard because we'd have to add synchronization to pretty much > everything in PZC. I don't think it's worth doing that at this point > considering we're planning on axing PZC in favour of APZC at some point > anyway. > > [1] http://irclog.gr/#show/irc.mozilla.org/mobile/289140 Ok, that's fine by me.

(In reply to Chris Lord [:cwiiis] from comment #60) > (In reply to Augustin Trancart [:autra] from comment #59) > > Comment on attachment 765957 [details] [diff] [review] > > JavaPanZoomController adaptation to RenderTask > > > > A little comment about this patch : I don't use BOUNCE_ANIMATION_DURATION, > > because this was very misleading : it was compared against mBounceFrame * > > the frame duration in ms, which is not the duration of the animation (it is > > so in the very special case when we draw at 60 fps perfectly, and we don't > > drop any frame). > > When I tried to rely on pure duration, the animation was very snappy and not > > really beautiful (on my emulator, the bounce never had time to bounce back > > and jumped quite violently). That's why I decided to switch back to number > > of runs. The result is closer to the original one, though its speed depends > > on framerate, which was not the case previously. What do you think about > > that? > > I don't think we should have any speed that depends on the frame-rate, it'll > accentuate any instances of jank that occur and might feel odd when we can't > quite hit 60fps and our framerate fluctuates a lot. > > If the previous code relied on number of runs, could we make it a duration > based on the overscroll distance? > > I'll review this patch regardless of this, we can fix this in another > revision. Ok, maybe even in another bug?

(In reply to Chris Lord [:cwiiis] from comment #61) > Comment on attachment 765957 [details] [diff] [review] > JavaPanZoomController adaptation to RenderTask > > Review of attachment 765957 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/gfx/GeckoLayerClient.java > @@ +901,5 @@ > > + > > + @Override > > + public void removeRenderTask(RenderTask task) { > > + mView.removeRenderTask(task); > > + } > > I don't think there's a need or these two functions, you can just call > getView().postRenderTask() instead. > I can't do it directly because the mTarget in JavaPanZoomController is of type PanZoomTarget, which is an interface. So that's why I added the methods to the interface (and implements them in GeckoLayerClient, the implementor). The interface is fine, in my opinion it should also have been used in LayerMarginsAnimator instead of using GeckoLayerClient directly, even if it is the only implementations of PanZoomTarget (contract programming FTW)... Alternatively, instead of exposing postRenderTask and removeRenderTask in the interface, I can expose getView(). But that would expose an internal representation of View to the consumer... What do you think?

(In reply to Chris Lord [:cwiiis] from comment #61) > Comment on attachment 765957 [details] [diff] [review] > JavaPanZoomController adaptation to RenderTask > > Review of attachment 765957 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +904,5 @@ > > > > /* Performs one frame of a bounce animation. */ > > private void advanceBounce() { > > synchronized (mTarget.getLock()) { > > + float t = easeOut( ((float)mBounceFrame) / BOUNCE_FRAME_NUMBER); > > nits, remove the space after the first bracket and remove the braces around > ((float)mBounceFrame). > Yes, it is true that the casts take precedence over the division, nevertheless, I like keeping the braces around to make this clear to the reader (I remove the unnecessary space) so that he does not need to double-check. In this case, I really want to cast mBounceFrame to float before the /, to avoid the integer division, which will always be a dreadful 0 here. Do you prefer it without the braces around (float)mBounceFrame?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #62) > Comment on attachment 765957 [details] [diff] [review] > JavaPanZoomController adaptation to RenderTask > > Review of attachment 765957 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +837,5 @@ > > + /** > > + * Terminate an animation. > > + */ > > + public void terminate() { > > + mContinueAnimation = false; > > This terminate() function is called on the UI thread, which means that > mContinueAnimation must be checked on the UI thread as well. With the code > as you've written it, there is a possible race where mContinueAnimation is > set to false while the compositor thread is in the middle of the internalRun > function, such that the animateFrame() runnable still gets posted. That > means that animateFrame() will run once on the UI thread after terminate() > has already been called on the UI thread, which is bad. I would suggest (1) > changing internalRun so that the only thing it does is post the runnable, > (2) providing a protected final implementation of animateFrame in this base > class that does the mContinueAnimation check/return, and (3) having an > animateFrameInternal or some such abstract method that is abstract for > subclasses to override. In effect you want to push the mContinueAnimation > check inside the UI-thread runnable. Or just check the mContinueAnimation inside the run method of the posted runnable. And still return the mContinueAnimation. Like that, we ensure in a simple way that this boolean is checked when the task is run, and not when the task is posted. And the return is correct because even if the boolean is externally modified before the end of the task, it makes sense to return false in this case (even if the task alone would have liked to return true).

I kept the method in mTarget, waiting for your answer Chris. I've tested the two version (with and without my patch) side-by-side, and as far as I can tell : - There is no visible differences (which is a bit disappointing actually ;-) ) apart from... - ...the speed of bounce animation. I agree with you Chris, we should time-based that, because it is awfully slow on my emulator. - There is maybe a gain in fps. But I can't really tell without proper benchmark. I'll try to do that in the next patch.

Comment on attachment 767801 [details] [diff] [review] JavaPanZoomController adaptation to RenderTask (address comments Review of attachment 767801 [details] [diff] [review]: ----------------------------------------------------------------- This patch addresses all my previous comments, so I'm happy. I didn't look too closely at the mBounceFrame thing, I'll let you and Chris sort that out. ::: mobile/android/base/gfx/GeckoLayerClient.java @@ +10,5 @@ > import org.mozilla.gecko.GeckoEvent; > import org.mozilla.gecko.Tab; > import org.mozilla.gecko.Tabs; > import org.mozilla.gecko.ZoomConstraints; > +import org.mozilla.gecko.gfx.LayerView; Don't need this import ::: mobile/android/base/gfx/JavaPanZoomController.java @@ +825,5 @@ > + @Override > + protected final boolean internalRun(long timeDelta, long currentFrameStartTime) { > + > + mTarget.post(mRunnable); > + return mContinueAnimation; That works, thanks!

Up, I'm waiting for Chris' review!

Comment on attachment 768817 [details] [diff] [review] JavaPanZoomController adaptation to RenderTask (remove useless import) Review of attachment 768817 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, preferably with one nit below fixed - the bounce fix can be done in another patch, let's minimise churn/make review easier. ::: mobile/android/base/gfx/JavaPanZoomController.java @@ +73,5 @@ > > // The maximum zoom factor adjustment per frame of the AUTONAV animation > private static final float MAX_ZOOM_DELTA = 0.125f; > > + // The number of time a bounce animation run at most. We've already discussed that we won't commit this patch until this is time based, I'm adding this comment as a reminder to myself. @@ +818,5 @@ > + > + private boolean mContinueAnimation = true; > + > + public PanZoomRenderTask(boolean aRunAfter) { > + super(aRunAfter); We never call this with true, so no point in having the aRunAfter parameter. Just call super(false) and kill the param.

With this, BounceRenderTask is now completely time-based. There is still another issue: the Fling animation is not time-based, albeit in a subtile way. It uses some velocities, and I think these are assuming a 60 fps display. I'm not yet completely sure of that though. If this is the case, we can weight the velocity according to the ratio 60fps / actual fps in order not to affect prefs and to keep them fps-independant (users with same prefs would have the same visual effects regardless of refresh rate). Does that make sense? I will try to investigate that deeper. Thanks for your comments!

Comment on attachment 771401 [details] [diff] [review] JavaPanZoomController adaptation to RenderTask (remove useless contructor param) Review of attachment 771401 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

Comment on attachment 771406 [details] [diff] [review] Make BounceRenderTask time-based Review of attachment 771406 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me, but I'll let Chris be the official reviewer here.

I've looked to the velocity things in Axis. It actually has a framerate multiplier, which is a constant. If we want everything to be fully time-based, we need to change that and to be able to adjust this multiplier in real time. Fortunately, Axis is only used in JavaPanZoomController. What do you think about that?

Comment on attachment 771401 [details] [diff] [review] JavaPanZoomController adaptation to RenderTask (remove useless contructor param) Review of attachment 771401 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, pending time-based bounce animation.

Comment on attachment 771406 [details] [diff] [review] Make BounceRenderTask time-based Review of attachment 771406 [details] [diff] [review]: ----------------------------------------------------------------- Bar the two nits, looks good to me. ::: mobile/android/base/gfx/JavaPanZoomController.java @@ +73,5 @@ > > // The maximum zoom factor adjustment per frame of the AUTONAV animation > private static final float MAX_ZOOM_DELTA = 0.125f; > > + // The duration of the bounce animation in ns Trailing space. @@ +869,5 @@ > * respectively. > */ > private ImmutableViewportMetrics mBounceStartMetrics; > private ImmutableViewportMetrics mBounceEndMetrics; > + // How long ago this bounce has started in ns. nit: s/has/was/, comma after 'started'.

Hey, I'm not dead! Here is Bounce Render task, with the nits.

I need advice on this one. The result seems good, but I need confirmation that only the Frictions need to be time-adjusted... It solves the slower fling animation I experienced previously. That being said, it is difficult to test this properly, as the emulators are quite slow on my pc (I started two of them at the same time, one with my patches, one without to compare). It might be a good test though, because I can see that the animation takes the same time regardless of the framerate. Again, it is not a very rigorous test.

Comment on attachment 795470 [details] [diff] [review] Make BounceRenderTask time-based (address nits) Review of attachment 795470 [details] [diff] [review]: ----------------------------------------------------------------- Can we just use millis instead of nanos? I recall hearing somewhere that getting the nanotime is more expensive so unless we really need the extra precision I don't know if it's worth it. Also this change will likely affect some of our perf tests so we should run these patches through the try server to make sure we understand and anticipate the changes.

Comment on attachment 795471 [details] [diff] [review] Make FlingRenderTask time-based Review of attachment 795471 [details] [diff] [review]: ----------------------------------------------------------------- This looks ok, except that the value you compute as realNsPerFrame doesn't seem right. Doesn't getStartTime return the time since the start of the animation? I think what you want to use there is the timeDelta value that is passed in to internalRun, no? I'm going to minus this for now but I can r+ it if I'm misunderstanding something and the value is actually the inter-frame interval.

Kats, you are right concerning realNsPerFrame. I'm now wondering how I could have the feeling it was working ;-) Concerning nanos, for a 60 hz display, a frame is 16,6ms. So a ms precision would mean we tolerate an error of about 6% of the duration for each frame. Imho it is acceptable. On the other hand, if we have a 100hz display, the error jumps to 10%, and for 120hz display it is 12,5%. So I have 2 questions: - what is an acceptable precision? Do we really care about that? For an animation of 250ms, it is unlikely that the user would detect an error of less than 10%. But we should talk carefully about that maybe. - What is the likeliness to have display with a different framerate than 60hz? At the moment, my code copes with different framerates mainly to deal with low-spec smartphones which won't manage to keep a fixed framerate (thus their real framerate would be below 60hz anyway, decreasing the need to use ns). This must be put into perspective with the loss of performance by using ns. What do you think about that? I can change back to ms quite easily anyway. But I would need to reupload all the patches :-)

Here is a corrected version of FlingRenderTask, still in ns at the moment.

I realized I made a mistake in my email adress in the patches. Do I need to reupload them all?

Comment on attachment 797246 [details] [diff] [review] Make FlingRenderTask time-based (correct the realFrame duration in ns) Review of attachment 797246 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/gfx/JavaPanZoomController.java @@ +939,5 @@ > } > > @Override > protected void animateFrame() { > + nit: trailing whitespace

(In reply to Augustin Trancart [:autra] from comment #84) > Concerning nanos, for a 60 hz display, a frame is 16,6ms. So a ms precision > would mean we tolerate an error of about 6% of the duration for each frame. > Imho it is acceptable. On the other hand, if we have a 100hz display, the > error jumps to 10%, and for 120hz display it is 12,5%. > So I have 2 questions: > - what is an acceptable precision? Do we really care about that? For an > animation of 250ms, it is unlikely that the user would detect an error of > less than 10%. But we should talk carefully about that maybe. > - What is the likeliness to have display with a different framerate than > 60hz? That's a good point. I would say that it's unlikely we will be rendering at anything other than 60Hz in the forseeable future. And while a 6% loss in precision is acceptable, it's better to have a 0% loss. Since you've already written the code in nanos we might as well keep it that way. (In reply to Augustin Trancart [:autra] from comment #86) > I realized I made a mistake in my email adress in the patches. Do I need to > reupload them all? If you want to, you can. However you don't need to. When it comes time to land the patches, don't set the checkin-needed flag on the bug. Instead, just needinfo me on the bug and I can land your patches and fix the email address while I'm doing that.

I just removed the extra line that kats pointed out in last comment.

Comment on attachment 795470 [details] [diff] [review] Make BounceRenderTask time-based (address nits) Review of attachment 795470 [details] [diff] [review]: ----------------------------------------------------------------- Apart from the one comment, looks good - r+ with that fixed. ::: mobile/android/base/gfx/JavaPanZoomController.java @@ +895,5 @@ > return; > } > > /* Perform the next frame of the bounce-back animation. */ > + mBounceDuration = System.nanoTime() - getStartTime(); You should use the currentFrameStartTime parameter here instead of System.nanoTime(), otherwise linked animations may get subtly out of sync (also to save on the amount of times we call nanoTime).

Comment on attachment 798298 [details] [diff] [review] Make FlingRenderTask time-based (nits) Review of attachment 798298 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the highlighted comment updated. ::: mobile/android/base/gfx/Axis.java @@ +85,5 @@ > }); > } > > static final float MS_PER_FRAME = 1000.0f / 60.0f; > + static final long NS_PER_FRAME = 1000000000 / 60; It's a difference of 1/3ns accuracy, but for the sake of pedantry, perhaps we should put Math.round(1000000000/60.0) here? Your call, I'm happy either way. @@ +91,5 @@ > private static final int FLING_VELOCITY_POINTS = 8; > > // The values we use for friction are based on a 16.6ms frame, adjust them to MS_PER_FRAME: > // FRICTION^1 = FRICTION_ADJUSTED^(16/MS_PER_FRAME) > // FRICTION_ADJUSTED = e ^ ((ln(FRICTION))/FRAMERATE_MULTIPLIER) This comment needs to be updated. @@ +104,1 @@ > VELOCITY_THRESHOLD = 10 / FRAMERATE_MULTIPLIER; It's weird how this gets defined... Should it just be a constant instead? Kind of unrelated to this patch though, so don't worry. ::: mobile/android/base/gfx/JavaPanZoomController.java @@ +824,5 @@ > > + /** > + * The current frame duration in ns. > + */ > + protected long mRealNsPerFrame; Naming here is a bit odd to me - why not just mLastTimeDelta, or mLastFrameTimeDelta to be extra clear?

This is good work here, excited to see this land and increase the consistency of our flinging animations :)

Thanks for the reviews! (In reply to Chris Lord [:cwiiis] from comment #91) > It's a difference of 1/3ns accuracy, but for the sake of pedantry, perhaps > we should put Math.round(1000000000/60.0) here? Your call, I'm happy either > way. Actually 2/3ns, as it truncates the value the way it is (Integer division). Let's not miss an occasion of being safely pedantic (you don't have so many of them)! For the comments, I removed the explanation of the calculus that was outdated as well. I think that it is unnecessary, as the calculus can be understood from the code (it is not even a javadoc anyway). I upload new version of these patches asap!

Here is the new version of BounceRenderTask

And here is FlingRenderTask

Comment on attachment 798563 [details] [diff] [review] Make BounceRenderTask time-based (use currentFrameStartTime instead of System.nanoTime()) Review of attachment 798563 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review for Chris too since he's gone for two months.

Some of the patches needed rebasing, so I rebased them and fixed up the email address. I pushed them to try just to make sure my rebasing was correct: https://tbpl.mozilla.org/?tree=Try&rev=b310a0ef9399 Once the try run is green I will land them. Flagging myself as needinfo so I don't forget.

Try run looks good. Landed on fx-team: https://hg.mozilla.org/integration/fx-team/rev/d6515c4d6cf8 https://hg.mozilla.org/integration/fx-team/rev/05c2d7b68a22 https://hg.mozilla.org/integration/fx-team/rev/f746195f5595 https://hg.mozilla.org/integration/fx-team/rev/0fb65049cb95 https://hg.mozilla.org/integration/fx-team/rev/f13c3491c9c8

https://hg.mozilla.org/mozilla-central/rev/d6515c4d6cf8 https://hg.mozilla.org/mozilla-central/rev/05c2d7b68a22 https://hg.mozilla.org/mozilla-central/rev/f746195f5595 https://hg.mozilla.org/mozilla-central/rev/0fb65049cb95 https://hg.mozilla.org/mozilla-central/rev/f13c3491c9c8