6
\$\begingroup\$

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.

<!doctype html>
<html>
<head>
<script type="text/javascript">
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  ("Duration:"     + videoElement.duration,10,280);
        context.fillText  ("Current time:" + videoElement.currentTime,260,280);
        context.fillText  ("Loop: "        + videoElement.loop,10,290);
        context.fillText  ("Autoplay: "    + videoElement.autoplay,80,290);
        context.fillText  ("Muted: "       + videoElement.muted,160,290);
        context.fillText  ("Controls: "    + videoElement.controls,240,290);
        context.fillText  ("Volume: "      + videoElement.volume,320,290);

        //Display Message
        for (var i =0; i < messages.length ; i++) {
            var tempMessage = messages[i];
            if (videoElement.currentTime > tempMessage.time) {
                context.font = "bold 14px sans";
                context.fillStyle    = "#FFFF00";
                context.fillText  (tempMessage.message,  tempMessage.x                                                                 ,
                            tempMessage.y);
            }
        }
    }

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

    var theCanvas = document.getElementById('canvasOne');
    var context   = theCanvas.getContext('2d');
    videoElement.play();

    function gameLoop() {
        window.setTimeout(gameLoop, 20);
        drawScreen();   
    }

    gameLoop();
}
</script>

</head>
<body>
<div style="position: absolute; top: 50px; left: 50px;">
       <canvas id="canvasOne" width="500" height="300"></canvas>
</div>

</body>
</html>
\$\endgroup\$
3
  • \$\begingroup\$ Based largely on the O'Reilly book HTML5 Canvas, Chapter 6. Video samples at orm-other.s3.amazonaws.com/html5canvasexamplecontent/chapter6/… (or .mp4 or .ogg). \$\endgroup\$ Apr 13, 2014 at 3:11
  • \$\begingroup\$ You are corrrect. That has been one of the best books that I have purchased in the base couple of years. I think that I sleep with the thing day in and day out and even read while I am eating dinner. The only thing I have seen about their code is that clearing the canvas works the way that they do it but I am not certain that is how it should be done. Perfect book i will have to say. it is worth the money. \$\endgroup\$ Apr 13, 2014 at 20:10
  • \$\begingroup\$ In the book they put all of the main code in one function and then have other functions that are inside of that code. This way they do not have to pass variables up the chain of function calls which works good but I have always been told to send parameters in a function call. \$\endgroup\$ Apr 13, 2014 at 20:11

1 Answer 1

2
\$\begingroup\$

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}
    ]
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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