Merge lp:~bladernr/checkbox/1100594-fix-camera_test-new-gir1.2 into lp:checkbox

[Jeff Lane ]

fixes camera test by adding bits to determine what version of gst we're using and then set the plugin and video type appropriately for either 0.10 or 1.x. This resolves a problem when fswebcam is not present and we fall back to gst.

Fixed the error handling for the fswebcam call to avoid having nested tracebacks that can be confusing. Now if the call to fswebcam fails, we instead trap the error and run the fallback code outside the exception. If THAT fails, we'll get a less confusing traceback now.

Removed an extraneous line in the requires for one of the camera test jobs (listed gir1.2 twice)

[Brendan Donegan (brendan-donegan)]

There seem to be some pep8 errors in your code related to spacing between ',' mainly. I can't remember for certain but I think this may cause Tarmac to reject the submission. Also I'm curious about the comment about fswebcam above - I have fswebcam installed on my system and still get this error. Is there a further problem in the script related to that? Probably best to check it out now.

review: Needs Fixing

[Zygmunt Krynicki (zyga)]

- package.name == 'gir1.2-gst-plugins-base-0.10' or package.name == 'gir1.2-gst-plugins-base-1.0'

This is a bit wrong in general (not the actual change but stuff really required). Anything that uses gir needs to depend on python3-gi

[lp:~bladernr/checkbox/1100594-fix-camera_test-new-gir1.2]

2099. By Jeff Lane  on 2013-05-03

"[r=zkrynicki][bug=1103647][author=bladernr] automatic merge by tarmac"

2100. By Brendan Donegan on 2013-05-03

"[r=zkrynicki][bug=1093718][author=brendan-donegan] automatic merge by tarmac"

2101. By Jeff Lane  on 2013-05-03

Fixed camera_test to work with both gir1.2-0.10 and gir1.2-1.x

2102. By Jeff Lane  on 2013-05-03

Removed an exraneous requires line in one of the camera test jobs

2103. By Jeff Lane  on 2013-05-03

Fixed problems with camera_test in Raring with gir1.2-1.0.6 changes

2104. By Jeff Lane  on 2013-05-03

Fixed a problem where running camera_test without a test specified resulted in a traceback. A lot of PEP8 fixes

[Jeff Lane  (bladernr)]

Brendan: fixed my two pep8 errors (spaces) the rest are original to sylvain's code. I've fixed all of those but one because no matter what I tried, I couldn't keep it from generating several more pep8 problems that were seemingly unsolvable. But this one remaining one has been in the test for a long while now so it shouldn't affect things, I hope.

As for fswebcam, it's not specifically the lack of fswebcam, that just brought it to the attention of the guy who reported the bug initially. IIRC, you mentioned the "display" test, which uses gstreamer to create the display window, so I'd expect you would see that bug there.

If this updated script doesn't resolve it on your end, there may be more going on than simply setting the right plugin and video type for gstreamer.

Zygmunt: Yeah, probably so, there are 17 scripts that rely on python3-gi, none of which list it as a requirement. I'm not going to fix those here. That said, however, python3-gi is required for installing checkbox itself, rathter than just a suggests. Then again, so are the gir1.2 packages, so really, neither of those should be required on a test by test basis, you'd think, since they're already required for installing checkbox to begin with.

Anyway, here's a new version with fixes to some problems I didn't catch the first time, and a rash of PEP8 fixes as well.

review: Needs Resubmitting

[Brendan Donegan (brendan-donegan)]

To me it looks fine then. I hope Tarmac doesn't get pissy about the pep8 errors - FYI only files which are modified get checked for errors, so that could be the reason why previous ones were missed, since this file hasn't been touched in a long time.

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2013-05-03 10:47:13 +0000
+++ debian/changelog	2013-05-03 14:34:26 +0000
@@ -10,6 +10,9 @@
     than define elements during parsing of /proc/cpuinfo (LP: #1111878)
   * scripts/lsmod_info: Corrected error handling for the check_output() call to
     trap the correct error. (LP: #1103647)
+  * jobs/camera.txt.in: removed an extraneous requres line for gir1.2
+    scripts/camera_test: added code to determine what version of gst we're
+    using and set video type and plugin accordingly. (LP: #1100594)
 
   [ Sylvain Pineau ]
   * jobs/suspend.txt.in, scripts/gpu_test: Remove the need of running the script
 
=== modified file 'jobs/camera.txt.in'
--- jobs/camera.txt.in	2012-11-06 17:11:49 +0000
+++ jobs/camera.txt.in	2013-05-03 14:34:26 +0000
@@ -9,7 +9,6 @@
 name: camera/display
 depends: camera/detect
 requires:
- package.name == 'gir1.2-gst-plugins-base-0.10' or package.name == 'gir1.2-gst-plugins-base-1.0'
  device.category == 'CAPTURE'
 command: camera_test display
 _description:
 
=== modified file 'scripts/camera_test'
--- scripts/camera_test	2012-11-01 11:58:46 +0000
+++ scripts/camera_test	2013-05-03 14:34:26 +0000
@@ -162,11 +162,13 @@
     """
     A simple class that displays a test image via GStreamer.
     """
-    def __init__(self, args):
+    def __init__(self, args, gst_plugin=None, gst_video_type=None):
         self.args = args
         self._mainloop = GObject.MainLoop()
         self._width = 640
         self._height = 480
+        self._gst_plugin = gst_plugin
+        self._gst_video_type = gst_video_type
 
     def detect(self):
         """
@@ -186,19 +188,25 @@
             print("    name   : %s" % cp.card.decode('UTF-8'))
             print("    driver : %s" % cp.driver.decode('UTF-8'))
             print("    version: %s.%s.%s"
-            % (cp.version >> 16, (cp.version >> 8) & 0xff, cp.version & 0xff))
+                  % (cp.version >> 16,
+                  (cp.version >> 8) & 0xff,
+                  cp.version & 0xff))
             print("    flags  : 0x%x [" % cp.capabilities,
-            ' CAPTURE' if cp.capabilities & V4L2_CAP_VIDEO_CAPTURE else '',
-            ' OVERLAY' if cp.capabilities & V4L2_CAP_VIDEO_OVERLAY else '',
-            ' READWRITE' if cp.capabilities & V4L2_CAP_READWRITE else '',
-            ' STREAMING' if cp.capabilities & V4L2_CAP_STREAMING else '',
-            ' ]', sep="")
+                  ' CAPTURE' if cp.capabilities & V4L2_CAP_VIDEO_CAPTURE
+                  else '',
+                  ' OVERLAY' if cp.capabilities & V4L2_CAP_VIDEO_OVERLAY
+                  else '',
+                  ' READWRITE' if cp.capabilities & V4L2_CAP_READWRITE
+                  else '',
+                  ' STREAMING' if cp.capabilities & V4L2_CAP_STREAMING
+                  else '',
+                  ' ]', sep="")
 
             resolutions = self._get_supported_resolutions(device)
             print('    ',
-            self._supported_resolutions_to_string(resolutions).replace(
-            "\n", " "),
-            sep="")
+                  self._supported_resolutions_to_string(resolutions).replace(
+                  "\n", " "),
+                  sep="")
 
             if cp.capabilities & V4L2_CAP_VIDEO_CAPTURE:
                 cap_status = 0
@@ -209,10 +217,12 @@
         Activate camera (switch on led), but don't display any output
         """
         pipespec = ("v4l2src device=%(device)s "
-                    "! video/x-raw-yuv "
-                    "! ffmpegcolorspace "
+                    "! %(type)s "
+                    "! %(plugin)s "
                     "! testsink"
-                    % {'device': self.args.device})
+                    % {'device': self.args.device,
+                       'type': self._gst_video_type,
+                       'plugin': self._gst_plugin})
         self._pipeline = Gst.parse_launch(pipespec)
         self._pipeline.set_state(Gst.State.PLAYING)
         time.sleep(10)
@@ -223,12 +233,14 @@
         Displays the preview window
         """
         pipespec = ("v4l2src device=%(device)s "
-                    "! video/x-raw-yuv,width=%(width)d,height=%(height)d "
-                    "! ffmpegcolorspace "
+                    "! %(type)s,width=%(width)d,height=%(height)d "
+                    "! %(plugin)s "
                     "! autovideosink"
                     % {'device': self.args.device,
+                       'type': self._gst_video_type,
                        'width': self._width,
-                       'height': self._height})
+                       'height': self._height,
+                       'plugin': self._gst_plugin})
         self._pipeline = Gst.parse_launch(pipespec)
         self._pipeline.set_state(Gst.State.PLAYING)
         time.sleep(10)
@@ -240,11 +252,11 @@
         """
         if self.args.filename:
             self._still_helper(self.args.filename, self._width, self._height,
-                self.args.quiet)
+                               self.args.quiet)
         else:
             with NamedTemporaryFile(prefix='camera_test_', suffix='.jpg') as f:
                 self._still_helper(f.name, self._width, self._height,
-                    self.args.quiet)
+                                   self.args.quiet)
 
     def _still_helper(self, filename, width, height, quiet, pixelformat=None):
         """
@@ -253,25 +265,30 @@
         user (quiet = True means do not display image).
         """
         command = ["fswebcam", "-S 3", "--no-banner",
-                  "-d", self.args.device,
-                  "-r", "%dx%d"
-                  % (width, height), filename]
+                   "-d", self.args.device,
+                   "-r", "%dx%d"
+                   % (width, height), filename]
+        use_gstreamer = False
         if pixelformat:
             command.extend(["-p", pixelformat])
 
         try:
             check_call(command, stdout=open(os.devnull, 'w'), stderr=STDOUT)
         except (CalledProcessError, OSError):
-            pipespec = \
-                    ("v4l2src device=%(device)s "
-                     "! video/x-raw-yuv,width=%(width)d,height=%(height)d "
-                     "! ffmpegcolorspace "
-                     "! jpegenc "
-                     "! filesink location=%(filename)s"
-                     % {'device': self.args.device,
-                         'width': width,
-                        'height': height,
-                        'filename': filename})
+            use_gstreamer = True
+
+        if use_gstreamer:
+            pipespec = ("v4l2src device=%(device)s "
+                        "! %(type)s,width=%(width)d,height=%(height)d "
+                        "! %(plugin)s "
+                        "! jpegenc "
+                        "! filesink location=%(filename)s"
+                        % {'device': self.args.device,
+                           'type': self._gst_video_type,
+                           'width': width,
+                           'height': height,
+                           'plugin': self._gst_plugin,
+                           'filename': filename})
             self._pipeline = Gst.parse_launch(pipespec)
             self._pipeline.set_state(Gst.State.PLAYING)
             time.sleep(3)
@@ -290,7 +307,7 @@
         ret = ""
         for resolution in supported_resolutions:
             ret += "Format: %s (%s)\n" % (resolution['pixelformat'],
-                resolution['description'])
+                   resolution['description'])
             ret += "Resolutions: "
             for res in resolution['resolutions']:
                 ret += "%sx%s," % (res[0], res[1])
@@ -314,13 +331,13 @@
         resolution = resolutions[0]
         if resolution:
             print("Taking multiple images using the %s format"
-                % resolution['pixelformat'])
+                  % resolution['pixelformat'])
             for res in resolution['resolutions']:
                 w = res[0]
                 h = res[1]
                 f = NamedTemporaryFile(prefix='camera_test_%s%sx%s' %
-                    (resolution['pixelformat'], w, h), suffix='.jpg',
-                     delete=False)
+                                       (resolution['pixelformat'], w, h),
+                                       suffix='.jpg', delete=False)
                 print("Taking a picture at %sx%s" % (w, h))
                 self._still_helper(f.name, w, h, True,
                                    pixelformat=resolution['pixelformat'])
@@ -329,7 +346,7 @@
                     os.remove(f.name)
                 else:
                     print("Failed to validate image %s" % f.name,
-                        file=sys.stderr)
+                          file=sys.stderr)
                     os.remove(f.name)
                     return 1
             return 0
@@ -354,9 +371,9 @@
                         pixelformat['pixelformat_int'] = fmt.pixelformat
                         pixelformat['pixelformat'] = "%s%s%s%s" % \
                             (chr(fmt.pixelformat & 0xFF),
-                            chr((fmt.pixelformat >> 8) & 0xFF),
-                            chr((fmt.pixelformat >> 16) & 0xFF),
-                            chr((fmt.pixelformat >> 24) & 0xFF))
+                             chr((fmt.pixelformat >> 8) & 0xFF),
+                             chr((fmt.pixelformat >> 16) & 0xFF),
+                             chr((fmt.pixelformat >> 24) & 0xFF))
                         pixelformat['description'] = fmt.description.decode()
                         supported_formats.append(pixelformat)
                 fmt.index = fmt.index + 1
@@ -404,16 +421,18 @@
                                       framesize) == 0:
                         if framesize.type == V4L2_FRMSIZE_TYPE_DISCRETE:
                             resolutions.append([framesize.discrete.width,
-                                framesize.discrete.height])
+                                               framesize.discrete.height])
                         # for continuous and stepwise, let's just use min and
                         # max they use the same structure and only return
                         # one result
                         elif framesize.type == V4L2_FRMSIZE_TYPE_CONTINUOUS or\
-                             framesize.type == V4L2_FRMSIZE_TYPE_STEPWISE:
+                            framesize.type == V4L2_FRMSIZE_TYPE_STEPWISE:
                             resolutions.append([framesize.stepwise.min_width,
-                                framesize.stepwise.min_height])
+                                                framesize.stepwise.min_height]
+                                               )
                             resolutions.append([framesize.stepwise.max_width,
-                                framesize.stepwise.max_height])
+                                                framesize.stepwise.max_height]
+                                               )
                             break
                         framesize.index = framesize.index + 1
                 except IOError as e:
@@ -455,11 +474,11 @@
 
             if outw != width:
                 print("Image width does not match, was %s should be %s" %
-                    (outw, width), file=sys.stderr)
+                      (outw, width), file=sys.stderr)
                 return False
             if outh != height:
                 print("Image width does not match, was %s should be %s" %
-                    (outh, height), file=sys.stderr)
+                      (outh, height), file=sys.stderr)
                 return False
 
             return True
@@ -519,10 +538,21 @@
 if __name__ == "__main__":
     args = parse_arguments(sys.argv[1:])
 
+    if not args.test:
+        args.test = 'detect'
+
     # Import Gst only for the test cases that will need it
     if args.test in ['display', 'still', 'led', 'resolutions']:
         from gi.repository import Gst
+        if Gst.version()[0] > 0:
+            gst_plugin = 'videoconvert'
+            gst_video_type = 'video/x-raw'
+        else:
+            gst_plugin = 'ffmpegcolorspace'
+            gst_video_type = 'video/x-raw-yuv'
         Gst.init(None)
+        camera = CameraTest(args, gst_plugin, gst_video_type)
+    else:
+        camera = CameraTest(args)
 
-    camera = CameraTest(args)
     sys.exit(getattr(camera, args.test)())