WIP: 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 Crash inside sws_scale with certain inputs #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:40:08Z

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:45:29Z

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:48:25Z

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:55:35Z

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.

codecov · 2021-02-18T12:01:02Z

Codecov Report

Merging #577 (08f8497) into develop (dd2735e) will not change coverage.
The diff coverage is 75.00%.

@@           Coverage Diff            @@
##           develop     #577   +/-   ##
========================================
  Coverage    51.86%   51.86%           
========================================
  Files          151      151           
  Lines        12334    12334           
========================================
  Hits          6397     6397           
  Misses        5937     5937

Impacted Files	Coverage Δ
src/FFmpegUtilities.h	`100.00% <ø> (ø)`
src/Frame.h	`100.00% <ø> (ø)`
src/FFmpegReader.cpp	`69.06% <66.66%> (ø)`
src/Frame.cpp	`46.78% <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 dd2735e...08f8497. Read the comment docs.

github-actions · 2021-02-25T21:45:48Z

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

fix crashes with video widths not multiple of 4

e658f74

- need to align 32 everywhere for sws_scale to not crash

ferdnyc reviewed Oct 14, 2020

View reviewed changes

github-actions bot added the conflicts A PR with unresolved merge conflicts label Nov 19, 2020

jonoomph changed the title ~~fix crashes with video widths not multiple of 4~~ WIP: fix crashes with video widths not multiple of 4 Jan 28, 2021

Merge branch 'develop' into fix_alignment

08f8497

ferdnyc removed the conflicts A PR with unresolved merge conflicts label Feb 18, 2021

github-actions bot added the conflicts A PR with unresolved merge conflicts label Feb 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: fix crashes with video widths not multiple of 4 #577

WIP: fix crashes with video widths not multiple of 4 #577

ckirmse commented Oct 13, 2020

ferdnyc left a comment

ferdnyc Oct 14, 2020

ckirmse Oct 14, 2020

ferdnyc Oct 14, 2020 •

edited

ferdnyc Oct 14, 2020

ferdnyc Oct 14, 2020

ckirmse Oct 14, 2020

github-actions bot commented Nov 19, 2020

codecov bot commented Feb 18, 2021 •

edited

github-actions bot commented Feb 25, 2021

		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);

WIP: fix crashes with video widths not multiple of 4 #577

Are you sure you want to change the base?

WIP: fix crashes with video widths not multiple of 4 #577

Conversation

ckirmse commented Oct 13, 2020

ferdnyc left a comment

Choose a reason for hiding this comment

ferdnyc Oct 14, 2020

Choose a reason for hiding this comment

ckirmse Oct 14, 2020

Choose a reason for hiding this comment

ferdnyc Oct 14, 2020 • edited

Choose a reason for hiding this comment

ferdnyc Oct 14, 2020

Choose a reason for hiding this comment

ferdnyc Oct 14, 2020

Choose a reason for hiding this comment

ckirmse Oct 14, 2020

Choose a reason for hiding this comment

github-actions bot commented Nov 19, 2020

codecov bot commented Feb 18, 2021 • edited

Codecov Report

github-actions bot commented Feb 25, 2021

ferdnyc Oct 14, 2020 •

edited

codecov bot commented Feb 18, 2021 •

edited