Bug #10001

Consider using less CPU-hungry codec for the test suite's --capture option

Added by intrigeri 2015-08-14 05:37:56 . Updated 2015-09-22 15:03:35 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Test suite
Target version:
Tails_1.6
Start date:
2015-08-14
Due date:
% Done:

100%

Feature Branch:
test/10148-capture-per-scenario-videos
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:

Description

Switching the video codec from libvpx to mpeg4 made features/evince.feature:12 run 18% faster on my system. The resulting video file was 3 times bigger. In some situations (e.g. local debugging), one would prefer to save test suite runtime. In some others (e.g. making Jenkins artifacts available to download) one would prefer to save bandwidth and download time. Perhaps we should make this configurable?

Subtasks

Related issues

Related to Tails - Feature #8667: Lead a discussion to specify what automatically build ISOs needs to be tested, when and how Resolved 2015-01-10 2015-07-15
Blocked by Tails - Feature #10148: Split test suite capture videos per-scenario Resolved 2015-09-02

History

#1 Updated by intrigeri 2015-08-14 05:39:27

Hold on: with mpeg4 the output file has pretty crappy video quality.

#2 Updated by intrigeri 2015-08-18 06:18:19

  • Assignee set to anonym
  • Target version set to Tails_1.6
  • % Done changed from 0 to 10
  • QA Check set to Info Needed

On current Debian sid, feature/evince:12 at commit:b6ce091:

  • -r 15 -c:v libvpx (baseline): 107.9 seconds, 857 kB, quality OK
  • -r 15 -c:v libx264 -qp 0 -preset ultrafast: 86.6 seconds, 849 kB, quality OK
  • -r 15 -c:v libx264: 89.7 seconds, 324 kB, quality OK
  • -r 30 -c:v libvpx (current): 110.8 seconds, 607 kB, quality OK
  • -r 30 -c:v libx264 -qp 0 -preset ultrafast: 87.1 seconds, 1.1 MB, quality OK
  • -r 30 -c:v libx264: 92.4 seconds, 415 kB, quality OK

So I propose to at least switch to libx264 (with default settings: the runtime benefit from the ultrafast settings is not worth the size increase): we’ll get a substantial runtime benefit and much smaller files. anonym, if you agree and can reproduce baseline vs. -r 15 -c:v libx264 on Jessie, I’ll prepare a branch.

And then (perhaps later?) consider increasing the framerate we force on the input stream.

#3 Updated by intrigeri 2015-09-01 03:25:10

  • related to Feature #8667: Lead a discussion to specify what automatically build ISOs needs to be tested, when and how added

#4 Updated by intrigeri 2015-09-01 03:30:50

anonym, kytv: this is apparently kinda blocking Feature #8667, can one of you please give it a quick try?

#5 Updated by anonym 2015-09-01 07:20:08

  • Assignee changed from anonym to intrigeri

intrigeri wrote:
> anonym, kytv: this is apparently kinda blocking Feature #8667, can one of you please give it a quick try?

Sure. How do you get those stats? When I remove the IO redirection from the avconv call, I do not consistently get it.

#6 Updated by intrigeri 2015-09-01 08:31:59

  • Assignee changed from intrigeri to anonym

> Sure. How do you get those stats?

I applied this patch:

--- a/run_test_suite
+++ b/run_test_suite
@@ -162,7 +162,7 @@ capture_session() {
     check_dependencies libvpx1
     echo "Capturing guest display into ${CAPTURE_FILE}"
     avconv -f x11grab -s 1024x768 -r 15 -i ${TARGET_DISPLAY}.0 -an \
-        -vcodec libvpx -y "${CAPTURE_FILE}" >/dev/null 2>&1 &
+        -c:v libx264 -y "${CAPTURE_FILE}" >/dev/null 2>&1 &
 }

 remove_control_chars_from() {

… and run the test suite with --capture feature/evince:12.
The test suite gives me the runtime, and ls gives me the video file size.

> When I remove the IO redirection from the avconv call, I do not consistently get it.

I don’t understand what you mean here.

#7 Updated by anonym 2015-09-01 09:53:39

intrigeri wrote:
> > Sure. How do you get those stats?
>
> I applied this patch:
>
> […]
>
> … and run the test suite with --capture feature/evince:12.
> The test suite gives me the runtime, and ls gives me the video file size.

Ah, I thought you meant how much CPU time was used by the encoding process. Now I get the goal.

Question: did you re-run tests to get a higher confidence in the runtimes?

> > When I remove the IO redirection from the avconv call, I do not consistently get it.
>
> I don’t understand what you mean here.

I referred to removing the >/dev/null 2>&1 part to see the output of avconv. It reports the info too, and I thought it included the CPU time… nevermind about this.

#8 Updated by intrigeri 2015-09-01 10:23:14

> Question: did you re-run tests to get a higher confidence in the runtimes?

I don’t think so, but the system was not particularly loaded, the test I used was offline and thus pretty much deterministic.

#9 Updated by anonym 2015-09-02 04:20:25

  • Assignee changed from anonym to intrigeri
  • % Done changed from 10 to 20
  • QA Check changed from Info Needed to Dev Needed

I did these tests with the test/6094-improved-snapshots branch checked out since it represents the imminent (hopefully!) future of the test suite better. On a Debian Jessie host.

for x in 0 1 2 3 4; do
  ./run_test_suite \
     --log-to-file /tmp/no-video.$x.webm.log \
     --old-iso tails-i386-1.5/tails-i386-1.5.iso \
     --iso tails-i386-1.5.1/tails-i386-1.5.1.iso \
     -- features/evince.feature:7
done

1m26.056s
1m26.693s
1m25.562s
1m25.863s
1m25.496s
Average: 1m25.934s

for x in 0 1 2 3 4; do
  ./run_test_suite \
    --capture /tmp/video.orig.$x.webm \
    --log-to-file /tmp/video.orig.$x.webm.log \
    --old-iso tails-i386-1.5/tails-i386-1.5.iso \
    --iso tails-i386-1.5.1/tails-i386-1.5.1.iso \
    -- features/evince.feature:7
done

1m40.015s 940K
1m36.771s 887K
1m38.118s 939K
1m43.319s 935K
1m45.475s 925K
Average: 1m40.740s 925.2K

# I applied your patch
for x in 0 1 2 3 4; do
  ./run_test_suite \
    --capture /tmp/video.libx264.$x.mkv \
    --log-to-file /tmp/video.libx264.$x.mkv.log \
    --old-iso tails-i386-1.5/tails-i386-1.5.iso \
    --iso tails-i386-1.5.1/tails-i386-1.5.1.iso \
    -- features/evince.feature:7
done

1m31.036s 324K
1m30.191s 326K
1m30.692s 338K
1m32.117s 326K
1m30.681s 329K
Average: 1m30.943s 328.6K

So the new video compressions essentially cuts all costs from using --capture with a whooping 2/3 (also note that the increased runtime is 17% with the old compression and just 5.8% with the new). So this looks awesome to me, even though I didn’t check if it increased the CPU strain. Still, I say go for it!

#10 Updated by anonym 2015-09-02 04:20:58

intrigeri wrote:
> And then (perhaps later?) consider increasing the framerate we force on the input stream.

Why do we care?

#11 Updated by anonym 2015-09-02 04:54:47

  • Assignee changed from intrigeri to anonym

Actually I’m thinking about implementing Feature #10148 right away. Perhaps you should wait? In fact, I can do this.

#12 Updated by anonym 2015-09-02 05:58:43

  • Status changed from Confirmed to In Progress

Applied in changeset commit:e5be0187e76a14e098da84e3862f3f0681570a18.

#13 Updated by anonym 2015-09-02 06:00:03

  • Assignee deleted (anonym)
  • % Done changed from 20 to 50
  • QA Check changed from Dev Needed to Ready for QA
  • Feature Branch set to test/10148-capture-per-scenario-videos

Drive-by fixed this while I was working on the related ticket, like promised above.

#14 Updated by intrigeri 2015-09-02 07:17:20

> So the new video compressions essentially cuts all costs from using --capture with a whooping 2/3 (also note that the increased runtime is 17% with the old compression and just 5.8% with the new). So this looks awesome to me, even though I didn’t check if it increased the CPU strain. Still, I say go for it!

Woohoo!

#15 Updated by intrigeri 2015-09-02 07:17:41

  • Assignee set to kytv

#16 Updated by intrigeri 2015-09-02 07:19:17

>> And then (perhaps later?) consider increasing the framerate we force on the input stream.

> Why do we care?

I was under the impression that it might be needed to get a higher framerate in the output. Quite often, the current value gives me videos that are hard to go through picture by picture, and sometimes the exact thing I want to look at is impossible to see. I may be totally wrong wrt. what could be a solution to this problem, though :)

#17 Updated by anonym 2015-09-02 10:21:00

intrigeri wrote:
> Quite often, the current value gives me videos that are hard to go through picture by picture, and sometimes the exact thing I want to look at is impossible to see.

Got it. I’ve never experienced it myself. Any example that comes to mind?

#18 Updated by intrigeri 2015-09-03 00:14:57

> Got it. I’ve never experienced it myself. Any example that comes to mind?

I don’t remember any precisely right now, but e.g. sometimes a scenario ends as soon as Sikuli has found the image we want on screen, and it sometimes happens that looking for this particular video frame in the capture is hard, if not impossible. But perhaps a better fix would be to wait 1 more second before shutting down the system (no idea how it can be implemented without ugly hacks).

#19 Updated by anonym 2015-09-03 00:53:12

intrigeri wrote:
> > Got it. I’ve never experienced it myself. Any example that comes to mind?
>
> I don’t remember any precisely right now, but e.g. sometimes a scenario ends as soon as Sikuli has found the image we want on screen, and it sometimes happens that looking for this particular video frame in the capture is hard, if not impossible. But perhaps a better fix would be to wait 1 more second before shutting down the system (no idea how it can be implemented without ugly hacks).

Waiting a bit longer before ending the video capture seems like the proper solution, and the good news is that it’s a one-liner (excluding a well-needed comment):

--- a/features/support/hooks.rb
+++ b/features/support/hooks.rb
@@ -129,6 +129,7 @@ end
 # AfterScenario
 After('@product') do |scenario|
   if @video_capture_pid
+    sleep 1 if scenario.failed?
     Process.kill("INT", @video_capture_pid)
   end
   if scenario.failed?


However, I’m surprised that this is a problem. Most of the time when an error is related to the screen, Sikuli has been wait():ing for multiple seconds. It’s quite rare that we explicitly look for the error condition using Sikuli (with the remote shell it’s different, but that shouldn’t matter too much), which would trigger a sudden failure => scenario ends, like you describe.

#20 Updated by kytv 2015-09-04 08:26:45

Code review looks good. Now the testing…

#21 Updated by kytv 2015-09-05 03:30:37

  • blocked by Feature #10148: Split test suite capture videos per-scenario added

#22 Updated by kytv 2015-09-05 04:11:14

  • Assignee changed from kytv to anonym
  • QA Check changed from Ready for QA to Dev Needed

Code review & testing pass but it would probably be good to add the one-liner mentioned at Bug #10001#note-19.

#23 Updated by anonym 2015-09-08 04:23:23

  • Assignee changed from anonym to kytv
  • QA Check changed from Dev Needed to Ready for QA

kytv wrote:
> Code review & testing pass but it would probably be good to add the one-liner mentioned at Bug #10001#note-19.

Done.

#24 Updated by kytv 2015-09-14 15:05:53

  • Assignee changed from kytv to intrigeri

Now happy with this branch. :)

#25 Updated by intrigeri 2015-09-15 02:09:56

> However, I’m surprised that this is a problem. Most of the time when an error is related to the screen, Sikuli has been wait():ing for multiple seconds. It’s quite rare that we explicitly look for the error condition using Sikuli (with the remote shell it’s different, but that shouldn’t matter too much), which would trigger a sudden failure => scenario ends, like you describe.

I was describing a false negative: Sikuli finds a picture that makes it happy, but should not. Sorry for being unclear.

#26 Updated by intrigeri 2015-09-15 06:49:08

  • Assignee changed from intrigeri to anonym
  • QA Check changed from Ready for QA to Dev Needed
  • Type of work changed from Research to Code

This doesn’t merge cleanly into current stable => please resolve conflicts.

Regarding the dependency on libx264-142:

  • Sadly, it’s Jessie-specific so it won’t work for those of us who run something newer. I suggest depending on x264, that in turn depends on the version of that library that’s shipped in the current distro. It will probably pull some unneeded deps as well, but given the hardware specs for running the test suite, a few dozens MBs of .deb’s is no big deal.
  • I think that wiki/src/contribute/release_process/test/setup.mdwn needs to be updated (kytv: that’s the kind of things you want to look for when reviewing — thanks!).
  • Once this is done we’ll need to update the tails::tester Puppet class. I can do that once we agree on a strategy for the above.

#27 Updated by anonym 2015-09-15 07:50:09

  • Assignee changed from anonym to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> This doesn’t merge cleanly into current stable => please resolve conflicts.

Right. There’s been so much jenkins stuff going on, breaking builds, so I haven’t looked at the logs as much as I should.

> Regarding the dependency on libx264-142:
>
> * Sadly, it’s Jessie-specific so it won’t work for those of us who run something newer.

Right, my bad!

> I suggest depending on x264, that in turn depends on the version of that library that’s shipped in the current distro.

Fixed, thanks for the suggestion!

> It will probably pull some unneeded deps as well, but given the hardware specs for running the test suite, a few dozens MBs of .deb’s is no big deal.

Agreed. On Jessie I also got libffms2-3 libgpac3 at 4,512 kB for instance.

In general, I’m worrying that our dependencies will require more than the simple dependency check we have. Since we do not want to reinvent Debian’s dependency parser etc, perhaps we should solve this via creating an equivs package when that time comes? OTOH, then it indeed must be run as root. Any idea how to invoke just the dependency parser/checker?

> * I think that wiki/src/contribute/release_process/test/setup.mdwn needs to be updated (kytv: that’s the kind of things you want to look for when reviewing — thanks!).

Actually, we don’t add the optional dependencies there any more. Should we re-add the “suggests” section, or are the errors one gets when trying the option enough?

> * Once this is done we’ll need to update the tails::tester Puppet class. I can do that once we agree on a strategy for the above.

Go ahead!

#28 Updated by intrigeri 2015-09-15 08:06:07

  • Assignee changed from intrigeri to anonym

Code review passes, please reassign to me once the issues raised on Feature #10148 are solved too.

#29 Updated by intrigeri 2015-09-15 10:21:49

  • % Done changed from 50 to 70
  • QA Check changed from Ready for QA to Dev Needed

--capture-all is still broken on sid, see git grep libx264-142.

#30 Updated by anonym 2015-09-15 12:55:00

  • Assignee changed from anonym to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> --capture-all is still broken on sid, see git grep libx264-142.

Urgh… doing to many things at the same time => none is done properly. Sorry! Fixed now.

#31 Updated by intrigeri 2015-09-16 05:14:36

  • Assignee changed from intrigeri to anonym

Please reassign to me at the same time as Feature #10148.

#32 Updated by anonym 2015-09-16 12:15:47

  • Assignee changed from anonym to intrigeri

intrigeri wrote:
> Please reassign to me at the same time as Feature #10148.
Done.

#33 Updated by intrigeri 2015-09-16 13:46:39

  • Status changed from In Progress to Fix committed
  • % Done changed from 70 to 100

Applied in changeset commit:1a749b0aaedbf1694f0bf8480c00778c24eb83bf.

#34 Updated by intrigeri 2015-09-16 13:48:30

  • Assignee deleted (intrigeri)
  • QA Check changed from Ready for QA to Pass

Here we are.

#35 Updated by bertagaz 2015-09-22 15:03:35

  • Status changed from Fix committed to Resolved
  • QA Check deleted (Pass)