Bug 115896 – [BlackBerry] Use requestAnimationFrame for animations

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Carlos Garcia Campos]

This way we don't need the changes proposed in bug #114501 to get 60fps in the BlackBerry port.

[Carlos Garcia Campos]

Created attachment 201330 [details]
Patch

[Arvid Nilsson]

Comment on attachment 201330 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=201330&action=review

The only call that will happen on a non-UI thread is animationFrameChanged(), perhaps we can invent a "weak" synchronization scheme similar to void WebPagePrivate::updateDelegatedOverlays(bool duringCommit, bool dispatched) to avoid using a mutex.

> Source/WebKit/blackberry/Api/WebPage.cpp:6333
> +    BlackBerry::Platform::AnimationFrameRateController::instance()->lockMutex();

Since addClient() already locks/unlocks the internal mutex, is this external lock/unlock pair really necessary? Perhaps it tries to use the mutex to protect m_isRunningRefreshAnimationClient, since stopRefreshAnimationClient is sometimes called on the UI thread (when it's called from animationFrameChanged)?

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 201330 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201330&action=review
> 
> The only call that will happen on a non-UI thread is animationFrameChanged(), perhaps we can invent a "weak" synchronization scheme similar to void WebPagePrivate::updateDelegatedOverlays(bool duringCommit, bool dispatched) to avoid using a mutex.
> 
> > Source/WebKit/blackberry/Api/WebPage.cpp:6333
> > +    BlackBerry::Platform::AnimationFrameRateController::instance()->lockMutex();
> 
> Since addClient() already locks/unlocks the internal mutex, is this external lock/unlock pair really necessary? Perhaps it tries to use the mutex to protect m_isRunningRefreshAnimationClient, since stopRefreshAnimationClient is sometimes called on the UI thread (when it's called from animationFrameChanged)?

I took this approach from the BlackBerry::Platform::AnimationBase implementation, but I think the mutex in this case is to call frameChanged with the lock, so maybe we don't need it here.

[Carlos Garcia Campos]

Created attachment 201336 [details]
Updated patch

Removed the mutexes and added early return in scheduleAnimation

[Arvid Nilsson]

Comment on attachment 201336 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=201336&action=review

> Source/WebKit/blackberry/Api/WebPage.cpp:6312
> +void WebPagePrivate::animationFrameChanged()

Now to be sure there are memory barriers all over the place (when dispatching messages, locking mutexes etc)
but just looking at these two methods in isolation...

> Source/WebKit/blackberry/Api/WebPage.cpp:6329
> +}

In the complete absence of memory barriers, scheduleAnimation() could set m_animationScheduled to true on Core 1, but that change is not synced over to Core 2, and when core 2 executes animationFrameChanged(), it will just stop and return.

[Arvid Nilsson]

there's also a "one shot" mode of operation for AnimationFrameRateController that might be more appropriate for the RAF usage, see WebPageCompositorPrivate for an example of one shot AFRClient
When useing the one shot flavour, you don't have to manually remove yourself

[Andrew Lo]

Comment on attachment 201336 [details]
Updated patch

looks good to me

[Carlos Garcia Campos]

Created attachment 201542 [details]
Updated patch

Added a mutex following the same approach as DisplayRefreshMonitor

[Carlos Garcia Campos]

Created attachment 201544 [details]
Another update

Sorry, attached the wrong patch, this one should apply.

[Arvid Nilsson]

Comment on attachment 201544 [details]
Another update

This looks pretty much exactly like the old code, and looks threadsafe to me. Informal R+

[Build Bot]

Comment on attachment 201544 [details]
Another update

Attachment 201544 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/460868

New failing tests:
editing/unsupported-content/table-delete-003.html
editing/unsupported-content/list-type-before.html
editing/unsupported-content/table-type-before.html
editing/unsupported-content/list-delete-003.html
editing/unsupported-content/list-delete-001.html
editing/unsupported-content/list-type-after.html
editing/unsupported-content/table-delete-002.html
editing/unsupported-content/table-type-after.html
editing/unsupported-content/table-delete-001.html

[Build Bot]

Created attachment 201648 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Rob Buis]

Comment on attachment 201544 [details]
Another update

Ok.

[WebKit Commit Bot]

Comment on attachment 201544 [details]
Another update

Clearing flags on attachment: 201544

Committed r150071: <http://trac.webkit.org/changeset/150071>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.