Project

General

Profile

Bug #10001

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

Added by intrigeri over 4 years ago. Updated about 4 years ago.

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

100%

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

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?


Related issues

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

Associated revisions

Revision e5be0187 (diff)
Added by anonym about 4 years ago

Use more efficient --capture video encoding.

Will-fix: #10001

Revision 1a749b0a
Added by intrigeri about 4 years ago

Merge remote-tracking branch 'origin/test/10148-capture-per-scenario-videos' into stable

Fix-committed: #10148, #10001

History

#1 Updated by intrigeri over 4 years ago

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

#2 Updated by intrigeri about 4 years ago

  • 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 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 about 4 years ago

  • 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 about 4 years ago

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

#5 Updated by anonym about 4 years ago

  • Assignee changed from anonym to intrigeri

intrigeri wrote:

anonym, kytv: this is apparently kinda blocking #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 about 4 years ago

  • 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 about 4 years ago

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 about 4 years ago

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 about 4 years ago

  • 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 about 4 years ago

intrigeri wrote:

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

Why do we care?

#11 Updated by anonym about 4 years ago

  • Assignee changed from intrigeri to anonym

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

#12 Updated by anonym about 4 years ago

  • Status changed from Confirmed to In Progress

#13 Updated by anonym about 4 years ago

  • 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 about 4 years ago

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 about 4 years ago

  • Assignee set to kytv

#16 Updated by intrigeri about 4 years ago

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 about 4 years ago

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 about 4 years ago

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 about 4 years ago

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 about 4 years ago

Code review looks good. Now the testing…

#21 Updated by kytv about 4 years ago

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

#22 Updated by kytv about 4 years ago

  • 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 #10001#note-19.

#23 Updated by anonym about 4 years ago

  • 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 #10001#note-19.

Done.

#24 Updated by kytv about 4 years ago

  • Assignee changed from kytv to intrigeri

Now happy with this branch. :)

#25 Updated by intrigeri about 4 years ago

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 about 4 years ago

  • 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 about 4 years ago

  • 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 about 4 years ago

  • Assignee changed from intrigeri to anonym

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

#29 Updated by intrigeri about 4 years ago

  • % 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 about 4 years ago

  • 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 about 4 years ago

  • Assignee changed from intrigeri to anonym

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

#32 Updated by anonym about 4 years ago

  • Assignee changed from anonym to intrigeri

intrigeri wrote:

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

Done.

#33 Updated by intrigeri about 4 years ago

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

#34 Updated by intrigeri about 4 years ago

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

Here we are.

#35 Updated by bertagaz about 4 years ago

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

Also available in: Atom PDF