Making webrtc video quality test page generic

chrome/test/data/webrtc/video_extraction.js

Index: chrome/test/data/webrtc/video_extraction.js
diff --git a/chrome/test/data/webrtc/video_extraction.js b/chrome/test/data/webrtc/video_extraction.js
index 93cf9e7fc49312342d6aab5981f2dd9bf666a273..e85b7bf85dbb37062f8265c2b16b36cb627c6062 100644
--- a/chrome/test/data/webrtc/video_extraction.js
+++ b/chrome/test/data/webrtc/video_extraction.js
@@ -11,6 +11,12 @@
 var gVideoId = 'remote-view';
 
 /**
+ * The ID of the canvas tag from which the frames are captured.
+ * @private
+ */
+var gCanvasId = 'remote-canvas';
+
+/**
  * Counts the number of frames that have been captured. Used in timeout
  * adjustments.
  * @private
@@ -78,21 +84,30 @@ window.onload = function() {
 /**
  * Starts the frame capturing.
  *
- * @param {Number} The width of the video/canvas area to be captured.
- * @param {Number} The height of the video area to be captured.
- * @param {Number} The height of the canvas where we put the video frames.
+ * @param {String} The ID of the video tag from which the height and width
+ *                 parameters are to be extracted.
  * @param {Number} The frame rate at which we would like to capture frames.
  * @param {Number} The duration of the frame capture in seconds.
  */
-function startFrameCapture(width, height, canvas_height, frame_rate, duration){
+function startFrameCapture(videoTagName, frame_rate, duration) {

[kjellander_chromium, 2014/04/29 08:17:37]

I suggest changing the first param to be the HTML element object instead, like
this:
function startFrameCapture(videoTag, frame_rate, duration) {

Then you should also document it using non-nullable annotation: 
{!Object} (see
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...)

[phoglund_chromium, 2014/04/29 08:48:20]

And how do you get a ref to the video tag object in the onplay context?

[kjellander_chromium, 2014/04/29 08:57:48]

Like this:
startFrameCapture(this, 30, 5)

[amogh.bihani, 2014/04/29 10:38:53]

Done.

   gFrameCaptureInterval = 1000/frame_rate;
   gCaptureDuration = 1000 * duration;
 
-  console.log('Received width is: ' + width + ', received height is: ' + height
-              + ', capture interval is: ' + gFrameCaptureInterval +
-              ', duration is: ' + gCaptureDuration);
+  var video = document.getElementById(videoTagName);

[kjellander_chromium, 2014/04/29 08:17:37]

...then you can remove this lookup and also the check below. That should be safe
since tests using this test page are checking for JS Console errors
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...).
Then a failure to pass the object would cause the test to fail.

[amogh.bihani, 2014/04/29 10:38:53]

Done.

+  if (!video)
+    throw failTest('Did not find the element ' + videoTagName + '.');
+  if (video.videoWidth == 0 || video.videoHeight == 0) {
+    throw failTest('Trying to capture from ' + videoTagName +
+                    ' but it is not playing any video');
+  }
+
+  console.log('Received width is: ' + video.videoWidth +
+              ', received height is: ' + video.videoHeight +
+              ', capture interval is: ' + gFrameCaptureInterval +
+              ', Duration is: ' + gCaptureDuration);
+
   gStartOfTime = new Date().getTime();
-  setTimeout(function() { shoot(width, height, canvas_height); },
+  setTimeout(function() { shoot(video.videoWidth, video.videoHeight); },
              gFrameCaptureInterval);
 }
 
@@ -107,7 +122,7 @@ function startFrameCapture(width, height, canvas_height, frame_rate, duration){
  * @return {Canvas}
  */
 function capture(video, width, height) {
-  var canvas = document.getElementById('remote-canvas');
+  var canvas = document.getElementById(gCanvasId);

[kjellander_chromium, 2014/04/29 08:17:37]

I think there's room for improvement here, creating the canvas dynamically
instead, like:
var canvas = document.createElement('canvas');
document.body.appendChild(canvas);

It should only be done once though, possibly using a global variable to check if
it's created or not (creating it in the startFrameCapture function seems nicer
than in this function).

That work can be done in a future CL though.

[phoglund_chromium, 2014/04/29 08:48:20]

Yeah, I suggested that too but in a future CL.

   var ctx = canvas.getContext('2d');
   ctx.drawImage(video, 0, 0, width, height);
   return canvas;
@@ -121,9 +136,8 @@ function capture(video, width, height) {
  *
  * @param {Number} The width of the video/canvas area to be captured.
  * @param {Number} The height of the video area to be captured.
- * @param {Number} The height of the canvas where we put the video frames.
  */
-function shoot(width, height, canvas_height){
+function shoot(width, height) {
   // The first two captured frames have big difference between the ideal time
   // interval between two frames and the real one. As a consequence this affects
   // enormously the interval adjustment for subsequent frames. That's why we
@@ -141,14 +155,8 @@ function shoot(width, height, canvas_height){
 
   // Extract the data from the canvas.
   var ctx = canvas.getContext('2d');
-  var img;
-  if (height == canvas_height) {
-    // We capture the whole video frame.
-    img = ctx.getImageData(0, 0, width, height);
-  } else {
-    // We capture only the barcode (canvas_height is the height of the barcode).
-    img = ctx.getImageData(0, 0, width, canvas_height);
-  }
+  // We capture the whole video frame.
+  var img = ctx.getImageData(0, 0, width, height);
   gFrames.push(img.data.buffer);
   gFrameCounter++;
 
@@ -160,7 +168,7 @@ function shoot(width, height, canvas_height){
 
   if (real_time_elapsed < gCaptureDuration) {
     // If duration isn't over shoot again
-    setTimeout(function() { shoot(width, height, canvas_height); },
+    setTimeout(function() { shoot(width, height); },
                gFrameCaptureInterval - diff);
   } else {  // Else reset gFrameCounter and send the frames
     dDoneFrameCapturing = true;