Initial checkin of media test matrix class, which will be used for media perf test later.

chrome/test/functional/media_test_matrix.py

+#!/usr/bin/python
+
+# Copyright (c) 2011 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import csv
+import random
+
+VIDEO_TEST_MATRIX_HOME = 'http://tskir-html5.kir.corp.google.com/testmatrix/'

[Nirnimesh, 2011/03/17 02:32:00]

Do not reference internal URLs in public scripts. Please keep these in
pyauto_private in data dir.

[imasaki1, 2011/03/17 23:29:18]

Done.

+NOT_AVAILABLE_STRING = 'not available'
+
+
+class MediaTestMatrix:
+  """This class reads video matrix file and present them in different
+  format.

[dennis_jeffrey, 2011/03/17 17:13:36]

The first line of this class docstring should be a 1-line summary, followed by a
blank line, followed by any  additional information.  I recommend you add some
more details here about what this class does and how to use it.  What is a
"video matrix file" that needs to be given as input?  What are the "different
formats" that are output?

[imasaki1, 2011/03/17 23:29:18]

Done.

+  """
+  # Video extensions such as "webm".
+  exts = []
+  # video specifications or video descriptions.

[dennis_jeffrey, 2011/03/17 17:13:36]

Capitalize "v" in "video"

[imasaki1, 2011/03/17 23:29:18]

Done.

+  specs = {}
+  # Video titles such as "bear".
+  video_titles = []

[dennis_jeffrey, 2011/03/17 17:13:36]

Are the above lists meant to be public attributes?  If so, you need to list them
in an "Attributes:" section within the class docstring above.  If not, I believe
it's convention to name them starting with an underscore.

[imasaki1, 2011/03/17 23:29:18]

I made them private instance variables.

+
+  def ReadData(self, csv_file):
+    """Reads CSV file and stores in list specs and extensions instance
+    variables.

[dennis_jeffrey, 2011/03/17 17:13:36]

This should be a 1-line summary of this method.

[imasaki1, 2011/03/17 23:29:18]

Done.

+
+    CSV file should have the following format:
+    "ext","title1","title2",....
+    "0.webm","description1","descrption2","not available"(when it is available)
+
+    This CSV file based on javaScripts file in videotestmatrix
+      http://tskir-html5.kir.corp.google.com/testmatrix/index.html

[dennis_jeffrey, 2011/03/17 17:13:36]

As Nirnimesh said, we should not reference internal URLs in public scripts.

[imasaki1, 2011/03/17 23:29:18]

Removed.

+
+    video files are stored less than and equal to 15th row
+    audio files are stored more then 15th row

[dennis_jeffrey, 2011/03/17 17:13:36]

Capitalize the start of each sentence and put periods at the end.  Also, change
"then" to "than" here.

[imasaki1, 2011/03/17 23:29:18]

Done.

+    Args:
+      csv_file : CSV file name contains video matrix information described
+                 above

[dennis_jeffrey, 2011/03/17 17:13:36]

Put period at the end of this sentence.

[imasaki1, 2011/03/17 23:29:18]

Done.

+    """
+    reader = csv.reader(open(csv_file, 'rb'))
+    counter = 0
+    for row in reader:

[dennis_jeffrey, 2011/03/17 17:13:36]

I think you can do:

for counter, row in enumerate(reader):

This will let you avoid having to initialize counter to 0 and having to
increment it on each loop iteration below.

[imasaki1, 2011/03/17 23:29:18]

Done.

+      if counter == 0:
+        # First row is for header (video titles).
+        for title in row[1:]:
+          self.video_titles.append(title)
+          self.specs[title] = []
+      else:
+        self.exts.append(row[0])
+        for i in range(len(row)-1):
+          self.specs[self.video_titles[i]].append(row[i + 1])

[dennis_jeffrey, 2011/03/17 17:13:36]

It might be good to do some kind of error checking here.  This line assumes that
"self.video_titles[i]" is defined, which is true only if the CSV file is
formatted according to your expectations.  If the CSV file is not formatted
properly, I think an exception could be raised here.

[imasaki1, 2011/03/17 23:29:18]

Done.

+      counter += 1
+
+  def GenerateMediaInfo(self, sub_type, name):
+    """Generate media information from matrix generated by CSV file.
+
+    Args:
+      sub_type: index of the extensions (rows in the CSV file)
+      name: the name of the video file (column name in the CSV file)

[dennis_jeffrey, 2011/03/17 17:13:36]

Put periods at the end of each sentence in the above 2 lines.

[imasaki1, 2011/03/17 23:29:18]

Done.

+    Returns:

[dennis_jeffrey, 2011/03/17 17:13:36]

Put a blank line before this line.

[imasaki1, 2011/03/17 23:29:18]

Done.

+      a tuple of (info, URL, link, tag, is_video, nickname)

[dennis_jeffrey, 2011/03/17 17:13:36]

"URL" --> "url"  (or else, in line 65 below, change "url:" into "URL:")

[imasaki1, 2011/03/17 23:29:18]

Done.

+        info: description of the video (an entry in the matrix)
+        url: URL of the video or audio
+        link: href tag for video or audio
+        is_video: True if the file is video; False otherwise
+        tag: a tag string can be used to display video or audio
+        nickname: nickname of the video for presentation

[dennis_jeffrey, 2011/03/17 17:13:36]

Put periods at the end of each sentence.

[imasaki1, 2011/03/17 23:29:18]

Done.

+    """
+    if name is "none":
+      return
+    cur_video = name
+    # all videos are placed up to the rows (below 15th row)

[dennis_jeffrey, 2011/03/17 17:13:36]

Capitalize the start of each sentence and put period at the end (throughout the
file)

[imasaki1, 2011/03/17 23:29:18]

Done.

+    is_video = (sub_type <= self.__GetMaxValue(True))

[dennis_jeffrey, 2011/03/17 17:13:36]

I don't think the parens around the right-hand side of this assignment are
needed.

[imasaki1, 2011/03/17 23:29:18]

Done.

+    file = cur_video + "/" + cur_video + self.exts[sub_type]

[dennis_jeffrey, 2011/03/17 17:13:36]

When generating a path string, it's safer to use "os.path.join".

[imasaki1, 2011/03/17 23:29:18]

Done.

+    # specs were generated from CSV file
+    info = self.specs[cur_video][sub_type]
+    url = VIDEO_TEST_MATRIX_HOME + file
+    link = ''.join(["<b><a href='", VIDEO_TEST_MATRIX_HOME,
+                    file, "'>", file, '</a>&nbsp;&nbsp;', info, '</b>'])

[dennis_jeffrey, 2011/03/17 17:13:36]

Is there a reason you're creating the string this way, rather than using the
string formatting operator (here and below)?:

link = '<b><a href="%s">%s</a>&nbsp;&nbsp;%s</b>' % (url, file, info)

[imasaki1, 2011/03/17 23:29:18]

Done.

+    tag = ''
+    if is_video:
+        tag = ''.join(["<video id='v' controls autoplay ",
+                       "playbackRate=1 loop valign=top src='", file, "'>"])
+    else:
+        tag = ''.join(["<audio id='v' controls autoplay ",
+                       "playbackRate=1 loop valign=top src='", file, "'>"])

[dennis_jeffrey, 2011/03/17 17:13:36]

Lines 83-88: the only difference between these two assignments is that one is
"video" and the other is "audio".  I recommend cutting down on the code like
this:

type = ['audio', 'video'][is_video]
tag = ''.join(["<", type, " id='v' controls autoplay ",
               "playbackRate=1 loop valign=top src='", file, "'>"])

[imasaki1, 2011/03/17 23:29:18]

Done.

+    nickname = cur_video + self.exts[sub_type]
+    return (info, url, link, tag, is_video, nickname)
+
+  def __GetMaxValue(self, video_only):
+    """Get maximum index value of exts for video or audio
+
+    Args:
+      video_only: True if the max value is for video
+    Returns:

[dennis_jeffrey, 2011/03/17 17:13:36]

Put blank line before this line.

[imasaki1, 2011/03/17 23:29:18]

Done.

+      the max index value of exts for video or audio
+    """
+    if video_only:
+      return 15

[dennis_jeffrey, 2011/03/17 17:13:36]

The 15 should probably be defined as a named constant at the top of the file.

[imasaki1, 2011/03/17 23:29:18]

Done.

+    else:
+      # max index is the size of the directory
+      return len(self.exts)-1

[dennis_jeffrey, 2011/03/17 17:13:36]

A shorter way to implement this function:

return [len(self.exts) - 1, 15][video_only]

[imasaki1, 2011/03/17 23:29:18]

Done.

+
+  def GenerateRandomMediaInfo(self, video_only):
+    """Generate random video info can be used for playing this media.

[dennis_jeffrey, 2011/03/17 17:13:36]

"info can" --> "info that can"?

[imasaki1, 2011/03/17 23:29:18]

Done.

+
+    Args:
+      video_only: True if generate random video only
+    Returns:
+      a list of a tuples (info, url, link, tag, is_video, nickname)
+        info: description of the video (an entry in the matrix)
+        url: URL of the video/audio
+        link: href tag for video or audio
+        tag: a tag string can be used to display video or audio
+        is_video: True if the file is video; False otherwise
+        nickname: nickname of the video for presentation
+    """
+    # Try 10 times to find avaialble video/audio.
+    for i in range(10):

[dennis_jeffrey, 2011/03/17 17:13:36]

Maybe the value 10 should be an input to this function, so that it can change if
the user desires.

[imasaki1, 2011/03/17 23:29:18]

Done.

+      sub_type = random.randint(0, self.GetMaxValue(video_only))
+      name = self.video_titles[random.randint(0, len(self.video_titles)-1)]
+      (info, url, link, tag, is_video) = self.GenerateMediaInfo(sub_type, name)
+      if "not available" not in info:

[dennis_jeffrey, 2011/03/17 17:13:36]

Use "NOT_AVAILABLE_STRING".

[imasaki1, 2011/03/17 23:29:18]

Done.

+        return  (info, url, link, tag, is_video, nickname)
+    # Gives up after that (very small chance)
+    return None
+
+  def GenerateAllMediaInfos(self, video_only):
+    """Generate all video infos that can be used for playing this media.
+
+    Args:
+      video_only: True if generate random video only (not audio)
+    Returns:
+      a list of a tuples (info, url, link, tag, is_video, nickname)
+        info: description of the video (an entry in the matrix)
+        url: URL of the video/audio
+        is_video: True if the file is video; False otherwise
+        nickname: nickname of the video for presentation (such as bear.webm)
+    """
+    video_infos = []
+
+    for i in range(0, len(self.video_titles)-1):
+      name = self.video_titles[i]
+      for sub_type in range(self._GetMaxValue(video_only)):
+        (info, url, link, tag,
+         is_video, nickname) = self.GenerateMediaInfo(sub_type, name)
+        if NOT_AVAILABLE_STRING not in info:
+          video_infos.append([info, url, link, tag, is_video, nickname])
+    return video_infos
+
+  def GenerateAllMediaInfosInCompactForm(self, video_only):
+    """Generate all video info can be used for playing this media
+    (in compact form).

[dennis_jeffrey, 2011/03/17 17:13:36]

Summary line should fit on one line.

[imasaki1, 2011/03/17 23:29:18]

Done.

+
+    Args:
+      video_only: True if generate random video only
+    Returns:
+      a list of a tuples (url, nickname, tag)
+        url: URL of the video/audio
+        nickname: nickname of the video/audio for presentation
+                  (such as bear.webm)
+        tag: HTML5 tag for the video/audio
+    """
+    video_infos = []
+    for i in range(0, len(self.video_titles)-1):
+      name = self.video_titles[i]
+      for sub_type in range(self.__GetMaxValue(video_only)):
+        (info, url, link, tag,
+         is_video, nickname) = self.GenerateMediaInfo(sub_type, name)
+        if is_video:
+          tag = 'video'
+        else:
+          tag = 'audio'

[dennis_jeffrey, 2011/03/17 17:13:36]

Lines 172-175:

tag = ['audio', 'video'][is_video]

[imasaki1, 2011/03/17 23:29:18]

Done.

+        if NOT_AVAILABLE_STRING not in info:
+          video_infos.append([url, nickname, tag])
+    return video_infos
+
+  @staticmethod
+  def LookForMediaInfoByNickName(compact_list, target):
+    """Look for video by its nickname in the compact_list.
+
+    Args:
+      compact_list: list generated by bgenerateAllMediaInfosInCompactForm

[dennis_jeffrey, 2011/03/17 17:13:36]

"bgenerateAllMediaInfosInCompactForm" -->
"GenerateAllMediaInfosInCompactForm"

[imasaki1, 2011/03/17 23:29:18]

Done.

+      target: taget nickname

[Nirnimesh, 2011/03/17 02:32:00]

need a blank line before Returns:

Repeat for all the methods above

[imasaki1, 2011/03/17 23:29:18]

Done.

+    Returns:
+      A tuple (info, url, link, tag, is_video, nickname) where nickname
+      is target
+        url: URL of the video/audio
+        tag: HTML5 tag for the video/audio
+        nickname: nickname of the video/audio for presentation
+                  (such as bear.webm)
+    """
+    for l in compact_list:
+      [url, nickname, tag] = l

[Nirnimesh, 2011/03/17 02:32:00]

You can use:

for url, nickname, tag in compact_list:
...

[imasaki1, 2011/03/17 23:29:18]

Done.

+      if(target == nickname):

[Nirnimesh, 2011/03/17 02:32:00]

remove parens

[imasaki1, 2011/03/17 23:29:18]

Done.

+            return l

[dennis_jeffrey, 2011/03/17 17:13:36]

This should only be indented 2 spaces underneath the line above it.

[imasaki1, 2011/03/17 23:29:18]

Done.