#290 ✓ checked-in
Steven W

Add parser support for SRT

Reported by Steven W | January 25th, 2011 @ 02:13 PM | in 0.3 release date

Add data-timeline-sources support to process and parse subtitles from an SRT file

Comments and changes to this ticket

  • Steven W

    Steven W January 25th, 2011 @ 05:18 PM

    Parser done, minor issues with unit tests.

    If needed in mean time: https://github.com/stevenaw/popcorn-js/tree/290

  • Steven W

    Steven W January 25th, 2011 @ 05:36 PM

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

    Scott Downe January 26th, 2011 @ 10:12 AM

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

    alert("looks good to me");

  • annasob

    annasob January 27th, 2011 @ 05:34 PM

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

    Same comments as #289

  • Steven W

    Steven W January 28th, 2011 @ 11:25 PM

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

    Adding documentation

  • Simon Jung

    Simon Jung January 29th, 2011 @ 10:19 AM

    How are we going to support multiple lines of dialogue?
    Right now, they are just added together with a single space.

    So instead of:

    Oh my god, Watch out!
    It's coming!!

    It looks like:

    Oh my god, Watch out! It's Coming!!

    Also, depending on the length and font-size the long sentence could cut anywhere (something I doubt was intended by the creator of the subtitles):

    Oh my god, Watch out! It's
    Coming!!

  • Steven W

    Steven W January 29th, 2011 @ 12:00 PM

    Good find, it looks like the majority of players use a line break too. That'll have to be changed.

    It's been tough finding "official" spec information, but these are about as close as my internet searching will yield:
    http://srt-subtitles.com/
    http://ale5000.altervista.org/subtitles.htm

    Regarding HTML tags, we could let the browser handle everything so as not to worry about checking opening/closing tags and filtering only the expected SRT tag subset (<b>, <i>, <u>, <font>, and <s>). This goes along with not doing explicit style setting like our other subtitle tickets (and identical behaviour to ignoring display coordinates like in #291).

  • Steven W

    Steven W February 1st, 2011 @ 12:25 PM

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

    Documentation added.
    It goes beyond the SRT spec but I've mimicked the inline style support of many players out there: HTML tags left in, SSA tags are stripped out. Explicit SSA line breaks (\N) and implicit line breaks (like Simon mentioned) are left in.
    More unit tests added to reflect these changes

    Branch: https://github.com/stevenaw/popcorn-js/tree/290

  • annasob

    annasob February 1st, 2011 @ 07:13 PM

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

    Hey Steven can you update your branch with my 0.3. I am getting some odd conflicts like:
    CONFLICT (delete/modify): parsers/parserSRT/data/unit.srt deleted in HEAD and mo
    dified in steven/290. Version steven/290 of parsers/parserSRT/data/unit.srt left
    in tree.

    I don't even have this parserSRT directory yet. Not sure what git is going. Also set back to SR because Scott PR this already.

  • Steven W

    Steven W February 1st, 2011 @ 09:29 PM

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

    The conflicts should be sorted out. We should be hitting just about every checkpoint in the comparison of cross-player SRT support.

    Branch: https://github.com/stevenaw/popcorn-js/tree/290

  • Rick

    Rick February 2nd, 2011 @ 04:13 PM

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

    Line 71:

    
    for(var i=0, l=lines.length; i<l;i++) {
    

    i has already been declared at the top and l is being leaked globally.

    Please review the style guide: https://gist.github.com/793649

  • Steven W

    Steven W February 3rd, 2011 @ 12:49 AM

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

    Thanks for the style guide info. Source updated and leaked variable eliminated.
    Branch: https://github.com/stevenaw/popcorn-js/tree/290

  • Rick

    Rick February 3rd, 2011 @ 10:27 AM

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

    When I pulled your 290 branch, I got updates for files that are unrelated to this ticket...

    $ git pull stevenaw 290
    From https://github.com/stevenaw/popcorn-js
     * branch            290        -> FETCH_HEAD
    Updating 02481e6..4de4383
    Fast-forward
     .gitignore                                     |    5 +-
     parsers/parserSRT/data/data.srt                |   16 +++
     parsers/parserSRT/data/unit.srt                |   40 ++++++++
     parsers/parserSRT/popcorn.parserSRT.html       |   56 +++++++++++
     parsers/parserSRT/popcorn.parserSRT.js         |  119 ++++++++++++++++++++++++
     parsers/parserSRT/popcorn.parserSRT.unit.html  |   44 +++++++++
     parsers/parserSRT/popcorn.parserSRT.unit.js    |  108 +++++++++++++++++++++
     parsers/parserTTXT/data/data.TTXT              |   25 +++++
     parsers/parserTTXT/data/unit.TTXT              |   25 +++++
     parsers/parserTTXT/popcorn.parseTTXT.html      |   46 +++++++++
     parsers/parserTTXT/popcorn.parseTTXT.js        |   73 +++++++++++++++
     parsers/parserTTXT/popcorn.parseTTXT.unit.html |   44 +++++++++
     parsers/parserTTXT/popcorn.parseTTXT.unit.js   |   61 ++++++++++++
     plugins/googlemap/popcorn.googlemap.js         |    2 +
     plugins/lastfm/popcorn.lastfm.html             |   25 ++++-
     plugins/lastfm/popcorn.lastfm.js               |   16 ++--
     plugins/lastfm/popcorn.lastfm.unit.js          |   14 ++-
     plugins/wikipedia/popcorn.wikipedia.html       |    1 +
     plugins/wikipedia/popcorn.wikipedia.js         |   40 ++++----
     plugins/wikipedia/popcorn.wikipedia.unit.js    |    5 +-
     20 files changed, 727 insertions(+), 38 deletions(-)
     mode change 100644 => 100755 .gitignore
     create mode 100644 parsers/parserSRT/data/data.srt
     create mode 100644 parsers/parserSRT/data/unit.srt
     create mode 100644 parsers/parserSRT/popcorn.parserSRT.html
     create mode 100644 parsers/parserSRT/popcorn.parserSRT.js
     create mode 100644 parsers/parserSRT/popcorn.parserSRT.unit.html
     create mode 100644 parsers/parserSRT/popcorn.parserSRT.unit.js
     create mode 100644 parsers/parserTTXT/data/data.TTXT
     create mode 100644 parsers/parserTTXT/data/unit.TTXT
     create mode 100644 parsers/parserTTXT/popcorn.parseTTXT.html
     create mode 100644 parsers/parserTTXT/popcorn.parseTTXT.js
     create mode 100644 parsers/parserTTXT/popcorn.parseTTXT.unit.html
     create mode 100644 parsers/parserTTXT/popcorn.parseTTXT.unit.js
    

    I'm going to review the files for the ticket, but we need to make sure these don't get pulled into the 0.3 branch like this.

  • Rick

    Rick February 3rd, 2011 @ 10:44 AM

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

    Reviewed - everything looks good, just update with the changes we discussed in IRC.

    @annasob - this needs some GIT love from you to work out the merging issue

  • Rick

    Rick February 3rd, 2011 @ 10:47 AM

    I just realized my own repo might have been out of sync causing me to see all those get updated.

    Anna - can you confirm that his branch is up-to-date - thanks :D

  • annasob

    annasob February 3rd, 2011 @ 02:40 PM

    I am getting:

    
    Fast forward
     parsers/parserSRT/data/data.srt               |   16 ++++
     parsers/parserSRT/data/unit.srt               |   40 ++++++++
     parsers/parserSRT/popcorn.parserSRT.html      |   56 ++++++++++++
     parsers/parserSRT/popcorn.parserSRT.js        |  119 +++++++++++++++++++++++++
     parsers/parserSRT/popcorn.parserSRT.unit.html |   44 +++++++++
     parsers/parserSRT/popcorn.parserSRT.unit.js   |  108 ++++++++++++++++++++++
     6 files changed, 383 insertions(+), 0 deletions(-)
     create mode 100644 parsers/parserSRT/data/data.srt
     create mode 100644 parsers/parserSRT/data/unit.srt
     create mode 100644 parsers/parserSRT/popcorn.parserSRT.html
     create mode 100644 parsers/parserSRT/popcorn.parserSRT.js
     create mode 100644 parsers/parserSRT/popcorn.parserSRT.unit.html
     create mode 100644 parsers/parserSRT/popcorn.parserSRT.unit.js
    
    So far so good
  • annasob
  • annasob

    annasob February 3rd, 2011 @ 05:38 PM

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

    Ok I had Steven redo the branch because there were some weird things happening. I staged this but you may want to take a look Rick

    Staged in annasob/popcorn-js commit

  • Rick

    Rick February 3rd, 2011 @ 05:45 PM

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

    looks good

  • annasob

    annasob February 3rd, 2011 @ 05:50 PM

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

    Great setting back to staged

  • annasob

    annasob February 11th, 2011 @ 03:55 PM

    • State changed from “staged” to “checked-in”

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