Change frame rate detection

[SuslikV]

FFmpeg re-encoding algorithm don't uses avg_frame_rate.

[ferdnyc]

I'm good with this, if it produces more accurate results. @eisneinechse ?

One code-style change requested, along with a suggestion for changes to the logging.

[codecov-io]

Codecov Report

Merging #448 into develop will increase coverage by 5.86%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #448      +/-   ##
===========================================
+ Coverage    42.37%   48.24%   +5.86%     
===========================================
  Files          129      128       -1     
  Lines        13276     9962    -3314     
===========================================
- Hits          5626     4806     -820     
+ Misses        7650     5156    -2494     

Impacted Files	Coverage Δ
src/FFmpegReader.cpp	68.39% <100.00%> (+0.59%)	⬆️
tests/FFmpegReader_Tests.cpp	100.00% <100.00%> (ø)
src/effects/Blur.cpp	63.63% <0.00%> (-8.78%)	⬇️
src/Timeline.cpp	38.52% <0.00%> (-2.88%)	⬇️
src/Frame.cpp	46.43% <0.00%> (-0.22%)	⬇️
src/Profiles.cpp	0.00% <0.00%> (ø)
include/Timeline.h	68.75% <0.00%> (ø)
src/ReaderBase.cpp	16.57% <0.00%> (ø)
src/TextReader.cpp	0.00% <0.00%> (ø)
src/ChunkReader.cpp	0.00% <0.00%> (ø)
... and 38 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 689f1e1...2701cf9. Read the comment docs.

[SuslikV]

I hope, now it looks a bit better. Updated on top of the current develop branch.

[ferdnyc]

This reminds me that I tried to add operators and conversion methods into openshot::Fraction so it would automatically convert from/to AVRational as needed, once. But I think the fact that AVRational is implemented in C instead of C++ probably doomed that idea right from the start.

[ferdnyc]

Are there unit tests that do any checks on frame-rate detection at all? I don't think so... in fact the only frame rate check I see is this one, which doesn't involve FFmpegReader at all.

libopenshot/tests/Timeline_Tests.cpp

Lines 82 to 90 in c38c55f

	TEST(Timeline_Framerate)
	{
	// Create a default fraction (should be 1/1)
	Fraction fps(24,1);
	Timeline t1(640, 480, fps, 44100, 2, LAYOUT_STEREO);
	// Check values
	CHECK_CLOSE(24.0f, t1.info.fps.ToFloat(), 0.00001);
	}

Oh, no, there is one in FFmpegWriter that checks the rate of a newly-written file, after reading it back in.

[ferdnyc]

I added an additional frame rate check in the FFmpegReader_Tests suite, using the standard sintel video source. (Which is uninteresting at 24fps, but like I said it's a sanity check.) With that in place, I'll merge this once Travis is done with it. I was hoping to get @eisneinechse 's take, but it's been a month and I don't want to hold this up forever.

[SuslikV]

All these unit tests is based on libopenshot own implementations, so they exist here just for fun. This is my own thoughts about these kind of stuff in libopenshot. With the broken instrument you'll be unable to make right measurements (it is more like freezing bugs at some level for the current build ^_^).

[ferdnyc]

All these unit tests is based on libopenshot own implementations, so they exist here just for fun.

Well, no — it's exactly what unit tests are. They check that the implementation matches expectations, in isolation. And, just as importantly (and the reason they're run on every build), they catch when changes to the code alter the implementation in unexpected ways.

The Android developer intro sums it up pretty well:

Unit tests are the fundamental tests in your app testing strategy. By creating and running unit tests against your code, you can easily verify that the logic of individual units is correct. Running unit tests after every build helps you to quickly catch and fix software regressions introduced by code changes to your app.

A unit test generally exercises the functionality of the smallest possible unit of code (which could be a method, class, or component) in a repeatable way. You should build unit tests when you need to verify the logic of specific code in your app. For example, if you are unit testing a class, your test might check that the class is in the right state. Typically, the unit of code is tested in isolation; your test affects and monitors changes to that unit only.

Unit tests don't test whether your structure is put together correctly, only that the logical pieces it's assembled from are really the shape we think they are.

Testing how it all fits together is where integration testing comes into play. But integration testing is pointless when you're assembling untested pieces.