fix crashes with video widths not multiple of 4 #577

ckirmse · 2020-10-13T18:38:56Z

need to align 32 everywhere for sws_scale to not crash
older ffmpegs don't have a nice function (thanks for telling me about it @ferdnyc , I didn't know about av_cpu_max_align), so 32 hard-coded in a few places
needed to tell QImage about the change too.
fixes issue #576

- need to align 32 everywhere for sws_scale to not crash

ferdnyc

Looks good, for the most part. I brought up a couple of points about AddImage(), but nothing critical or imperative.

My real concerns are about effects on the rest of the code, which is unfortunately not in a state where it's resilient enough to easily weather a change like this. We're probably going to have to shore up a bunch of bad algorithms elsewhere.

ferdnyc · 2020-10-14T13:57:48Z

include/Frame.h

@@ -156,7 +156,7 @@ namespace openshot
 		void AddColor(int new_width, int new_height, std::string new_color);

 		/// Add (or replace) pixel data to the frame
-		void AddImage(int new_width, int new_height, int bytes_per_pixel, QImage::Format type, const unsigned char *pixels_);
+		void AddImage(int new_width, int new_height, int bytes_per_pixel, int bytes_per_line, QImage::Format type, const unsigned char *pixels_);


Perhaps it'd be better to add this as an additional overload, rather than redefining the existing Frame::AddImage() semantics? There may be external API consumers who rely on the previous signature.

To make overload resolution slightly less harrowing, there's some argument for tacking bytes_per_line on at the very end. Arguably it's a detail specific to the buffer, not a parameter describing the image (like all of the other parameters surrounding it), so it should (optionally) follow the buffer it describes — and only in the cases where it's necessary to specify it. (IOW, only when bytes_per_line != width * bytes_per_pixel.)

A lot of good questions there, and I'm open to any solution. Here's my thoughts:

I don't consider the Frame class part of the externally exposed API of libopenshot, so since there are no other uses of this function, I felt safe changing it. If we consider the Frame class as something external callers can use, then I agree it should be a separate overload.

I see your point about bytes_per_line at the end; it doesn't matter to me, and I see the argument that it's a "property" of buffer.

ferdnyc · 2020-10-14T13:57:48Z

src/Frame.cpp

-void Frame::AddImage(int new_width, int new_height, int bytes_per_pixel, QImage::Format type, const unsigned char *pixels_)
+void Frame::AddImage(int new_width, int new_height, int bytes_per_pixel, int bytes_per_line, QImage::Format type, const unsigned char *pixels_)


The old signature could then just be turned into:

void Frame::AddImage( int width, int height, int bytes_per_pixel, QImage::Format type, const unsigned char* pixels_) { AddImage(width, height, bytes_per_pixel, type, pixels_, (width * bytes_per_pixel)); }

ferdnyc · 2020-10-14T13:57:48Z

src/Frame.cpp

 {
 	// Create new buffer
 	const GenericScopedLock<juce::CriticalSection> lock(addingImageSection);
-	int buffer_size = new_width * new_height * bytes_per_pixel;
+	int buffer_size = bytes_per_line * new_height;


Alternatively, if you are gonna change the signature of Frame::AddImage(), it makes sense to drop bytes_per_pixel (and replace it with bytes_per_line), since it no longer has any purpose.

(Plus it's totally implied by QImage::Format, so it was never necessary to pass it in anyway.)

ferdnyc · 2020-10-14T13:57:48Z

src/Frame.cpp

@@ -772,7 +772,7 @@ void Frame::AddImage(int new_width, int new_height, int bytes_per_pixel, QImage:
 	// Create new image object, and fill with pixel data
 	#pragma omp critical (AddImage)
 	{
-		image = std::shared_ptr<QImage>(new QImage(qbuffer, new_width, new_height, new_width * bytes_per_pixel, type, (QImageCleanupFunction) &openshot::Frame::cleanUpBuffer, (void*) qbuffer));
+		image = std::shared_ptr<QImage>(new QImage(qbuffer, new_width, new_height, bytes_per_line, type, (QImageCleanupFunction) &openshot::Frame::cleanUpBuffer, (void*) qbuffer));


If openshot::Frames are going to contain QImages with possible line-padding, I think this is actually going to necessitate an audit of all of our image-processing code. Because, for instance, I can't see how this wouldn't break this loop:

libopenshot/src/effects/Shift.cpp

Lines 79 to 84 in 414a2cd

// X-SHIFT

// Loop through rows

for (int row = 0; row < frame_image->height(); row++) {

// Copy current row's pixels

int starting_row_pixel = row * frame_image->width();

memcpy(temp_row, &pixels[starting_row_pixel * 4], sizeof(char) * frame_image->width() * 4);

And potentially dozens more just like it, hiding scattered around the code. We are unfortunately not rigorous about using QImage::[const]scanLine. Not at all.

Very good catch. Now, using QImage but NOT using bytesPerLine(), in my opinion, is very poor code hygiene and we should fix all of these cases, since we should not assume only certain types of QImages get here.

However, fixing all of those effects to use bytesPerLine() properly is costly. Here's another option: we could leave the input to AddFrame with this bytes_per_line, but we could copy row-by-row into the QImage without any extra padding, so the QImages remain "packed" and all those effects will work unchanged. This would be slightly lower performance (hundreds of row-by-row copies instead of one bit batch copy), but that may not be meaningful, and it wouldn't change assumptions out from underneath all our users of QImage.

As I said above, I'm open to any/all of these thoughts. Let me know what you think and I'll revise the pull request based on what we decide!

github-actions · 2020-11-19T13:49:04Z

Merge conflicts have been detected on this PR, please resolve.

fix crashes with video widths not multiple of 4

Loading status checks…

e658f74

- need to align 32 everywhere for sws_scale to not crash

ferdnyc reviewed Oct 14, 2020

View changes

github-actions bot added the conflicts label Nov 19, 2020

OpenShot / libopenshot

fix crashes with video widths not multiple of 4 #577

fix crashes with video widths not multiple of 4 #577

ckirmse commented Oct 13, 2020

ferdnyc left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 19, 2020

		void Frame::AddImage(int new_width, int new_height, int bytes_per_pixel, QImage::Format type, const unsigned char *pixels_)
		void Frame::AddImage(int new_width, int new_height, int bytes_per_pixel, int bytes_per_line, QImage::Format type, const unsigned char *pixels_)

	// X-SHIFT
	// Loop through rows
	for (int row = 0; row < frame_image->height(); row++) {
	// Copy current row's pixels
	int starting_row_pixel = row * frame_image->width();
	memcpy(temp_row, &pixels[starting_row_pixel * 4], sizeof(char) * frame_image->width() * 4);

OpenShot / libopenshot

fix crashes with video widths not multiple of 4 #577

fix crashes with video widths not multiple of 4 #577

Conversation

ckirmse commented Oct 13, 2020

ferdnyc left a comment

This comment has been minimized.

ferdnyc Oct 14, 2020 Collaborator

This comment has been minimized.

ckirmse Oct 14, 2020 Author

This comment has been minimized.

ferdnyc Oct 14, 2020 • edited Collaborator

This comment has been minimized.

ferdnyc Oct 14, 2020 Collaborator

This comment has been minimized.

ferdnyc Oct 14, 2020 Collaborator

This comment has been minimized.

ckirmse Oct 14, 2020 Author

github-actions bot commented Nov 19, 2020

ferdnyc Oct 14, 2020
Collaborator

ckirmse Oct 14, 2020
Author

ferdnyc Oct 14, 2020 •

edited

Collaborator

ferdnyc Oct 14, 2020
Collaborator

ferdnyc Oct 14, 2020
Collaborator

ckirmse Oct 14, 2020
Author