8
\$\begingroup\$

Although my code works as expected, there are a few gotchas.

  1. A single row is not filled at once; instead, I can see partially filled rows during the rendering process(Fixed in the updated code below.).
  2. The application still feels slow; can it be optimized further?
  3. And the biggest thing, how can I make the code more OOPsy?

index.html

 <!DOCTYPE html>
    <html>
      <head>
        <script type="text/javascript" src="js/client.js"></script>
        <script type="text/javascript" src="js/mosaic.js"></script>
        <title>Mosaic</title>
        <style>
          .container {
            margin: 0 auto;
            width: 50%;
          }
          .container ul {
            list-style: none;
            margin-left: 0;
          }
        </style>
      </head>
      <body>
        <script id="worker1" type="javascript/worker">
          var SVG_URL = 'http://localhost:8765/color/';
          function httpGet(url) {
            return new Promise(function(resolve, reject) {
              var req = new XMLHttpRequest();
              req.open('GET', url);
              req.onload = function() {
                if (req.status == 200) {
                  resolve(req.response);
                } else {
                  reject(Error(req.statusText));
                }
              };
              req.onerror = function() { reject(Error('Network Error')); };
              req.send();
            });
          };

      function getSvg(data) {
        return httpGet(SVG_URL + data.hex)
            .then(function(svg) {
              return {svg: svg, x: data.x, y: data.y};
            })
            .catch(function(error) {
              console.log(error);
            });
      };

      function messageHandler(e) {
        var chunks = e.data;
        Promise.all(chunks.map(function(data) { return getSvg(data); }))
          .then(function(response) {
            self.postMessage(response);
            // Close the woker to be garbage collected.
            //self.close();
          });
      };

      self.addEventListener('message', messageHandler, false);
    </script>

    <div class="container">
      <ul id="image-list">
      </ul>
      <input id="input" type="file" accept="image/*">
    </div>
    <script>
      (function(app) {
        document.addEventListener('DOMContentLoaded', function() {
          var blob = new Blob([document.querySelector('#worker1').textContent]);
          app.run(window.URL.createObjectURL(blob));
        });
      })(window.app || (window.app = {}));
    </script>
  </body>
</html>

index.js

    /**
     * @fileoverview Creates the PhotoMosaic of the given image.
     * @author Vivek Poddar
     */

 (function(window, document, app) {
  'use strict';

  var DOMURL = window.URL || window.webkitURL;

  // classy, since V8 prefers predictable objects.
  function Tile(rgb, x, y) {
    this.hex = Tile.rgbToHex(rgb);
    this.x = x * TILE_WIDTH;
    this.y = y * TILE_HEIGHT;
  };

  // classy, since V8 prefers predictable objects.
  function SVGTile(svg, x, y) {
    this.svgURL = SVGTile.createSVGUrl(svg);
    this.x = x;
    this.y = y;
    this.width  = TILE_WIDTH;
    this.height = TILE_HEIGHT;
  };

  SVGTile.createSVGUrl = function(svg) {
    var svgBlob = new Blob([svg], {type: 'image/svg+xml;charset=utf-8'});
    return DOMURL.createObjectURL(svgBlob);
  };

  Tile.componentToHex = function(c) {
    var hex = c.toString(16);
    return hex.length == 1 ? '0' + hex : hex;
  };

  Tile.rgbToHex = function(rgb) {
    return Tile.componentToHex(rgb[0]) + Tile.componentToHex(rgb[1]) +
        Tile.componentToHex(rgb[2]);
  };

  /**
   * Draws a offscreen canvas to get averaged rgb per tile.
   * see https://stackoverflow.com/a/17862644/4260745
   */
  function getOffScreenContext(width, height) {
    var canvas = document.createElement('canvas');
    canvas.width  = width;
    canvas.height = height;
    return canvas.getContext('2d');
  };

  /**
   * Gets tiles data from the source image.
   * @param {HTMLElement} sourceImage
   * @return {!Array<!Tile>}
   */
  function getTiles(sourceImage, tilesX, tilesY) {
    var res = [];
    var context = getOffScreenContext(tilesX, tilesY);
    // see https://stackoverflow.com/a/17862644/4260745
    context.drawImage(sourceImage, 0, 0, tilesX, tilesY);
    var data = context.getImageData(0, 0, tilesX, tilesY).data;
    var i = 0;
    for (var row = 0; row < tilesY; row++) {
      for (var col = 0; col < tilesX; col++) {
        res.push(new Tile(data.subarray(i * 4, i * 4 + 3), col, row));
        i++;
      }
    }
    return res;
  };

  /**
   * @param {CanvasRenderingContext2D} ctx
   * @param {!Array<!SVGTile>} tiles
   */
  function drawTiles(ctx, tiles) {
    var context = getOffScreenContext(tiles.length * TILE_WIDTH, TILE_HEIGHT);
    tiles.forEach(function(tile, index) {
      renderTile(context, tile, function() {
        if (tiles.length === index + 1) {
          ctx.drawImage(context.canvas, 0, tiles[0].y);
        }
      });
    });
  };

  /**
   * Draws PhotoMosaic on screen.
   * @param {HTMLElement} image The source image from file input.
   */
  function drawMosiac(image, ctx, url) {
    var rowData = {};
    function renderRow(i) {
      if (!rowData[i]) return i;
      var tiles = [];
      rowData[i].forEach(function(data) {
        var tile = new SVGTile(data.svg, data.x, data.y);
        tiles.push(tile);
      });
      drawTiles(ctx, tiles);
      return renderRow(++i);
    };
    var tilesX = Math.floor(image.width / TILE_WIDTH);
    var tilesY = Math.floor(image.height / TILE_HEIGHT);

    var tiles = getTiles(image, tilesX, tilesY);
    var i = 0;
    var maxWorkers = navigator.hardwareConcurrency || 4;
    function runWorker(worker) {
      worker.onmessage = function(e) {
        var row = e.data[0].y / TILE_HEIGHT;
        rowData[row] = e.data;
        if (row === i) {
          i = renderRow(i);
        }
        if (tiles.length) {
          runWorker(worker)
        } else {
          worker.terminate();
        };
      }
      worker.postMessage(tiles.splice(0, tilesX));
    }
    if (tiles.length) {
      for(var x = maxWorkers; x--; ) runWorker(new Worker(url));
    }
  };

  /**
   * Renders svg tile on the given context.
   * @param {CanvasRenderingContext2D} ctx
   * @param {!Tile} tile The tile to render.
   * @param {function()} callback To be called after image is loaded.
   * @throws Error
   */
  function renderTile(ctx, tile, callback) {
    var img = new Image();
    img.onload = function() {
      try {
        ctx.drawImage(this, tile.x, 0, tile.width, tile.height);
        ctx.imageSmoothingEnabled = false;
        ctx.mozImageSmoothingEnabled = false;
        DOMURL.revokeObjectURL(tile.svgURL);
        callback();
      } catch (e) {
        throw new Error('Could not render image' + e);
      }
    };
    img.src = tile.svgURL;
  };

  /**
   * Handles image upload.
   * @param {function(HTMLElement)} callback To be called after image loads.
   */
  function handleFileUpload(callback) {
    var img = new Image();
    img.src = window.URL.createObjectURL(this);
    img.onload = function() { callback(this); }
  };

  /**
   * Main function which starts the rendering process.
   * @throws RangeError
   */
  app.run = function run(url) {
    var inputElement  = document.getElementById('input');
    var outputElement = document.getElementById('output');
    inputElement.addEventListener('change', function() {
      handleFileUpload.call(this.files[0], function(image) {
        if (image.width < TILE_WIDTH || image.height < TILE_HEIGHT) {
          console.log(TILE_WIDTH, TILE_HEIGHT);
          throw new RangeError(
              'Tile dimension cannot be greater than source image.');
        }
        var canvas  = document.createElement('canvas');
        var context = canvas.getContext('2d');
        canvas.width = image.width;
        canvas.height = image.height;
        drawMosiac(image, context, url);
        outputElement.appendChild(canvas);
      });
    }, false);
  };
})(window, document, window.app || (window.app = {}));
\$\endgroup\$
  • \$\begingroup\$ I don't know rules enough in here so I'll let it in a comment for now, but performance wise, you're over-complicating everything and not taking any advantage of canvas' methods... Here is a fiddle showing you a completely refactored code. \$\endgroup\$ – Kaiido Jul 14 '16 at 4:43
  • \$\begingroup\$ @Kaiido looks good to me, but to let you know I needed to fetch teh svgs from the server (this was an interview question). \$\endgroup\$ – CodeYogi Jul 14 '16 at 5:32
  • \$\begingroup\$ Well As I said in comments, at the end of the draw function, I think it's a bad idea, and I would have tell the interviewer about it. But you just have to replace the var svgData = '<svg... and svgShape.src =... to an xhr request to the server (requesting a single black shape) and you're done. \$\endgroup\$ – Kaiido Jul 14 '16 at 5:44
  • \$\begingroup\$ Its not about just replacing the data with XHR, the whole lot of problem arises from that, like doing a lot of network calls, using promises and all \$\endgroup\$ – CodeYogi Jul 14 '16 at 6:33
  • 1
    \$\begingroup\$ The only global in my code is doMozaic. The idea to fetch one colored shape per tile is a bad idea, to keep the things simple is always better IMO, and even if you die to show how much you know about different APIs, as an interviewer, I would prefer if you are able to do the same thing, faster, with less work for everyone. These APIs are useless in this case and won't serve the final product. As the interviewed I would have tell it's not the way to do it. My code works way faster than yours, make only one request to the server, and works in all browsers from IE10 (File API won't work in IE9) \$\endgroup\$ – Kaiido Jul 14 '16 at 7:03
2
\$\begingroup\$

Obviously this code was posted nearly 3 years ago, so you have likely updated it, and since then has become a lot more prevalent/standard, so you could utilize many features/keywords of that specification to streamline this code. You could also use the fetch API or another promise-based XHR library to simplify the XHR promise code.

Your Questions

The application still feels slow; can it be optimized further?

You could consider using for...of or even regular for loops instead of functional iterators.

And the biggest thing, how can I make the code more OOPsy?

You could use the class syntax, even though it is just "syntactical sugar over JavaScript's existing prototype-based inheritance"1.

Other feedback/suggestions

Let's look at the forEach loop within renderRow() (defined inside drawMosiac()).

var tiles = [];
rowData[i].forEach(function(data) {
  var tile = new SVGTile(data.svg, data.x, data.y);
  tiles.push(tile);
});

Perhaps you know this already but this code could be simplified using .map().

const tiles = rowData[i].map(data => new SVGTile(data.svg, data.x, data.y));

If you kept the .forEach() then there isn't much point in assigning the value to tile right before it is pushed into the array - a linter should point that out as excess memory utilization. As was mentioned above, the functional code might be one reason why "the application still feels slow", so you might consider using a for loop if necessary.


One micro-optimization might be to move the incrementing of i here:

res.push(new Tile(data.subarray(i * 4, i * 4 + 3), col, row));
    i++;

To have the post-fix increment operator after the last usage of i within the subarray() call:

res.push(new Tile(data.subarray(i * 4, i++ * 4 + 3), col, row));

1https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes

\$\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.