#118 ✓ staged
gerv

Use ISO8601 time formats

Reported by gerv | October 19th, 2010 @ 11:43 AM | in 0.5

The acceptable time formats (HH:MM:SS:DD, MM:SS:DD, SS:DD, SS) are inconsistent with each other. It's not clear whether 13:45 is SS:DD or MM:SS. There are standard ways of writing times, and we should use them.
http://en.wikipedia.org/wiki/ISO_8601

(This would be HH:MM:SS.DDD).

Gerv

Comments and changes to this ticket

  • Scott Downe

    Scott Downe October 19th, 2010 @ 03:38 PM

    • State changed from “new” to “assigned”
    • Milestone cleared.
    • Assigned user set to “Scott Downe”
    • Milestone order changed from “12” to “0”
  • Scott Downe

    Scott Downe October 19th, 2010 @ 04:09 PM

    I think the initial intent was to try and emulate "frames" in the final iteration, which is not intended to be milliseconds. It does handle SS.DDD if you specify a decimal, though.
    HH:MM:SS.DDD:FF with FF being "frames". If you specified 01 in the "frames" iteration, it would try and make the closest guess, in milliseconds, to the first frame in that second. Never worked as well as we hoped in the current spec, and personally, I preferred just decimal from the beginning, which is why I made sure SS.DDD did still in fact work.

    I think removing the "frames" hack altogether will solve this. We will end up with HH:MM:SS MM.SS SS with .DDD being optional for seconds in each case.

    Anyway, I will make that change and see what everyone thinks.

    Oh, and the comments for the toSeconds function need to be updated.

  • Scott Downe

    Scott Downe October 19th, 2010 @ 04:18 PM

    Something like this, imo, is simple and gets the job done, and still supports milliseconds, which is the smallest form of time we can handle.

      // Simple function to convert 04:00 to 240 in seconds
      // acceptable formats are HH:MM:SS, MM:SS, SS
      var toSeconds = function(time) {
        var t = time.split(":");
        if (t.length === 1) { // SS
          return parseFloat(t[0], 10);
        } else if (t.length === 2) { // MM:SS
          return parseInt(t[1] * 60, 10) + parseFloat(t[2], 10);
        } else if (t.length === 3) { // HH:MM:SS
          return parseInt(t[0] * 3600, 10) + parseInt(t[1] * 60, 10) + parseFloat(t[2], 10);
        }
    
  • annasob

    annasob November 3rd, 2010 @ 11:36 AM

    • Milestone cleared.

    popcorn-js@@@ @@@ [bulk edit]

  • annasob

    annasob November 3rd, 2010 @ 11:36 AM

    • Milestone set to 0.2 release
    • Milestone order changed from “28” to “0”
  • Brett Gaylor

    Brett Gaylor November 15th, 2010 @ 04:13 PM

    I'd still like to see an option for frames. Could this perhaps be an external plugin? ie if you want to specify HH:MM:SS;FF, you can add a seperate JS file? Otherwise it will only go to HH:MM:SS.MS?

  • Scott Downe

    Scott Downe November 16th, 2010 @ 09:58 AM

    We cannot truly support frames anyway; it was a hack, and a pretty bad one now that I think of it. We basically took the most precise increment people have access to, and made it less precise, and more confusing.

    But the idea of supporting frames as an option isn't such a bad idea, and it should be accessed via the XML. But I don't think it should be a toggle that changes the way the timecode is handled, but some way to specify the frames inside the in and out. Something along these lines.

    <subtitle in="HH:MM:SS.DDD FF"
    

    Using a completely different separator, I used a space in the above example, means we can separate its meaning from the time, and we wouldn't have the confusion of FF being interpreted as DDD, and we could be sure that HH:MM:SS MM:SS SS can all be supported and understood. So inside the code we say split on space (or whatever separator we use), then convert the first item (which is the timecode) into seconds, and if a second item exists (frames) convert that to the 1/30th frame, as milliseconds, and add that to the seconds.

  • Brett Gaylor

    Brett Gaylor November 16th, 2010 @ 10:47 AM

    The thing about frames, though, is it depends upon the frame rate that
    the video was encoded - this is another reason to approach this as an
    external plug-in. The creator can either specify time in SS:DDD, or
    add an external file which they would have to edit to specify what
    frame rate they encoded the video in. Seconds and milliseconds will
    remain no matter what frame rate you use. So your above example makes
    sense until the very end, where you mention that you divide it by
    1/30th. Doesn't make sense if its encoded at 24fps, or 12fps, etc.

  • Scott Downe

    Scott Downe November 16th, 2010 @ 11:29 AM

    Yeah, I didn't think we had control of how the video element plays it back. I was under the impression the video element didn't care, and we targeted 30fps because that is what we decided with. Although, I see your point. If we are going to emulate frames, we should emulate the original encoded rate. There is no way we can get this rate from the video itself, the user must specify this as well.

    I still think doing this in the XML is best, including specifying the frame rate. Something like:

    <subtitle in="HH:MM:SS.DDD FF/rate"
    

    So, an example of this would be:

    <subtitle in="00:00:10 01/24"
    

    That would get the first frame out of 24 a second, or something like this.

  • Scott Downe

    Scott Downe November 16th, 2010 @ 02:31 PM

    Another way might be to have an encoded, or frames attribute, and give it a number. That way you can specify it once at the top most element, and never have to set it again. Then inside the in and out, specify the frames like this:

    <subtitle in="00:00:10 01" frames="24"
    

    Would be similar to the above comment, cept you can specify frames at the top most element, and have it cascade to all containing elements (we already support this) This also allows future use of the sequencer, which may have multiple videos attached to one Popcorn instance, but with more than one encoded rate.

    This is considering the future when the sequencer exists, and when proper frame values exist in the video element.

  • Scott Downe

    Scott Downe November 16th, 2010 @ 02:36 PM

    and the above frames attribute is optional.

  • Scott Downe

    Scott Downe January 8th, 2011 @ 11:34 AM

    • Milestone changed from 0.2 release to 0.3 release date
    • Milestone order changed from “24” to “0”
  • Scott Downe

    Scott Downe January 18th, 2011 @ 10:37 AM

    • Tag set to starter
    • Assigned user cleared.
  • Scott Downe

    Scott Downe January 18th, 2011 @ 10:37 AM

    • State changed from “assigned” to “open”
  • edsfault

    edsfault January 20th, 2011 @ 11:16 AM

    • Assigned user set to “edsfault”
  • annasob

    annasob February 8th, 2011 @ 05:32 PM

    • Milestone changed from 0.3 release date to 0.4
    • Milestone order changed from “7” to “0”
  • edsfault

    edsfault February 12th, 2011 @ 04:35 PM

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

    I believe have correctly added support for the framerate information in the following formats:

    SS F/FR
    SS:MMM F/FR
    MM:SS:MM F/FR
    HH:MM:SS:MM F/FR

    Where F is the targeted frame, and FR is the framerate in which the video was recorded

    https://github.com/edsfault/Edsfault-popcorn-js/commit/a1549edb1c89...
    https://github.com/edsfault/Edsfault-popcorn-js/tree/a1549edb1c89f1...

  • Scott Downe

    Scott Downe February 14th, 2011 @ 10:21 AM

    • State changed from “peer-review-requested” to “under-review”
    • Assigned user changed from “edsfault” to “Scott Downe”
  • Scott Downe

    Scott Downe February 14th, 2011 @ 11:01 AM

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

    Make sure your code complies with the style guide used by the project found here: https://webmademovies.lighthouseapp.com/projects/63272/styleguide

    I am also going to ask you that you include an example or a unit test of sorts. I am not sure how this will look though. Probably be best to add some tests to this file https://github.com/edsfault/Edsfault-popcorn-js/blob/master/parsers...

    Now, I think we should break the API here and support the standards. Now that we have a way to pull in frames, we should support this:

    SS.DDD F/FR
    MM:SS.DDD F/FR
    HH:MM:SS.DDD F/FR

    SS.DDD
    MM:SS.DDD
    HH:MM:SS.DDD

    SS
    MM:SS
    HH:MM:SS

    SS F/FR
    MM:SS F/FR
    HH:MM:SS F/FR

    This way DDD and F/FR are independent from each other, and will not be confused, and we support the HH:MM:SS.DDD standard.

    It is a small fix, just remove all references to parseFloat(t[1] / 12, 10) and make sure your length checks are accurate as any decimals will not need to be checked as an int and decimal are the same in JavaScript.

    You solution passes, just fix up the styling, add a test, and remove the parseFloat(t[1] / 12, 10) references and this will pass. Good work.

  • Brett Gaylor

    Brett Gaylor February 14th, 2011 @ 11:50 AM

    How do different frame rates used in video files effect this? for example, some may be 12fps, some 24fps, some 30fps etc.

  • edsfault

    edsfault February 14th, 2011 @ 07:09 PM

    In
    HH:MM:SS.DDD
    D represents milliseconds. Right?

    So:
    00:01:38.329
    Means minute 1, second 38, 329 milliseconds.

    And
    00:01:38 2/24
    Means minute 1, second 38, second frame (where a second contains 24 frames)

    To me, this means we can not have both milliseconds and frames. because they are both units immediately smaller than second, example

    00:01:38.045 20/24

    The first part is easy: minute 1, second 38.
    But does it mean 045 milliseconds, or in the 20th frame, where a second contains 24 frames.

    So far, to me they seem exclusive, and both can not be used at the same time.

    Thank you beforehand for the clarification

  • edsfault

    edsfault February 14th, 2011 @ 07:19 PM

    To Brett, If you read Scott's post above, you'll see F/FR where F is the number of the frame you refer to, and FR refers to the frame rate.
    Given the frame number and the framerate, popcorn can do the math and identify where in the second you should attach an event.

  • Brett Gaylor

    Brett Gaylor February 14th, 2011 @ 10:19 PM

    Thanks, edsfault.

    However, if I'm grokking this correctly, this would require someone to type:
    , ie, specify which frame out of the given framerate they are on - really inconsistent with how people specify frames. Like this: 00:01:38 2/24

    The use case is this: someone with a quicktime file on their computer, which does NOT specify time in milliseconds, needing to be able to type that out (lets park for a second whether or not they should be using butter).

    So the idea was to provide a way for them to specify time as they normally would, rather than ask them to do a conversion for us each time they want to use popcorn.

  • David Humphrey

    David Humphrey February 14th, 2011 @ 10:26 PM

    The trouble we have here is that we can't index time to anything smaller than a ms value. The way framerates work in our (Firefox) implementation is this:

    • ogg - there is a playback rate in the file header, and we'll honour it.
    • WebM - each frame has a timecode, and frames get shown based on this time. There is a framerate, but it's just a hint.

    So whatever you do with fractional times into the ms values, it's just rounding.

  • Scott Downe

    Scott Downe February 15th, 2011 @ 10:03 AM

    Humph is right, framerate in Popcorn is just rounding, a hack in my opinion.

    My concern is making sure that milliseconds and framerates are no longer confused with each other.

    previously HH:MM:SS.DDD:FF was how we did it. With frames just being a rounding hack.

    What made no sense to the user was how we prioritized larger times. No colon meant it was SS, that made sense. Now, most people that saw one colon, thought MM:SS, but in actuality, it was SS:FF. I wanted to make this not so. That was my intent. To move the rounding hack off of the end of the timecode, and make it optional. From SS:FF on, MM:SS:FF and HH:MM:SS:FF made sense.

    In the end, whatever we do, it must be converted into SS.DDD, and me, I will always be using SS.DDD whenever possible.

    "So far, to me they seem exclusive, and both can not be used at the same time." They are exclusive, and using them together gives you unexpected results you can expect. You are explicitly telling Popcorn you want the nth frame out of n frames, then add on more milliseconds. Would someone want this? probably not, but why would someone even do this?

    Brett, correct me if I am wrong, but is HH:MM:SS:FF the standard way to do this in film?

  • edsfault

    edsfault February 15th, 2011 @ 11:00 AM

    So, should we require that the user inputs the framerate? Like in my example:
    00:01:38 4/24

    I think so far the alternatives I've read are:
    a) The user can specify milliseconds OR both frame number and framerate: 00:01:38 2/24

    b) The user can specify milliseconds OR frame number. The framerate will be obtained from the video file (ogg, or webM as Humph indicated.) The user would only need to input the frame number: 00:01:38 2

    c) We can have a different framerate xml attribute, which would allow our tag to look something like
    <..... in="00:01.38 2" framerate="24" ... >

    I think all of them make sense, although option A implies the rounding hack (turning frame number and framerate into milliseconds.)

  • Brett Gaylor

    Brett Gaylor February 15th, 2011 @ 02:06 PM

    Implementation aside, the goal here is that a user can enter:
    HH:MM:SS:FF

    Rounding is totally fine. If you are interested, there are all kind of
    crazy hacks and rounding things that video systems do in order to make these
    framerates. So, you won't be the first hackers to do that :)

  • annasob

    annasob March 14th, 2011 @ 04:10 PM

    • Milestone changed from 0.4 to 0.5

    edsfault whats the status of this?

  • edsfault

    edsfault March 15th, 2011 @ 08:03 AM

    Hello. I do have a working fix for this bug. The fix currently applies to the xml parser. But the fix for every other parser is literally the same. I have been trying to push my fix onto github, but it's the first push so It's been taking very long, and giving me a broken pipe error. I'm going to blame it on my internet connection. I'm going to Seneca this morning and trying it there once more. If that was to fail, I hope I can stop by cdot with the fix.

    On a more technical note, and as you will see in the bug fix, I implemented the following for the time stamp format:
    HH:MM:SS[.DDD | F/FR]
    MM:SS[.DDD | F/FR]
    SS[.DDD | F/FR]

    Where HH is hour, MM is minute, SS is second, DDD is millisecond, F is frame in the second, and FR is framerate.
    I hope the linux notation is clear, as in milliseconds and frame/framerate are both optional. They are also exclusive (one may either specify milliseconds or frame/framerate but not both.

    Examples:

    (03:23:12)     Hour 3, Minute 23, Second 12 (implictly millisecond 000)
    (12:03:45.345) Hour 12, Minute 03, Second 45, millisecond 345
    (04:00:2 3/16) Hour 4, Minute 00, Second 2, 3rd frame where each second contains 16 frames.
    

    I believe this should fix this bug particularly. However this implies the API is being broken, since this is not how time stamps are currently formatted.

    I include here the core of my fix (source code):

        // Simple function to convert the whole timestamp in seconds
        // Acceptable formats based on milliseconds are:
        //
        // HH:MM:SS[.DDD],
        // MM:SS[.DDD],
        // SS[.DDD],
        // In all of the previous cases, the milliseconds (.DDD) are
        // optional. Where milliseconds are no specified, milliseconds
        // will default to .000
        // 
        // Acceptable formats based on frames are:
        // HH:MM:SS[ F/FR],
        // MM:SS[ F/FR],
        // SS[ F/FR],
        // F represents the frame in the second, and FR represents the
        // framerate of the video (number of frames in each second).
        // F/FR is also optional
        var toSeconds = function(time) {
          var t = time.split(':'),
              lastIndex = t.length - 1,
              lastElement = t[lastIndex];
              
          //Fix last element: 
          if (lastElement.indexOf(' ') > -1) {
            var secondInfo = lastElement.split(' '),
                frameInfo = secondInfo[1].split('/');
            
            t[lastIndex] =
              parseInt(secondInfo[0], 10) +
              (parseInt(frameInfo[0], 10) / parseFloat(frameInfo[1], 10)) ;  
          }
          
          if (t.length === 1) {
            return parseFloat(t[0], 10);
          } else if (t.length === 2) {
            return (parseFloat(t[0] , 10) * 60) + parseFloat(t[1], 10);
          } else if (t.length === 3) {
            return (parseInt(t[0], 10) * 3600) + (parseInt(t[1], 10) * 60) + parseFloat(t[2], 10) ;
          }
        };
    
  • edsfault

    edsfault March 15th, 2011 @ 09:10 AM

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

    Ok, me again. It was indeed my internet connection giving me some problems when uploading (at 7 Kbps.)
    Here is my proposed fix for the time stamps. The fix is currently for the xml parser, but It does apply to every other parser:

    Commit:
    https://github.com/edsfault/Edsfault-popcorn-js/commit/f639feee053c...
    Tree:
    https://github.com/edsfault/Edsfault-popcorn-js/tree/f639feee053c95...

  • Rick

    Rick March 15th, 2011 @ 01:13 PM

    • State changed from “peer-review-requested” to “super-review-requested”
    • Assigned user changed from “edsfault” to “annasob”

    Tested in Chrome 9, 10, 11; Firefox 4b13, 4

    Tested with parserXML Test Suite

  • annasob

    annasob March 17th, 2011 @ 11:25 AM

    edsfault I am not clear on your approach. You added your stuff to the core but then you commented it out (line 218+ needs two tabs). And then you changed the XML parser's toSeconds(). Why not put toSeconds into popcorn.js and edit all of the parsers/plugins to use popcorn.js' toSeconds() to do the converting and remove the multiple toSeconds() functions inside the parsers/plugins?

  • edsfault

    edsfault March 18th, 2011 @ 07:11 AM

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

    I reviewed my fix. I have added a parser function to the core that parses the supported time format strings to seconds. I left the parser files untouched. I added a test for this parser function.
    This fix allows to use the format strings from code, but it does not work for parsers yet.

    commit: https://github.com/edsfault/Edsfault-popcorn-js/commit/69b03f63a052...
    tree: https://github.com/edsfault/Edsfault-popcorn-js/tree/69b03f63a0523b...

  • edsfault

    edsfault March 29th, 2011 @ 09:41 AM

    • no changes were found...
  • Scott Downe

    Scott Downe March 31st, 2011 @ 11:26 AM

    I think I figured out how to do this right.

    I wish I found this link or knew the name of the format earlier, but THIS http://en.wikipedia.org/wiki/SMPTE_time_code is what we want to support.

    "HH:MM:SS;FF" A semi-colon! that is all we were missing, and this is what I want.

    Can someone please either agree or disagree with this, and say why, so we can finally put this ticket to rest?

    It is so much easier to just follow a standard instead of inventing it ourselves.

  • edsfault

    edsfault March 31st, 2011 @ 12:44 PM

    • Assigned user changed from “annasob” to “edsfault”

    Hey Scott, I assume FF allows you to specify the frame in the second to attach an event to. The problem is we do not know at that point how many frames in a second we have.
    so we came up with:

    HH:MM:SS FF/FR

    If you think it's more fitting, we can substitute the space for a semi colon.

    HH:MM:SS;FF/FR

    In my most recent commit post I proposed a solution for it that uses the first format, but It should not be too difficult to change for for a semi colon.

    What do you think? deal? You may know of a better way to represent the framerate

  • Scott Downe

    Scott Downe March 31st, 2011 @ 01:30 PM

    I know next to nothing about framerates.

    all I know is it was confusing with regards to decimals, frames and seconds. Example:

    00:00:00
    00:00
    

    By looking at that, you would think it is:

    HH:MM:SS
    MM:SS
    

    or:

    MM:SS:FF
    SS:FF
    

    Note that "HH:MM:SS:FF" and "SS" wasn't a problem, and it was obvious what each number meant.

    I had many people fall into this hole, so I felt frames needed to be split by a unique modifier. A standard modifier would be best, space, semi colon, it didn't matter to me, just whatever was standard. I finally found out that semi-colon was standard for this.

    When I first suggested using

    <subtitle in="HH:MM:SS.DDD FF/rate"
    

    it was one of two ideas, the other being

    <subtitle in="00:00:10 01" frames="24"
    

    with the second one being better as you can specify the frames once in a parent element instead of on each track.

    This is even MORE ideal now that we have a standard to follow, and FF/FR will break that standard.

    I have to apologize for the confusion, bikeshedding, and inconsistency of the ticket, and commend you for your patience. This ticket got out of hand and way out of scope. If you go back to the first post, all it is asking for is "clear up whether 13:45 is SS:DD or MM:SS" and "standard ways of writing times, and we should use them" we just never got a standard that supported frames, and thus, things fell apart.

  • Scott Downe

    Scott Downe March 31st, 2011 @ 01:32 PM

    I am in favour of:

    <subtitle in="00:00:10;01" frames="24"
    

    because it solves the confusion of "SS:FF" vs "MM:SS", it follows and doesn't break the standard here: http://en.wikipedia.org/wiki/SMPTE_time_code

    While non-drop time code is displayed with colons separating the digit pairs—"HH:MM:SS:FF"—drop frame is usually represented with a semi-colon (;) or period (.) as the divider between all the digit pairs—"HH;MM;SS;FF", "HH.MM.SS.FF"—or just between the seconds and frames—"HH:MM:SS;FF" or "HH:MM:SS.FF". The period is usually used on VTRs and other devices that don't have the ability to display a semi-colon.
    

    Gives us the ability to supply the framrate because we have no other way to get it, and also has the advantage of allowing framerate to be supplied once in a parent element.

  • edsfault

    edsfault March 31st, 2011 @ 01:43 PM

    That's valid. The problem is that would only work for the xml parser. I believe every parser must comply though.
    My proposed solution centralizes all of these changes in the core. with the following code

                  // Simple function to convert the whole timestamp in seconds
                  // Acceptable formats based on milliseconds are:
                  //
                  // HH:MM:SS[.DDD],
                  // MM:SS[.DDD],
                  // SS[.DDD],
                  // In all of the previous cases, the milliseconds (.DDD) are
                  // optional. Where milliseconds are no specified, milliseconds
                  // will default to .000
                  // 
                  // Acceptable formats based on frames are:
                  // HH:MM:SS[ F/FR],
                  // MM:SS[ F/FR],
                  // SS[ F/FR],
                  // F represents the frame in the second, and FR represents the
                  // framerate of the video (number of frames in each second).
                  // F/FR is also optional
                  var toSeconds = function(time) {
                    if (typeof(time) == 'number') {
                      return time;
                    }
                    else if (typeof(time) == 'string')
                    {
                      //Hours and minutes are optional
                      //Seconds must be specified
                      //Seconds can be followed by milliseconds OR by the frame information
                      var validTimeFormat = /^([0-9]+:){0,2}[0-9]+(\.[0-9]+| [0-9]+\/[0-9]+){0,1}$/
    
                      if (!validTimeFormat.test(time))
                      {
                        throw 'Invalid time format' ;
                      }
                    }
                    else
                    {
                      throw 'Invalid time format' ;
                    }
    
                    var t = time.split(':'),
                        lastIndex = t.length - 1,
                        lastElement = t[lastIndex];
                      
                    //Fix last element: 
                    if (lastElement.indexOf(' ') > -1 ) {
                      var secondInfo = lastElement.split(' '),
                          frameInfo = secondInfo[1].split('/');
                    
                      t[lastIndex] =
                        parseInt(secondInfo[0], 10) +
                        (parseInt(frameInfo[0], 10) / parseFloat(frameInfo[1], 10)) ;  
                    }
                  
                    if (t.length === 1) {
                      return parseFloat(t[0], 10);
                    } else if (t.length === 2) {
                      return (parseFloat(t[0] , 10) * 60) + parseFloat(t[1], 10);
                    } else if (t.length === 3) {
                      return (parseInt(t[0], 10) * 3600) + (parseInt(t[1], 10) * 60) + parseFloat(t[2], 10) ;
                    }
                  };
    

    Two examples of valid time strings would be

    1) 01:34:21.561 Hour 1, Minute 34, Second 21, Millisecond 561
    2) 02:12:07 3/24 Hour 2, Minute 12, Third frame of second 7. Where a second contains 24 Frames.

    The commit I posted includes tests and samples. You may want to take a look at those.
    Currently, with my proposed solution you can specify times as follows:

          Popcorn('#video').subtitle({
            start: "1 12/24",
            end: "15 17/24",
            text: 'this is a subtitle'
          } ) ;
    

    I beleive your idea is valuable, in terms that we could do the following

          Popcorn('#video').subtitle({
            start: "1;12", //Second 1, frame 12
            end: "15;17", //Second 15, frame 17
            framerate: "24" text: 'this is a subtitle'
          } ) ;
    

    This way, we would not have to repeat the framerate.
    By the way, with my proposed solution there would be NO CONFUSION. Seconds and milliseconds are separated by a decimal point. We could then indeed separate seconds and frame with a colon.

  • edsfault

    edsfault March 31st, 2011 @ 02:14 PM

    This is my proposal for this bug.

    Time strings may follow the following format: [HH]:[MM]:SS[;FF | .MMM[/FR]]

    HH is hour
    MM is minute
    SS is second
    FF is frame
    MMM is millisecond
    FR is frame rate

    Considerations:
    Hour (HH) is optional. With the default as 0.
    Minute (MM) is optional. With the default as 0.
    Second is manditory.
    After second we may encounter a decimal point followed by milliseconds OR
    after second we may encounter a semicolon followed by a frame.
    If frame is encountered, it may be followed by a slash (/) followed by the framerate
    If a frame is encountered and a framerate is not encountered, another attribute named framerate must contain the framerate value.

    Examples:
    With milliseconds
    01.34
    34:21.451
    02:13:34.301

    With frame and framerate
    01;4/24
    34:21;17/36
    02:13:34;7/41

    With frame without framerate:
    02:13:34;17

    Where the actual plugin use looks like:

    Popcorn('#video').subtitle({
      start: "01:57:55;9",
      end: "02:13:34;17",
      framerate: "24",
      text: 'this is a subtitle'
    } ) ;
    

    A regular expression that might satisfy a timestamp is: (If you wish to study it)
    /^([0-9]+:){0,2}[0-9]+(.[0-9]+|;[0-9]+(\/[0-9]+)?)?$/ ^ Beginning of string ([0-9]+:){0,2} : Hour and minute are optional [0-9]+ : Second is manditory .[0-9]+ : Milliseconds OR ;[0-9]+ : Frame number OPTIONALLY FOLLOWED BY (\/[0-9]+)? : Framerate $End of string

  • Brett Gaylor

    Brett Gaylor March 31st, 2011 @ 02:19 PM

    Looks good. Can we get the framerate from the videos metadata? Then we don't have to have authors include it...

  • Scott Downe

    Scott Downe March 31st, 2011 @ 03:40 PM

    Brett, unfortunately no, we cannot get the framerate, as it does not exist.

  • Scott Downe

    Scott Downe March 31st, 2011 @ 03:47 PM

    I am not totally against the idea of putting time codes into core, as I personally found the need for it this weekend.

    I do however, think framerate should be something that is defined once per video, and not once per track and yet still be definable from a parsers point of view. Maybe passed into the constructor?

  • edsfault

    edsfault March 31st, 2011 @ 03:54 PM

    I agree wit Scott. Frame rate should be defined one per video.

    Would it be ok if it looked like this?

    var p = Popcorn('video')
            .setFrameRate ( 24 )
            .subtitle({
               start: "01:57:55;9",
               end: "02:13:34;17",
               text: 'this is a subtitle'
            }) ;
    

    If time stamps specify frames and no framerate was specified, a default framerate could be used, or the frame could just be ignored.

  • Scott Downe

    Scott Downe March 31st, 2011 @ 04:08 PM

    Yeah, I am liking this.

    Only small nit pick I have against making a setter function for framerate as it seems weird to be able to change the framerate at any time. Where as a constructor param, or even better an options object as a constructor param, so we can add other things to later. Something like this:

    var p = Popcorn('video', { framerate: 24 } )
            .subtitle({
               start: "01:57:55;9",
               end: "02:13:34;17",
               text: 'this is a subtitle'
            }) ;
    

    optional, of course.

  • edsfault

    edsfault March 31st, 2011 @ 06:28 PM

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

    I believe I have implemented the time formats as agreed.

    commit: https://github.com/edsfault/Edsfault-popcorn-js/commit/979e0ec1af36...
    tree: https://github.com/edsfault/Edsfault-popcorn-js/tree/979e0ec1af3659...

    Among the changes:

    Added a framerate optional parameter to the constructor:
    Popcorn( 'video' , { framerate: 24 } ).....

    And added support for time strings, such as: "03:38:13.567" and "04:42:20;12"
    I also added some tests, where you can actually see the functionality added being used.

  • edsfault

    edsfault March 31st, 2011 @ 06:28 PM

    • State changed from “assigned” to “peer-review-requested”
  • Brett Gaylor

    Brett Gaylor March 31st, 2011 @ 07:17 PM

    Really? The file headers don't have the frame rate? Sorry to be annoying on this one, just seems like something one would assume.

  • Rick

    Rick March 31st, 2011 @ 07:21 PM

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

    Rick March 31st, 2011 @ 07:28 PM

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

    Please review the style guide: https://webmademovies.lighthouseapp.com/projects/63272/styleguide and bring your changes into parity - thank you!

    Additional notes:

    1. Within the constructor, properties on this should be used as a root to complex object data; if you're adding a new options object to argument list, it makes more sense to extend options onto this instead of singling it out. Change Lines 139-142 to:
    
    this.options = options || {};
    

    .. Additionally, it should probably go after this.data is defined

    1. toSeconds should be defined as a static function on a static Popcorn property (not to be confused with Popcorn.prototype or Popcorn.p) Something like this...
    
    Popcorn.util = {
      toSeconds: function( popcornObject, time ) {
        // make the time 
        // using: popcornObject.options.framerate; time
      }
    };
    

    In it's current implementation, its actually being redefined everytime the timeupdate event is fired, so it has to move. The Popcorn.util object makes sense to placed at the end of the lib, just prior to the exposure line, so right before "global.Popcorn = Popcorn;"

    1. "'Invalid time format'" is used twice so should be stored in a var

    2. no parens with "typeof foo"

    3. "ii" should be defined with the rest of the vars in the timeupdate listener and should be more descriptive. "smpteIdx" makes sense

  • edsfault

    edsfault March 31st, 2011 @ 10:15 PM

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

    I believe I have fixed the things you've mentioned.

    Thank you very much for the feedback. The best I've gotten after a review.

    commit: https://github.com/edsfault/Edsfault-popcorn-js/commit/48f41c115765...
    tree: https://github.com/edsfault/Edsfault-popcorn-js/tree/48f41c1157659f...

  • Rick

    Rick April 1st, 2011 @ 09:57 AM

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

    This looks fantastic - thank you for being so receptive to my review.

    A few things...

    1. I like the approach you took to the second argument of Popcorn.util.toSeconds( time, framerate )... much better then my suggestion.

    2. Update your single quotes to double quotes ;)

    3. Use Popcorn.error( message ) instead of "throw errorMessage;"

    4. I would move this...

    
    var validTimeFormat = /^([0-9]+:){0,2}[0-9]+([.;][0-9]+)?$/,
        errorMessage = "Invalid time format";
    

    To the very beginning of the Popcorn.util.toSeconds() definition

    ... All your tests need to be moved into /test/popcorn.unit.js - if you need help converting, dont hesitate to ping me on IRC (or anyone else in the popcorn channel!)

    Really Awesome work!!!

  • David Humphrey

    David Humphrey April 1st, 2011 @ 11:14 AM

    Ed, how are you doing with review fixes? Can we get this in for 0.5?

  • edsfault

    edsfault April 1st, 2011 @ 01:37 PM

    I strongly believe so. The review feedback I've gotten has been very constructive so far. It should be done by the end of the day.

  • edsfault

    edsfault April 1st, 2011 @ 02:54 PM

    @Rick.

    What do you mean convert? I really dont get it. I hope you can give me an example when you can. Meanwhile I'll just IRC. I'm sure they'll help me there. I'm working on it

  • Rick

    Rick April 1st, 2011 @ 04:54 PM

    I mean, rewrite them so they are in the same file as the rest of the unit tests and are written as QUnit assertions.

  • edsfault

    edsfault April 1st, 2011 @ 10:48 PM

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

    I hope I did it right this time.
    I wrote some tests to make sure my function converts time strings to seconds correctly, and they are part of the unit test.

    commit: https://github.com/edsfault/Edsfault-popcorn-js/commit/ad1c009a09ad...
    tree: https://github.com/edsfault/Edsfault-popcorn-js/tree/ad1c009a09ad29...
    parent: https://github.com/edsfault/Edsfault-popcorn-js/tree/d060d5a773b12c...

  • Scott Downe

    Scott Downe April 4th, 2011 @ 01:57 PM

    • State changed from “peer-review-requested” to “under-review”
    • Assigned user changed from “edsfault” to “Scott Downe”
  • Scott Downe

    Scott Downe April 4th, 2011 @ 02:00 PM

    edsfault, for next time, none of those links above show the branch your work is on, just the work itself. All you truly need for a review is: https://github.com/edsfault/Edsfault-popcorn-js/tree/ticket118 Now I know all your work is in branch "ticket118" which also helps when I try to pull your work.

  • edsfault

    edsfault April 4th, 2011 @ 02:05 PM

    Ohhh, Im so sorry about that. I had no idea, and I now I do, so it won't happen again. I thought the commit info was enough. Thanks for letting me know

    All my work is in the following branch: https://github.com/edsfault/Edsfault-popcorn-js/commits/bug118

    I only did one commit (If I remember correctly)

  • Rick

    Rick April 4th, 2011 @ 02:22 PM

    edsfault, nice work!

    A few last bits:

    On https://github.com/edsfault/Edsfault-popcorn-js/commit/ad1c009a09ad... "==" should be "==="

    https://github.com/edsfault/Edsfault-popcorn-js/commit/ad1c009a09ad... "else" on the same line as the closing curly brace, like: "} else if {"

    https://github.com/edsfault/Edsfault-popcorn-js/commit/ad1c009a09ad... "==" should be "==="

    https://github.com/edsfault/Edsfault-popcorn-js/commit/ad1c009a09ad... "else" on the same line as the closing curly brace, like: "} else {"

  • Scott Downe

    Scott Downe April 4th, 2011 @ 04:01 PM

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

    I see many uses of "==" and "!=". you should change them to be "===" and "!==".

    Line 89 of popcorn.unit.js:

    } ,

    should be

    },

    this occurs on other lines as well.

    Line 164 of popcorn.unit.js:

    for ( var periodsIdx = 0; periodsIdx < timePeriods.length; periodsIdx++ ) {
    

    would be faster as. It is a good habit to always write your for loops like this in JavaScript.

    for ( var periodsIdx = 0, timPeriodsLength = timePeriods.length; periodsIdx < timPeriodsLength; periodsIdx++ ) {
    

    Lines in your JSON object look like this:

    start : '1:48:27;9',
    

    should be like this to stay consistent with the rest of the library:

    start: '1:48:27;9',
    

    You also mix single and double quotes in JSON lines. Should stick to what is currently being used in the rest of the file, unless otherwise needed, or you are specifically testing cases with the other quotes?

    You horizontal spaces are good minus the JSON lines, but the vertical spacing is off. Generally, something like this:

    
    var fn = function () {
    
      for () {
    
        console.log();
      }
    
      if () {
        
        console.log();
      }
    };
    

    notice the vertical space above and below a function definition, and a empty line before a logical block (if, for, while, etc). I am still perfecting this, though, but I have found the above to be working for me. Refer to this: https://webmademovies.lighthouseapp.com/projects/63272/styleguide if you have any doubts or questions, or ask me.

    So quite a few nit picks here, individually they are trivial, but they add up. Mostly related to style, your functionality is good.

  • Scott Downe

    Scott Downe April 4th, 2011 @ 04:20 PM

    • Assigned user changed from “Scott Downe” to “edsfault”
  • edsfault

    edsfault April 4th, 2011 @ 09:58 PM

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

    Hello there. I did my best to fix the errors pointed out. Thank you very much for the very fast and accurate feedback.

    About the quotes, yes. I was trying to make sure It worked with any kind of quotes. I now commented it so it doesn't look like plain inconsistency.

    Branch : https://github.com/edsfault/Edsfault-popcorn-js/tree/ticket118
    Commit: https://github.com/edsfault/Edsfault-popcorn-js/commit/7015a2034402...
    Tree: https://github.com/edsfault/Edsfault-popcorn-js/tree/7015a20344029a...
    Parent: https://github.com/edsfault/Edsfault-popcorn-js/tree/4c7482d77eea6f...

    I hope that with every R- I get closer to an acceptable solution.

  • Scott Downe

    Scott Downe April 5th, 2011 @ 12:32 PM

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

    OK, this is fine with me.

  • annasob

    annasob April 7th, 2011 @ 01:28 PM

    • Assigned user changed from “edsfault” to “David Humphrey”
  • Rick

    Rick April 14th, 2011 @ 12:14 PM

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

    Awesome work , way to ride out the reviews and come out shining! The tests look great as well!

    Tested with Core, Plugins, Parsers and Players Test Suites and passing 100% in:

    Chrome 9, 10, 11

    Firefox 3.0.12, 3.6.16, 4.0

  • Rick

    Rick April 14th, 2011 @ 12:16 PM

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

    Sorry, meant to change to "review looks good"

  • annasob

    annasob April 14th, 2011 @ 02:37 PM

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

    Staged in annasob/popcorn-js commit

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

Tags

Referenced by

Pages