Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

ckirmse
Copy link

@ckirmse ckirmse commented Oct 13, 2020

  • 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
Copy link
Contributor

@ferdnyc ferdnyc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

include/Frame.h Outdated
@@ -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_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/Frame.cpp Outdated
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_)
Copy link
Contributor

@ferdnyc ferdnyc Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/Frame.cpp Outdated
{
// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/Frame.cpp Outdated
@@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

// 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 github-actions bot added the conflicts A PR with unresolved merge conflicts label Nov 19, 2020
@github-actions
Copy link

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

@jonoomph 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
@ferdnyc ferdnyc removed the conflicts A PR with unresolved merge conflicts label Feb 18, 2021
@codecov
Copy link

codecov bot commented Feb 18, 2021

Codecov Report

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

Impacted file tree graph

@@           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 github-actions bot added the conflicts A PR with unresolved merge conflicts label Feb 25, 2021
@github-actions
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflicts A PR with unresolved merge conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants