Enhanced transcoding integration tests with ffprobe report

[cameel]

This pull request contains major part of my and @pawelkisielewicz's work on adding detailed ffprobe metadata comparisons to transcoding integration tests. It has been rebased on the latest CGI/transcoding/master and updated to take into account changes from the master branch and to work with the current ffmpeg_tools master.

This is the first part. It's meant to work with the current transcoding method - without changing the split&merge method or adding support for new formats. It contains the bulk of the implementation but due to the limitations of the current method it does not test everything it could. That will come in subsequent, smaller pull requests.

What's included

1. ffprobe_report.py (+unit tests): classes that wrap JSON data obtained from ffprobe and define an interface for accessing the attributes. This is an intermediate layer that abstracts out low level details like: deciding which field to use (sometimes there's more than one with similar information), how to parse and compare it it, which fields are actually of interest to us, etc.
2. ffprobe_report_set.py (+unit tests): a helper class that gathers a set of ffprobe report diffs from the executed test cases and can print a summary in a table at the end. It's hard to get an overview of what works and what does not for a big set of video files otherwise. The test output is too verbose to be useful when 1000 tests fail.
3. simulated_transcoding_operation.py (+unit tests): a class that started out as a set of private helper methods in test_ffmpegintegration.py but grew enough to warrant creating a separate class. Its main purpose is to keep stuff out of the tests themselves to make them shorter and more readable. It allows us to concisely define how the file should be transcoded, define what to ignore when comparing results and run the transcoding operation.
4. New integration tests for the following cases:
   • codec/container change
   • resolution change
   • frame rate change
   • splitting into various numbers of segments

What is not included

1. Testing on the big, varied set of video files that we prepared. The current tests run only on test_video.mp4 and test_video2.mp4 included in the repo. My work on getting the tests to run on the whole set is nearing completion but there are still issues I need to deal with and I did not want to add failing tests to the pull request. I'm going to be adding files from that set in the subsequent pull requests.
2. Ignoring/handling the attribute differences in the failing tests that we agreed upon. Some of the smaller ones are already here but the rest was on top of the rest of my branches and getting them to work with the new master is what I'm currently working on. They'll be coming as separate pull requests for ease of review.
3. The new split&merge method with audio separation. I still have to update that code to work on the current master. And the changes have to go to two separate repositories (golem and ffmpeg_tools). I've decided that it will be easier to first get the tests in. This way you'll be able to see that it does not break stuff that used to work and I'll be able to submit the rest in smaller, more manageable pieces.
4. Support for a wider range of codecs/contrainers. That will come with the new split&merge implementation.

Stuff you may want to take a closer look at when reviewing

1. The range of currently supported codec/container combinations is very limited so only conversion to H.264/AVI is being tested. I have more but I'm going to add them along with the new split&merge implementation.
2. The current resolution validations don't allow any conversion if the input file does not have one of the predefined sizes (even if the conversion would retain the aspect ratio). So for example test_video.mp4 is 320x240 and you can't convert it to 640x480. I'm not sure it that's something that's going to be fixed or a deliberate decision. Anyway, that's the reason why I'm testing conversion from 320x240 to 320x240 :) No other sizes would work with that file. Even though it does not really change the size, it does run ffmpeg with the scaling filter so it is testing something. I'll be able to add more resolutions when I add more test files or when this validation rule is relaxed.
3. The tests are ignoring video bitrate when comparing results. Turns out that even if you give ffmpeg your desired bitrate with -b:v, the bitrate of the resulting file can vary wildly. I'll expand on that in a separate pull request in ffmpeg_tools for a feature that implements preserving bitrate. Here I just wanted to let you know that I disabled these comparisons. It's done explicitly with operation.exclude_from_diff({'video': {'bitrate'}}) in the test and is even in a separate commit to make it hard to miss.
4. ffprobe reports wrong frame count in test_video.mp4 transcoded to H.264/AVI using your current ffmpeg_tools. The input file has 50 frames and so does the output file (I checked with -show_frames) but the nb_frames field reported by ffprobe says 100. Also r_frame_rate changes from 25 to 50 in the output file even though the conversion does not specify frame rate. avg_frame_rate, on the other hand, remains unchanged at 25. This is something I did not expect and we'll have to decide what to do about it but the bottom line is that the frame count read from nb_frames is not reliable. I'm ignoring the FfprobeReport.frame_count in the tests.
5. We have implemented StreamOperator.get_metadata() because there used to be no other way to run the compute-metrics operation from the ffmpeg image. I see that split operation now returns the metadata but that only works for the input file. For the transcoded file we have to get the metadata separately.
   • You may want to take a look at intermediate files/directories produced by StreamOperator.get_metadata() and FfprobeReport.build(). I'm not sure it this fits well into the directory structure in the image.
6. FfprobeReportSet is not enabled because it prints a big markdown table that is only relevant to you if you're working on transcoding. And the table only looks remotely readable if you render it as markdown (too much information to make it readable in the terminal unfortunately). Currently the lines that enable it are simply commented out in test_ffmpegintegration test case setUp() and you can uncomment it if you want to see the result. Is this acceptable? Or should we have some switch/setting for it? Or maybe just print it to a file and add it to .gitignore just like it's done with the code coverage report?
7. I have kept existing integration test cases mostly intact (apart from extracting the task definition into a single place). Do you want it left that way or should they also be rewritten using the new classes?

[nieznanysprawiciel]

I would separate these tests for extended video testset to other file.
But in this PR don't move anything. These tests you wrote until now, could be in this basic test set on CI.

Probably test_case_simple checks things, that your tests check too. Maybe it can be removed entirely.

[cameel]

I would separate these tests for extended video testset to other file.
But in this PR don't move anything. These tests you wrote until now, could be in this basic test set on CI.

OK. So just so that I'm sure I understood this correctly:

• In this pull request:
  • Don't move any tests outside of test_ffmpegintegration.py or even inside the file.
  • Creating a separate class without moving things around is OK.
• In subsequent pull requests:
  • Leave the current tests for codec/resolution/frame rate change in test_ffmpegintegration.py and have them test only test_video.mp4 and test_video2.mp4. Keep them there so that they can run on CI.
  • Create a separate module and copy the current tests for codec/resolution/frame rate change tests there. The files from the extended video set should be added to this module.

Is that correct?

Probably test_case_simple checks things, that your tests check too. Maybe it can be removed entirely.

Well, the new tests don't cover what it does yet (conversion to H.265; they convert to H.264) but will when I extend them.

Actually, I think I'd keep this test as is. Since it's so simple it might be much easier to debug than the other ones when things break.

[cameel]

@pawelkisielewicz To summarize, here's a final list of fixes to do. Look at the individual threads for details.

1. Saving FfprobeReportSet to a file in the test directory
2. Hard-coding important fixture values in tests instead of verifying them with assert
3. Constants for reason field in the diff
4. Move FfprobeReport, FfprobeReportSet and SimulatedTranscodingOperation and their tests to a separate module
5. Better names for modified/original in FfprobeReport diff methods
6. Copy the new integration tests to a separate directory but keep them also in test_ffmpegintegration for CI (later, in a separate pull request)

[nieznanysprawiciel]

* Creating a separate class without moving things around is OK.

I don't see reason to separate class.
Rest is correct.

• Move FfprobeReport, FfprobeReportSet and SimulatedTranscodingOperation and their tests to a separate module

I meant directory.

[cameel]

I don't see reason to separate class.

OK. I thought it might be useful to separate the tests into two groups. But I guess just copying them to another module achieves the same thing.

I meant directory.

Right. My bad. Fixed.

[cameel]

I've just pushed 5 commits with review fixes from @pawelkisielewicz that address points 1-5 from the above.

In addition to that there are 7 new commits with a few extra tweaks that make more sense here than in a separate pull request. I strongly recommend just looking at the individual commits. They're small and have descriptions explaining what they do.