#494 ✓ staged
brianchirls

per frame animation

Reported by brianchirls | April 30th, 2011 @ 05:01 PM | in 0.9 (closed)

Sure would be useful to be able to animate active Popcorn events. I took a stab at it. Check out my "animate" branch:
https://github.com/brianchirls/popcorn-js/tree/animate

There is also a working example. I modified Scott's lowerthird plugin, now under lowerthird-anim.

The main Popcorn loop now keeps a list of any active tracks with a .frame() method and calls it on every frame, passing the currentTime.

It looked pretty awful, since the timeupdate event doesn't get called often enough for a decent frame rate. So I also modified this branch to use requestAnimationFrame instead. I kept the timeupdate listener to save the event object and pass it to the plugin for backward compatibility. But it doesn't seem very useful, so I wouldn't mourn its loss if we ever got rid of it altogether.

It might be helpful to add a few utility functions for different kinds of tweening.

Comments and changes to this ticket

  • Rick

    Rick April 30th, 2011 @ 05:24 PM

    Is this on your 0.5 branch? The upstream is at 0.6 and you should move this to a branch called "t494" so it doesn't conflict with your upstream branches.

  • Rick

    Rick April 30th, 2011 @ 05:27 PM

    Nevermind, I missed the "animate" branch...

  • David Humphrey

    David Humphrey April 30th, 2011 @ 05:49 PM

    I do exactly this in the code plugin, which does frames between start/end.

  • Rick

    Rick April 30th, 2011 @ 05:53 PM

    • State changed from “new” to “review-needs-work”

    For new features such as this, it's best if a ticket is filed first so that a discussion can be had about the merits of the idea, use cases and real world application. Core changes are especially sensitive, even the player plugins were granted only a minor change in the core to accomodate passing a DOM element or "video-like" object.

    Initial review notes...

    • Any change to the core must ensure 100% pass to the existing tests. See list below **.
    • Any enhancement or feature that involves changing the core must be accompanied by unit tests
    • New Plugins should be filed separate from core changes
    • New Plugins require 4 files: source, demo, unit tests and unit test runner
    • If you use code that is copied directly from another source, you absolutely must credit the source. See: http://paulirish.com/2011/requestanimationframe-for-smart-animating/
    • If you copy code that is from another source, in addition to being creditted, it must also be formatted to conform to the project style guide.
    • Please review: https://webmademovies.lighthouseapp.com/projects/63272/styleguide
    • "window" should be replaced by the explicitly provided "global"

    Linting Popcorn.js with your changes results in 14 lint errors.

    Failing Core Tests (fail, pass, total) **

    • Popcorn Methods: exec (2, 0, 2)
    • Popcorn Position: position called from plugin (2, 1, 3)
    • Popcorn Plugin: Manifest (1, 4, 5)
    • Popcorn Plugin: Update Timer (1, 0, 1) --- THIS IS MAJOR
    • Popcorn Plugin: Plugin Factory (3, 17, 20)
    • Popcorn Plugin: Plugin Breaker (3, 3, 6)
    • Popcorn Plugin: Plugin Closure (1, 46, 47)
    • Popcorn Plugin: Remove Plugin (5, 51, 56)
    • Popcorn Parser: Parsing Integrity (1, 19, 20)
    • Popcorn Parser: Parsing Handler - References unavailable plugin (1, 2, 3)
    • Audio: Parser Support (6, 4, 10)

    409 tests of 435 passed, 26 failed.

  • brianchirls

    brianchirls April 30th, 2011 @ 06:12 PM

    I haven't submitted it formally, Rick. It's just a proof of concept. This IS the discussion.

    But thank you for the reference material.

  • Rick

    Rick April 30th, 2011 @ 06:16 PM

    Sorry, I guess I misunderstood and assumed it was since you created a ticket in the project issue tracker. I too have plugins that are not submitted formally, but also have never filed a ticket.

  • David Humphrey

    David Humphrey April 30th, 2011 @ 06:20 PM

    This is something we should put some thought to in another ticket, namely, the place for people to take cool plugins that aren't necessarily going to land in the tree (Brian, I'm not commenting on yours, just speaking in general), and issues with the core/core plugins. We really need to make this easy for both groups, especially now that the plugin stuff is spurring people to do cool things. It's very much something we want to encourage, and that means we need to find a home for it. I'll file a bug.

  • David Humphrey

    David Humphrey April 30th, 2011 @ 06:25 PM

    Filed #495 to address the issue raised in comment 8.

  • brianchirls

    brianchirls April 30th, 2011 @ 06:28 PM

    Yeah there didn't seem to be a better place for discussion.

  • brianchirls

    brianchirls May 1st, 2011 @ 01:21 PM

    Humph,

    I took a look at your code plugin. Very interesting. It brings a number of issues to mind:

    1. In my experience working on a number of real-world-ish Popcorn projects, there's a fair bit of demand for more advanced control over visuals, the kind that would require per-frame animation.

    2. I find myself building my own custom plugins just about every time. The existing ones are great models to build on, but they're almost always too specific. e.g., they set specific styles and assume a certain document structure.

      This is why #495 is a good idea. But the way your code plugin handles this is a bit complicated to re-write in so many plugins, in particular if they're for one-time use. This holds especially true if Popcorn.js is to be available to inexperienced programmers, like many of the ones we ran into at Buttercamp and in Ben's class at NYU. It's a credit to the Popcorn.js design that plugins can be quite simple, and it should stay that way.

    3. Consider a scenario in which there are dozens of tracks, or more. Almost all my clients and filmmakers have asked about automating Popcorn content, so this is quite realistic. Would it be efficient to have a separate requestAnimationFrame/setTimeout loop running for every single one of those? Humph, you know the browser internals much better than I do, so what do you think?

    I'm sure there are other ways of coding this than my version, but the overhead seems quite low, and it's a useful enough feature that it's worth considering adding to the core.

  • Rick

    Rick May 1st, 2011 @ 07:52 PM

    Can you provide examples of the real-world-ish projects that you've mentioned? I'd like to see the resulting code changes that were required to meet your needs.

  • Brett Gaylor

    Brett Gaylor May 10th, 2011 @ 05:12 PM

    Hey, I wanted to revisit this ticket, but before I do, lets endeavor to keep things civil and focus on the specific issues of the ticket.

    So if I'm understanding correctly, and Brian is facing issues with his clients that require
    a) tweening of events, and
    b) greater accuracy over frames

    Then that does sound like something that should be addressed in our core down the line. Humph, what do you make of Brian's specific proposal and follow up questions?

  • David Humphrey

    David Humphrey July 20th, 2011 @ 03:31 PM

    • Assigned user set to “brianchirls”
    • Milestone set to 0.8
    • Milestone order changed from “55” to “0”
  • Rick

    Rick July 24th, 2011 @ 01:40 PM

    Brian,

    You should coordinate your updates with Dave Seifried, Scott Downe and myself; Dave is currently undertaking the process (with support from Scott and I) of moving the time update callback out to it's own static function, located at Popcorn.timeUpdate() -- this would be the equivalent of your updateTrackEvents function expression.

    (Note: This is also going to serve as the basis for non-moving track updates (needed to efficiently address a number of issues, including your toggle ticket). )

    Despite a few style guide nits, your branch appears to be passing the 0.7 release test suite 100% now that you've made the requestAnimationFrame logic optional, this approach is ideal. I'd like to get your t494 branch reviewed by everyone and ideally landed for 0.8 - I'd like to avoid any long term divergence.

    I think the next move is to finish up the work Dave and Scott are doing, get it into the upstream development branch and then integrate your approach with that

  • Rick

    Rick July 24th, 2011 @ 01:41 PM

    • no changes were found...
  • Rick

    Rick July 24th, 2011 @ 01:41 PM

    I've added Dave, Chris and Scott to the notifications list

  • brianchirls

    brianchirls July 24th, 2011 @ 01:45 PM

    Thanks, Rick.

    Yes, you're absolutely right. I have an email out to Scott about setting up a call for Monday. Scott and Dave, if you're on here, please get in touch.

    Yes, the tests are passing right now. I want to tweak a couple things and add the .frame event to plugins as well as a proper demo. Should have that by tomorrow. I'll take another pass with the style guide when I clean it up.

    B

  • Scott Downe

    Scott Downe July 24th, 2011 @ 04:46 PM

    I'm up for a call Monday.

    Me and Dave will be in.

    It may also be good to have Rick and Bobby on the call if they are free.

    I also have not received an email from you Brian, could you PM me in irc some time and I'll make sure you have my correct email?

  • Bobby Richter

    Bobby Richter July 24th, 2011 @ 05:02 PM

    Certainly available. Glad to see this progressing.

  • Rick

    Rick July 24th, 2011 @ 07:00 PM

    I feel like one of those old school switch board operators hahaha :)

  • Rick

    Rick July 25th, 2011 @ 01:43 PM

    I did a trial run of combining dseif's branch with this current work and it came together really easily, it really just boils down to a handful of copy/paste tasks, rename t494's updateTrackEvents to Popcorn.timeUpdate, change the function invocations to be .call( this, ... ) and that's basically it.

  • brianchirls

    brianchirls August 3rd, 2011 @ 03:20 PM

    • State changed from “review-needs-work” to “peer-review-requested”

    Ok, everyone. Have at it:
    https://github.com/brianchirls/popcorn-js/blob/t494/popcorn.js

    Some notes notes on the unit tests...

    Per Bobby's suggestion, I created a whole separate unit test file (test/anim.html) that's a duplicate of the original, with every instance of popcorn given the frameAnimation option.

    I'm aware that this fails a whole range of existing unit tests. There are at least two issues here. First is that there are a number of existing bugs having to do with asynchronicity that this changes exposes as a result of the main update loop running more often. The second is that the added frame animation stuff runs code after .start and .end, running into trouble caused by exec() calls and other plugins that make changes to popcorn.

    I've already filed tickets on two of these bugs, and I'm sure there are more. Even before I started writing this code, there have been a number of tests that fail some times but not others.

  • Rick

    Rick August 15th, 2011 @ 12:37 PM

    Today at the jQuery core devs meeting, we voted to remove requestAnimationFrame from jQuery completely, with the possibility of re-introduction at a later time - to be determined by improved stability of the API and its implementation across different browsers. We've run into a number of issues with bizarre, unexpected behaviour most of which can be reviewed by starting here: http://bugs.jquery.com/ticket/9381

    I'm not really sure what this means for Popcorn and the status of this ticket, but I would be remiss if I didn't relay the information and ultimate decision.

  • brianchirls

    brianchirls August 15th, 2011 @ 01:09 PM

    Hey Rick, thanks for sharing this. Good stuff.

    On one hand, I wouldn't think this is a problem, since that is exactly the point of requestAnimationFrame. But on the other, a rush of updates when the focus/visibility returns would be very bad, especially if reflows are required. I do think this would be less of a problem for Popcorn than for jQuery, since it seems less likely that users would leave an interactive video playing in the background - but, then, I've been known to do it myself.

    It's worth thinking about implementing our own wrapper function that would use requestAnimationFrame normally, but intercept it when it's not working. One option would be to run an interval every second or so that checks if an update hasn't been run in a while and then force one. That would replicate what rAF is actually supposed to do. Do you know if it's possible/easy to directly detect when the browser window is invisible? Another option would be to keep an eye on that and then swap out rAF for setTimeout(1000).

    I've been thinking of doing something like this on another project to implement the second parameter on rAF, since most (all?) browsers don't yet do that. But I don't think that'd be appropriate for Popcorn, since you're likely to have updates all over the page.

    Anyway, I do hesitate to completely replace requestAnimationFrame with setTimeout, since it does offer some good features. We can expect a lot of reflows with Popcorn, and it would be nice to avoid those when they're not visible.

  • Rick

    Rick August 15th, 2011 @ 01:31 PM

    At this point, I'm certainly not suggesting replacing requestAnimationFrame with anything - as there is nothing to replace since we haven't landed it in the Popcorn core yet. The fact is, Paul's fallback is already going to use setTimeout if rAF isn't found.

    I'm going to experiment with some window focus/blur ideas I have that might circumvent out-of-focus issues. In particular, I'm thinking that if the window is out of focus, then rAF does nothing... I'll post later

  • Rick

    Rick August 26th, 2011 @ 04:51 PM

    • State changed from “peer-review-requested” to “review-needs-work”

    This needs to be updated with the new develop branch

  • Scott Downe

    Scott Downe August 29th, 2011 @ 10:52 AM

    I feel this can land in 0.9.

    All things considered (timeupdate just landed, and the issues regarding focus) I think that's fair.

    I am leaving it open for 0.8, but the window is closing and there is no rush, imo.

  • Scott Downe

    Scott Downe August 29th, 2011 @ 11:01 AM

    • Milestone changed from 0.8 to 0.9
  • brianchirls

    brianchirls August 29th, 2011 @ 11:11 AM

    I need some time to review timeupdate changes anyway, so that makes sense.

    Rick, how did your focus experiments go?

  • Rick

    Rick August 29th, 2011 @ 11:18 AM

    No findings of notable use.

    Scott and I discussed this and both agree that despite the potential awkward behaviour that may occur when focus is lost and regained, we should still move forward with including this in its opt-in form.

  • brianchirls

    brianchirls September 2nd, 2011 @ 05:15 PM

    • State changed from “review-needs-work” to “peer-review-requested”

    Here is my rewrite, to work with the new timeUpdate.
    https://github.com/brianchirls/popcorn-js/commit/1d45eb48926036b8f6...

    I will keep an eye on requestAnimationFrame weirdness. I haven't had any problems with it in Popcorn, but it was giving me some trouble with Seriously, so I will be experimenting with a few different options and will post another ticket if I find anything.

  • Rick

    Rick September 2nd, 2011 @ 05:36 PM

    • State changed from “peer-review-requested” to “review-needs-work”

    I think something went wrong with your branch or your commit:

    "Showing 71 changed files with 2,134 additions and 1,559 deletions."

    You may want to start with a fresh branch from an updated and rebased master and apply your changes there.

  • brianchirls

    brianchirls September 2nd, 2011 @ 05:40 PM

    Oops, I may have pulled and made the changes all in one commit. Perhaps a bit of a mess. Yeah, I'll do that.

  • brianchirls

    brianchirls September 6th, 2011 @ 12:28 PM

    • State changed from “review-needs-work” to “peer-review-requested”
  • Rick

    Rick September 6th, 2011 @ 12:48 PM

    • State changed from “peer-review-requested” to “under-review”
    • Assigned user changed from “brianchirls” to “Rick”
  • Rick

    Rick September 6th, 2011 @ 01:50 PM

    There are no tests, so I'm not sure what to base a review beyond syntax issues. The upside is that none of this breaks the current API.

    I've added line comments to the commit. Additionally, this is failing lint:

    
    Linting popcorn.js
    
            if (tracksAnimating[animIndex]._running == false) {
    
        Problem at line 981 character 49: Use '===' to compare with 'false'.
    
            if (tracksAnimating[animIndex]._running == false) {
    
        Problem at line 1036 character 49: Use '===' to compare with 'false'.
    

    ... These can actually be changed to something even more simple with if ( !tracksAnimating[ animIndex ]._running ) {

  • Rick

    Rick September 6th, 2011 @ 01:52 PM

    Sorry, I spoke too soon.

    Uncaught TypeError: Cannot read property '_natives' of undefined, popcorn.js:968

  • brianchirls

    brianchirls September 6th, 2011 @ 02:32 PM

    • State changed from “under-review” to “peer-review-requested”

    Updated:
    https://github.com/brianchirls/popcorn-js/commits/t494

    • reformatted per Rick's comments
    • fixed _natives TypeError
    • added anim.html for separate unit tests
  • Rick

    Rick September 6th, 2011 @ 03:47 PM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “Rick” to “brianchirls”

    Great, thanks for adding in the tests.

    Currently, when I run test/anim.html, I get the following failures:

    http://gyazo.com/f672fc422e088b34528dc79a67b4a424.png

  • Rick

    Rick September 6th, 2011 @ 03:52 PM

    With regard to the tests...

    Because this is an option of the core library, the tests need to be in the same test suite from the same test files as the rest of the core. There is only one exception to this, which is the Popcorn(function() { // dom ready }) test, which stands alone for obvious reasons.

    Please add your test units to the core test suite in their own test blocks. Be sure that no other tests are modified and that your test properly cleans up after itself.

    Thanks!

  • brianchirls

    brianchirls September 7th, 2011 @ 12:53 AM

    • State changed from “review-needs-work” to “blocked”

    All of those tests pass for me when they are run individually.

    The "Frame Animation" test is giving an error message from the test before it ("disable/enable/toggle" because the other test does not clean up after itself properly, and its custom plugins are still running during my test.

    Popcorn.removeInstance doesn't do anything besides removing a reference. It should tear down all the tracks and detach all the event listeners. And once that's done, I will have the animate loop check that the instance is still good and abort the loop if not.

    I've logged a new ticket #701 for this issue, and will block this one until that's fixed.

  • brianchirls

    brianchirls September 7th, 2011 @ 01:15 AM

    Index Integrity test was failing because I was using an old version of it from before #675. When this ticket is unblocked and I re-integrate the tests, this problem will go away.

  • Rick

    Rick September 7th, 2011 @ 10:24 AM

    The problem with listing all of the reasons why your test doesn't work in the test suite is that all of the other tests run exactly as expected. I responded to #710 and I don't think that should block anything... changing the core library to accommodate the failing tests of an optional feature is a road we should avoid.

    Popcorn.removeInstance doesn't do anything besides removing a reference. It should tear down all the tracks and detach all the event listeners.

    See #710

  • brianchirls

    brianchirls September 7th, 2011 @ 10:33 AM

    I'm not advocating changing the core library to accommodate my failing tests. I'm advocating a change in the core library to fix a test that is itself broken. There's no way my test should be showing an error message from a previous test unless that test didn't clean up after itself, either because removeInstance didn't do what it's supposed to or because the tests shouldn't be using removeInstance.

    See #710

  • Rick

    Rick September 7th, 2011 @ 11:07 AM

    I'm out of the office until about 12:30, when I get back I'm going to help get these tests to full pass. At this point I still think that tests can be written without making any changes to the api. If I am wrong, we will take it from there. I only want to make sure all possibilities are exhausted.

  • Rick
  • Rick
  • Rick

    Rick September 7th, 2011 @ 01:46 PM

    • State changed from “blocked” to “peer-review-requested”
    • Assigned user changed from “brianchirls” to “Scott Downe”

    First draft set of complete and passing tests, representative of behavioural parity between the two playback methods:

    https://github.com/rwldrn/popcorn-js/tree/t494-valid-tests

    Scott, can you confirm these tests for me? Thanks!

  • Rick

    Rick September 7th, 2011 @ 01:47 PM

    I just realized this will need to be updated with the latest develop branch.

  • Rick
  • Scott Downe

    Scott Downe September 8th, 2011 @ 09:51 AM

    • State changed from “peer-review-requested” to “under-review”
  • Rick
  • Rick
  • Scott Downe

    Scott Downe September 8th, 2011 @ 12:42 PM

    • State changed from “under-review” to “review-needs-work”
    • Assigned user changed from “Scott Downe” to “Rick”

    So, I like where this is going in regards to rAF.

    I do have issues with plugin.frame(), though.

    To me, RAF and plugin.frame() and not inclusive; we can have one without the other. If I understand plugin.frame(), it is a function that is called on every rAF. If so, why not do the same functionality on timeupdate? each time timeupdate is called, fire plugin.frame().

    I don't see any tests for plugin.frame().

    Finally, the tests for Index Integrity (frameAnimation) need some whitespace work.

    Not exactly sure who to assign this back to, Rick or Brian, so I will assign it back to Rick, because that's who assigned it to me.

  • Rick
  • Rick
  • Rick
  • Scott Downe

    Scott Downe September 8th, 2011 @ 02:29 PM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user changed from “Rick” to “David Seifried”

    ok, I am satisfied.

    Tests pass, linting passes.

    I ran it through the original demo, changing it to use request animation frame, ran beautifully.

    I think because of Dave's experience in time update, he's the one to do the next review.

  • David Seifried

    David Seifried September 9th, 2011 @ 12:12 PM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “David Seifried” to “Rick”

    Alright a few small nitpicks with styling

    In popcorn.unit.js:

    On line 1485

    
    test("frame function (frameAnimation)", function() {
    

    There can be a space after the first round bracket.

    Line 1487 also has the same problem

    
    var $pop = Popcorn("#video", { frameAnimation: true }),
    

    1492 can have padding around expects

    
    expect(expects);
    

    1497 can also use some whitespace padding

    
    Popcorn.removePlugin("frameFn");
    

    Other than this everything looks good to me. Testing on a bunch of plugin demo's and everything runs awesomely. Passes lint.

    Tested on:

    FF 6
    Chrome 12

    After those few styling fixes this should be good to stage.

  • David Seifried

    David Seifried September 9th, 2011 @ 01:25 PM

    • State changed from “review-needs-work” to “review-looks-good”

    There is a ticket being created to fix all of the style fixes in the unit tests at the moment, so we can stage this and fix all the style changes at once. This is good to go, awesome that this is finally getting in.

    SR+!

  • David Seifried

    David Seifried September 9th, 2011 @ 03:45 PM

    • Assigned user changed from “Rick” to “cadecairos”
  • cadecairos

    cadecairos September 9th, 2011 @ 03:48 PM

    • State changed from “review-looks-good” to “staged”

    staged: https://github.com/cadecairos/popcorn-js/commit/bcff31079a51384e09d...

    Creating a ticket for the index integrity test cleanup.

  • Rick
  • brianchirls

    brianchirls September 13th, 2011 @ 05:48 PM

    Thank you, everyone, for helping to land this feature. It will be most useful.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Popcorn.js is an HTML5 video framework that lets you bring elements of the web into your videos.

Popcorn.js is a project of Web Made Movies, Mozilla's Open Video Lab.

Shared Ticket Bins

Referenced by

Pages