Page MenuHome

FFmpeg: Add regression tests
ClosedPublic

Authored by Sergey Sharybin (sergey) on Mar 5 2018, 3:34 PM.

Details

Summary

Currently only covering handful of files from reports about wrong fps detected.

It will need D3083 applied first to get tests passed, also tests themselves
are to be committed to svn.

But there are some python code which needs to be reviewed, like blendfile
passed to run_blender().

Diff Detail

Repository
rB Blender

Event Timeline

Hopefully less crappy way to allow empty file

Overall LGTM (we already talked about the optional blendfile arg on IRC).

tests/python/ffmpeg_tests.py
59

don’t understand why you use collections.deque here? why not directly iterate over output.split() result?
Actually, better to just use splitlines(), that way you don’t have to do replacement above either ;)

This revision is now accepted and ready to land.Mar 5 2018, 4:12 PM

Remove dequeue and EOL replacement

Was a copy-paste residue from alembic_tests.py.

Looks good, just a few style issues to nag about.

tests/python/ffmpeg_tests.py
38

unused import

45

Use PEP 8 for naming (yes this is inconsistent even in Python stdlib, but that's no excuse for us), so get_script_for_file or just script_for_file.

You may also want to declare parameter type (filename: pathlib.Path) and return type -> str. Since using pathlib is relatively new in Blender's Python code, I would at least do the former.

51

Replace '%s' with %r. This will use single quotes, unless there is a single quote in the string, then it'll use double quotes. If both single and double quotes are there, it'll also escape the filename properly.

Alternatively, do an assert "'" not in str(movie), 'Filename should not contain a single quote: %r' % movie before the return statement.

52

Since this'll probably just be in master, you can use the new format string syntax introduced in Python 3.6:

print(f'fps:{strip.fps}'))

60

Why not simply use return round(fps, 2)?

tests/python/modules/test_utils.py
80

Using a list instead of a tuple will allow you to do command.extend(...) or command.append(...) without requiring the existing tuple to be copied. Doesn't matter much, though.

86–91

You may want to leave the trailing comma in place. It allows for clean diffs (now and in the future).

Sergey Sharybin (sergey) marked 7 inline comments as done.Mar 6 2018, 11:03 AM
Sergey Sharybin (sergey) added inline comments.
tests/python/ffmpeg_tests.py
51

You'll want to look into alembic_tests.py for that.

52

PHP-like string formatting, yay! Finally!

Sergey Sharybin (sergey) marked 2 inline comments as done.

Address comments from Sybren

LGTM. Just found one thing that could be improved, but use your own judgement in that.

tests/python/ffmpeg_tests.py
64

New insight: maybe trade the round(fps, 2) call above for using self.assertAlmostEqual() here? That way you are also certain that there won't be any nasty decimal-to-binary errors creeping in and causing false failures.

Use assertAlmostEqual().

Think i indeed like this idea more.

This revision was automatically updated to reflect the committed changes.