Display text on video screen

The code displays a video that will play a video and then display some text on the screen at specific locations on the video screen.

The code works but I would like to see if I could change somethings to make it better and how I would do that. You should be able to just copy the code into a web browser and it should work fine.

```

window.addEventListener('load', eventWindowLoaded, false);
var videoElement;
var videoDiv;

function eventWindowLoaded() {
videoElement = document.createElement("video");
videoDiv = document.createElement('div');

document.body.appendChild(videoDiv);
videoDiv.appendChild(videoElement);
videoDiv.setAttribute("style", "display:none;");
var videoType = supportedVideoFormat(videoElement);

videoElement.addEventListener("canplaythrough",videoLoaded,false);
videoElement.setAttribute("src", "muirbeach." + videoType);
}

function supportedVideoFormat(video) {
var returnExtension = "";
if (video.canPlayType("video/webm") =="probably" || video.canPlayType("video/webm") == "maybe") {
returnExtension = "webm";
} else if(video.canPlayType("video/mp4") == "probably" || video.canPlayType("video/mp4") == "maybe") {
returnExtension = "mp4";
} else if(video.canPlayType("video/ogg") =="probably" || video.canPlayType("video/ogg") == "maybe") {
returnExtension = "ogg";
}
return returnExtension;
}

function canvasSupport () {
return Modernizr.canvas;
}

function videoLoaded() {
canvasApp();
}

function canvasApp() {
function drawScreen () {
//Background and redraw background screen
context.fillStyle = '#ffffaa';
context.fillRect(0,0,theCanvas.width,theCanvas.height);
context.strokeStyle = '#000000';
context.strokeRect(5,5,theCanvas.width-10,theCanvas.height-10);

context.drawImage(videoElement,85,30);

context.fillStyle = "#000000";
context.font = "10px sans";
context.fillText ("Dur

Interesting code,

you are skirting the rules of 'code you wrote', but I still think review of this code is valuable. In essence this code is fine, there are some style changes you might want to consider.

• I am guessing part of your goal is to have a "one page has it all" HTML page, but you should really have that JavaScript in separate script file

• You create a video and a div tag with JavaScript, I would have these elements already in the HTML

• You are not using canvasSupport anywhere

-
videoLoaded is a one liner that calls another functions without parameters, I would kill that function and do

videoElement.addEventListener("canplaythrough",canvasApp,false);

-
It is considered better to have one single comma separated var statement:

var videoElement;
    videoDiv;

-
I am a fan of early returns, I would write supportedVideoFormat like this:

function supportedVideoFormat(video) {
    if (video.canPlayType("video/webm") =="probably" || video.canPlayType("video/webm") == "maybe") {
        return "webm";
    }
    if(video.canPlayType("video/mp4") == "probably" || video.canPlayType("video/mp4") == "maybe") {
        return "mp4";
    }
    if(video.canPlayType("video/ogg") =="probably" || video.canPlayType("video/ogg") == "maybe") {
        return "ogg";
    } 
    return "";
}

But probably I would then get irritated by the repeated calls for the same property, so I would probably shortcut those, it would also help with the horizontal stretching:

function supportedVideoFormatExtesion(video) {
   var supportsWebM = video.canPlayType("video/webm"),
       supportsMP4  = video.canPlayType("video/mp4"),
       supportsOgg  = video.canPlayType("video/ogg");
    if (  supportsWebM =="probably" || supportsWebM == "maybe") {
        return "Webm";
    }
    if( supportsMP4 == "probably" || supportsMP4 == "maybe") {
        return "mp4";
    }
    if( supportsOgg =="probably" || supportsOgg == "maybe") {
        return "ogg";
    } 
    return "";
}

This would irritate me as well, at this point it is blindingly obvious that you are trying to map a video format string to an extension string.. The above code is not the best approach to do that..

function supportedVideoFormatExtesion(video) {
   var formats    = ['video/webm','video/mp4','video/ogg'],
       extensions = ['Webm','mp4','ogg'],
       support;
   for( var i = 0 ; i < formats.length ; i++ ){
     support = video.canPlayType( formats[i] );
     if( support == 'probably' || support == 'maybe' )
     {
       return extensions[i];
     }
   }
   return "";
}

Now I can support multiple versions, and adapt easily to future changes.

• In canvasApp, I would put the var statements on top. It looks odd to see you access theCanvas without it being assigned. It works, because of hoisting, but that doesnt mean you have to put all the declarations at the bottom.

-
The messages code looks odd, I will just say that var messages = []; is preferred over var messages = new Array(); and that personally I would write

var messages = [
    {time:7,message:"", x:0 ,y:0},
    {time:1,message:"I love David Mathews Band!", x:90 ,y:200},
    {time:4,message:"Look At Those Waves!", x:240 ,y:240},
    {time:8,message:"Look At Those Rocks!", x:100 ,y:100}
]

StackExchange Code Review Q#47039, answer score: 2