3
\$\begingroup\$

With HTML5 you can add captions to your video using the <track /> element. However, only vtt files are officialy supported, while the current most populair subtitle format is the well known srt format.

So instead of converting all the srt files to vtt files, I had the idea of writing a script which will do all of this on the fly. Without any manual converting.

Using the script you can add srt files to the <track /> element like so:

<track label="English" kind="subtitles" srclang="en" src="https://s3-us-west-2.amazonaws.com/s.cdpn.io/1178475/Fantastic.Beasts.The.Crimes.of.Grindelwald.2018.1080p.WEB-DL.H264.AC3-EVO.srt" default>

What the script basically does is following

  • Gets the content from the file using a XMLHttpRequest
  • Converts the srt format to a vtt format
  • Creates a blob from the vtt string
  • Creates a file from the blob
  • Replaces the original src with the generated vtt file

The script itself is very simple and small, only 4kb in size. I wonder if I can make the script even smaller and if I wrote the code well.

document.addEventListener("DOMContentLoaded", function () {
    /**
     * Get all videos
     */
    var videoElements = document.getElementsByTagName('video');

    /**
     * This function converts all srt's to vtt files
     */
    function convertSrtToVtt() 
    {
        /**
         * Generate an unique identifier
         */
        this.id = '_' + Math.random().toString(36).substr(2, 9);

        /**
         * All tracks assigned to current video element
         */
        var tracks = document.querySelectorAll("#" + this.id + " track");

        var subtitle = {
            data: 
            {
                track: {}
            },
            /**
             * Load the file from url
             *
             * @param {object}    track   - DOM <track /> object
             */
            load: function(track) 
            {
                subtitle.track = track;

                if(subtitle.isSrt(subtitle.track.src)) 
                {
                    var client = new XMLHttpRequest();
                    client.open('GET', subtitle.track.src);
                    client.onreadystatechange = function() 
                    {
                        subtitle.convert(client.responseText).then(
                            function (file) 
                            {
                                /**
                                 * Replace the srt file with the generated vtt file
                                 */
                                subtitle.track.src = file   
                            }
                        );
                    }
                    client.send();
                }
            },
            /**
             * Converts the SRT string to a VTT formatted string
             *
             * @param   {string}    content     - SRT string
             * @return  {object}    promise     - Returns a promise with the generated file as the return value
             */
            convert: function(content) 
            {
                var promise = new Promise( 
                    function (resolve, reject) 
                    {
                        /**
                         * Replace all (,) commas with (.) dots. Eg: 00:00:01,144 -> 00:00:01.144
                         */
                        content = content.replace(/(\d+:\d+:\d+)+,(\d+)/g, '$1.$2');
                        content = "WEBVTT - Generated using SRT2VTT\r\n\r\n" + content;

                        /**
                         * Convert content to a file
                         */
                        var blob = new Blob([content], {type: 'text/vtt'});
                        var file = window.URL.createObjectURL(blob);

                        resolve(file);
                    }
                );

                return promise;
            },
            isSrt: function(filename) 
            {
                return filename.split('.').pop().toLowerCase() === 'srt' ? true : false;
            },
            isVTT: function(filename) 
            {
                return filename.split('.').pop().toLowerCase() === 'vtt' ? true : false;
            }
        }

        for(var i = 0;i < tracks.length;i++) 
        {
            subtitle.load(tracks[i]);
        }
    }

    for(var i = 0;i < videoElements.length;i++) 
    {
        videoElements[i].addEventListener('loadstart', convertSrtToVtt);
    }
});

Working demo on CodePen
Github repository

\$\endgroup\$
3
\$\begingroup\$

Bloated

You have 104 lines of code, that can easily by made more readable with some simple redunctions. Remove comments, same line open blocks {, and remove empty lines. That reduces the code to 54 lines.

Review

I can see you are somewhat new to JS

Some JS items

  • Use const for variables that do not change.
  • In JS we put the open block delimiter { on the same line.
  • When using vars you should hoist them to the top of the function they are scoped to.
  • Use arrow functions for anonymous functions.
  • window is the default object, you don't need to use it. You don't use it for Blob eg new window.Blob( but next line down you use it for URL Why?
  • Use shortest form. eg for(var i = 0;i < tracks.length;i++) { the index is not needed so for(const track of tracks) or tracks.forEach(subtitle.load) would be better;.
  • Don't use substr it is on the great list of the depreciated. Use substring or slice
  • Use function shorthand when declaring object function. Eg load: function(track) { should be load(track) {

Code style and logic

  • comment are way over the top and just repeat what is self evident in the code.
  • Space between if and (
  • You don't use the second argument of the promise callback, so why define it?
  • Don't use generic names for variables and arguments. new Promise( function (resolve, reject) { has no semantic meaning, something like new Promise(converted => { would be more apt.
  • Don't add redundant code return filename.split('.').pop().toLowerCase() === 'srt' ? true : false; is identical to return filename.split('.').pop().toLowerCase() === 'srt';
  • Why add the two functions isSrt and isVTT to the object. They are generic and can be defined once outside the object (BTW should it be isSrt or isSRT I don't know what it stands for, but if an acronym then it should be uppercase?)
  • The random id can produce a string shorter than 7 characters. eg (1/36).toString(36).substr(2, 9) would create "1" not "1000000"
  • I do not see the point of creating an random id (There is a definite chance of a clash) when you already have the element you want to query.

Structure

  • Why create an object subtitle when its not needed?
  • Why is subtitle: { data: { track: {}}... created. I can find no reference to it.
  • You could have used the Fetch API rather than XMLHttpRequest

Alternative example.

A lot of bloat can be removed and the process can thus be viewed in one page, this improves maintainability, readability, and reduces the chance of bugs.

document.addEventListener("DOMContentLoaded", function () {
    const isSrt = name => name.split(".").pop().toLowerCase() === "srt";  
    const convert = content => new Promise(converted => {
        content = content.replace(/(\d+:\d+:\d+)+,(\d+)/g, "$1.$2");
        content = "WEBVTT - Generated using SRT2VTT\r\n\r\n" + content;
        converted(URL.createObjectURL(new Blob([content], {type: "text/vtt"})));
    });    
    for (const vid of document.getElementsByTagName("video"))  {
        vid.addEventListener("loadstart", event => {
            const tracks = [...event.target.querySelectorAll("track")];
            tracks.forEach(track => {
                if (isSrt(track.src))  {
                    const client = new XMLHttpRequest();
                    client.open("GET", track.src);
                    client.onreadystatechange = () => {
                        convert(client.responseText).then(file => track.src = file);
                    };
                    client.send();
                }
            });
        }
    }
});

Update

To support legacy browsers you should use something like Babel. Please note this will not cover media textTrack support, only javascript

\$\endgroup\$
  • \$\begingroup\$ Wow, thanks for this realy helpfull review. This review realy helps me understand what I could do better. A few notes to some points you made, but I do understand them. 1) Bloated - I added the comments on purpose so others can see whats going on, I included a minified file which doenst contain all this bloat. 2) In JS we put the open block delimiter { on the same line. - Ah ok, I'm used to it using PHP. So I put them on new lines. Is this realy a bad practice? In production we use minified files anyway? \$\endgroup\$ – Red Feb 21 at 22:11
  • \$\begingroup\$ I agree with a lot of this advice, but I don't think that it's fair to assume ES6 support, when the original code does not. \$\endgroup\$ – 200_success Feb 21 at 22:17
  • \$\begingroup\$ @Red The reduction of code size is not for transport (its all zipped anyways) Each line of code increase the chance of a bug, needs additional cognitive effort (we don't read individual characters, but line break force us to do that), can force interruption of thought when you need to scroll. Comments... Others are all coders and do not need comments that reiterate what is self evident in the code, the comment get in the way of readability.. The new line { in PHP and C/C++/C# (and like) is something these languages need to get over. \$\endgroup\$ – Blindman67 Feb 21 at 22:20
  • \$\begingroup\$ @200_success he uses Promise in the original. The original code will only run on ES6+ \$\endgroup\$ – Blindman67 Feb 21 at 22:23
  • \$\begingroup\$ Just as an example, Promises are supported in Safari ≥ 7.1 (released Sep 2014), but arrow functions require Safari ≥ 10 (released Sep 2016). That's a two-year compatibility gap you have introduced. \$\endgroup\$ – 200_success Feb 21 at 22:34
2
\$\begingroup\$

Clobbering the id attribute of each <video> element is unexpected and unfair. That could break the page (such as any CSS rules that rely on the original id).

Your huge convertSrtToVtt() definition is placed between the var videoElements definition and the loop that uses those videoElements. That's a huge unnecessary annoyance for readability.

Personally, I find the

/**
 * Multi-line comments
 */

to be a hindrance to readability, especially when the comments apply to single statements.

The isVTT function is never used, and should be dropped. (Note that its capitalization is inconsistent with isSrt and convertSrtToVtt.)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.