Adding some new functionality and documentation to DummyReader

[jonoomph]

Adding the ability to add test frames WriteFrame(), with fake image and audio data. This will can be used in unit-tests, and will soon be used to verify some new audio improvements (coming soon).

[jonoomph]

@musteresel Please take a look and let me know what you think.

[codecov]

Codecov Report

Merging #527 into develop will increase coverage by 0.50%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #527      +/-   ##
===========================================
+ Coverage    48.30%   48.81%   +0.50%     
===========================================
  Files          128      129       +1     
  Lines         9967    10042      +75     
===========================================
+ Hits          4815     4902      +87     
+ Misses        5152     5140      -12     

Impacted Files	Coverage Δ
include/DummyReader.h	0.00% <ø> (ø)
src/DummyReader.cpp	76.92% <100.00%> (+20.31%)	⬆️
src/Frame.cpp	47.70% <100.00%> (+1.16%)	⬆️
tests/DummyReader_Tests.cpp	100.00% <100.00%> (ø)

---

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 2834e77...8b12c1f. Read the comment docs.

[jonoomph]

This PR is loosely related to #447, and is related to some timeline audio merging improvements being worked on by @musteresel.

[ferdnyc]

Adding the ability to add test frames WriteFrame(), with fake image and audio data.

I have to say, I really hate that idea. Adding a write-API to the dummy reader? That's... I don't see the point of unit-testing something if it has to be fitted with special APIs that make no sense in the rest of the codebase. What is that even testing?

Let me propose a slightly less invasive approach. Instead of adding WriteFrame() to the DummyReader just so the tests can populate its cache, why not just give it a constructor that takes a CacheMemory object? The unit tests can create the cache, call testcache.Add(frame) to their heart's content, and then call new openshot::DummyReader(testcache) to wrap a Reader around the completed cache. That at least makes it work roughly the same way any other reader works, where it gets wrapped around some data (whether it's a file path, HTML source, etc...) in its constructor.

That way, the "special" API is limited to a constructor overload, and the rest of it is just a normal reader.

[jonoomph]

@ferdnyc 👍 I like that as well. Let me tweak things real quick.

[jonoomph]

We are going to be generating Frame objects with artificial audio data, such as an always incrementing numbers, and then build unit-tests which pass these frames through the Timeline with different FPS and sample rates. The idea is to verify audio data being truncated in the Timeline when combining Frames of different amounts of audio samples, and then correct the behavior once we can verify the failures. Hope that makes sense, haha.

[ferdnyc]

Like I said in the inline comments, this part suddenly makes me nervous:

libopenshot/src/Frame.cpp

Lines 737 to 756 in 7831cfe

	// Add (or replace) pixel data to the frame (based on a solid color)
	void Frame::AddColor(int new_width, int new_height, std::string new_color)
	{
	// Set color
	color = new_color;
	// Create new image object, and fill with pixel data
	const GenericScopedLock<juce::CriticalSection> lock(addingImageSection);
	#pragma omp critical (AddImage)
	{
	image = std::shared_ptr<QImage>(new QImage(new_width, new_height, QImage::Format_RGBA8888));
	// Fill with solid color
	image->fill(QColor(QString::fromStdString(color)));
	}
	// Update height and width
	width = image->width();
	height = image->height();
	has_image_data = true;
	}

If calling GetPixelData() can trigger an AddColor(), then theoretically it could be called from two threads at the same time. There's no reason to mistrust a getter that returns const, normally. But that color = new_color; bit is outside all of the thread synchronization. Yeah, it's going to be the same string, but because we pass everything by value the code doesn't actually KNOW that, and it's going to copy it over again anyway, potentially colliding on the writes.

[musteresel]

Just adding a quick note: This works great so far :) Using the constructor is also definitively the saner way to pass the artificial frames to the reader.

Regarding the modification made in Frame: I, too, think that this is a bad idea from a conceptional standpoint but at least now GetPixels and GetPixels(int) have the same (conceptionally bad) behavior; and as discussed fixing this will be a bigger thing :)