Frame: Fix interlaced AddImage

[ferdnyc]

Reading through the openshot::Frame code trying to debug the image-format switch, I came across the mess that was the current version of this method:

openshot::Frame::AddImage(QImage new_image, bool only_odd_lines) {
}

It's a good thing it's only called from one spot in the FrameMapper code, because holy crap is it broken! Over the years, changes piled up that just kept moving it further away from what it was supposed to be doing, and because there are no unit tests for interlaced video, they were never noticed. I'm not surprised our interlaced support is broken! I don't know that this will fix it, but it should at least un-break this particular method.

I had to go way, way back in the git history to find the ImageMagick version that it evolved from. Here's the original code, for comparison:

// Add (or replace) pixel data to the frame (for only the odd or even lines)
void Frame::AddImage(tr1::shared_ptr<Magick::Image> new_image, bool only_odd_lines)
{
	// Replace image (if needed)
	if (image->columns() == 1)
		image = new_image;

	// Loop through each odd or even line, and copy it to the image
	int starting_row = 0;
	if (!only_odd_lines)
		// even rows
		starting_row = 1;

	// Replace some rows of pixels
	for (int row = starting_row; row < new_image->rows(); row += 2)
	{
		// Get row of pixels from original image
		Magick::PixelPacket* row_pixels = image->getPixels(0, row, image->columns(), 1);
		const Magick::PixelPacket* new_row_pixels = new_image->getConstPixels(0, row, image->columns(), 1);

		// Copy pixels
		for (int col = 0; col < image->columns(); col++)
			row_pixels[col] = new_row_pixels[col];

		// Replace them with updated pixels
		image->syncPixels();
	}

}

I'll leave some inline comments below about the new version (prior to this PR), for comparison purposes.

[ferdnyc]

Ignoring the weird use of the ret boolean here, what's up with those conditions!?!?

if ( image->size() != image->size() )!? if ( image->format() != image->format() )!?

Clearly the first was supposed to be image->size() != new_image->size(), so I fixed it to that. The second was presumably supposed to be image->format() != new_image->format(), but since we can format-convert with QImage, instead of doing that test I convert new_image to the format of image if it doesn't match.

[ferdnyc]

Ignoring the weird use of the ret boolean here,

(Aha, it seems there's a reason for that, due to OpenMP's weird semantics.)

[ferdnyc]

consts are nice and all, when appropriate, but when you're modifying the pixels in the buffer I'm pretty sure we're not supposed to use constBits() since it suppresses Qt's copy-on-write handling for shared QImage buffers.

[ferdnyc]

This is where things really break.

The way that loop was previously written, it would go through every other row (odd or even) in new_pixels, and copy that row's data to the first row of pixels — every time! Nothing was ever offsetting the pixels location to advance it by the row stride, and nothing would have ever written to any part of pixels (and therefore image) beyond the first scanline/row.

[ferdnyc]

(Oh, yeah — the telltale cast on pixels — to cast away its const status, which it shouldn't have had in the first place — is also fun, in the previous code.)

[ferdnyc]

(Needless to say, this part of the code could really use some unit tests to ensure that my new version is actually correct. But it almost can't be any worse.)

[codecov-io]

Codecov Report

Merging #418 into develop will decrease coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop     #418      +/-   ##
===========================================
- Coverage    42.37%   42.37%   -0.01%     
===========================================
  Files          129      129              
  Lines        13276    13278       +2     
===========================================
  Hits          5626     5626              
- Misses        7650     7652       +2

Impacted Files	Coverage Δ
src/Frame.cpp	46.43% <0%> (-0.22%)	⬇️

---

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 49972b2...86bfa2f. Read the comment docs.

[jonoomph]

Good catch! LGTM. This method just disintegrated over time.