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.

Sign up for GitHub

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

Jump to bottom

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
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e658f74
fix crashes with video widths not multiple of 4
Oct 13, 2020
08f8497
Merge branch 'develop' into fix_alignment
ferdnyc Feb 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions include/FFmpegUtilities.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@
#define AV_SET_FILENAME(oc, f) oc->AV_FILENAME = av_strdup(f)
#define MY_INPUT_BUFFER_PADDING_SIZE AV_INPUT_BUFFER_PADDING_SIZE
#define AV_ALLOCATE_FRAME() av_frame_alloc()
#define AV_ALLOCATE_IMAGE(av_frame, pix_fmt, width, height) av_image_alloc(av_frame->data, av_frame->linesize, width, height, pix_fmt, 1)
#define AV_ALLOCATE_IMAGE(av_frame, pix_fmt, width, height) av_image_alloc(av_frame->data, av_frame->linesize, width, height, pix_fmt, av_cpu_max_align())
#define AV_RESET_FRAME(av_frame) av_frame_unref(av_frame)
#define AV_FREE_FRAME(av_frame) av_frame_free(av_frame)
#define AV_FREE_PACKET(av_packet) av_packet_unref(av_packet)
Expand All @@ -175,8 +175,8 @@
#define AV_GET_CODEC_ATTRIBUTES(av_stream, av_context) av_stream->codecpar
#define AV_GET_CODEC_PIXEL_FORMAT(av_stream, av_context) (AVPixelFormat) av_stream->codecpar->format
#define AV_GET_SAMPLE_FORMAT(av_stream, av_context) av_stream->codecpar->format
#define AV_GET_IMAGE_SIZE(pix_fmt, width, height) av_image_get_buffer_size(pix_fmt, width, height, 1)
#define AV_COPY_PICTURE_DATA(av_frame, buffer, pix_fmt, width, height) av_image_fill_arrays(av_frame->data, av_frame->linesize, buffer, pix_fmt, width, height, 1)
#define AV_GET_IMAGE_SIZE(pix_fmt, width, height) av_image_get_buffer_size(pix_fmt, width, height, av_cpu_max_align())
#define AV_COPY_PICTURE_DATA(av_frame, buffer, pix_fmt, width, height) av_image_fill_arrays(av_frame->data, av_frame->linesize, buffer, pix_fmt, width, height, av_cpu_max_align())
#define AV_OUTPUT_CONTEXT(output_context, path) avformat_alloc_output_context2( output_context, NULL, NULL, path)
#define AV_OPTION_FIND(priv_data, name) av_opt_find(priv_data, name, NULL, 0, 0)
#define AV_OPTION_SET( av_stream, priv_data, name, value, avcodec) av_opt_set(priv_data, name, value, 0); avcodec_parameters_from_context(av_stream->codecpar, avcodec);
Expand All @@ -194,7 +194,7 @@
#define AV_SET_FILENAME(oc, f) snprintf(oc->AV_FILENAME, sizeof(oc->AV_FILENAME), "%s", f)
#define MY_INPUT_BUFFER_PADDING_SIZE FF_INPUT_BUFFER_PADDING_SIZE
#define AV_ALLOCATE_FRAME() av_frame_alloc()
#define AV_ALLOCATE_IMAGE(av_frame, pix_fmt, width, height) av_image_alloc(av_frame->data, av_frame->linesize, width, height, pix_fmt, 1)
#define AV_ALLOCATE_IMAGE(av_frame, pix_fmt, width, height) av_image_alloc(av_frame->data, av_frame->linesize, width, height, pix_fmt, 32)
#define AV_RESET_FRAME(av_frame) av_frame_unref(av_frame)
#define AV_FREE_FRAME(av_frame) av_frame_free(av_frame)
#define AV_FREE_PACKET(av_packet) av_packet_unref(av_packet)
Expand All @@ -210,8 +210,8 @@
#define AV_GET_CODEC_ATTRIBUTES(av_stream, av_context) av_stream->codecpar
#define AV_GET_CODEC_PIXEL_FORMAT(av_stream, av_context) (AVPixelFormat) av_stream->codecpar->format
#define AV_GET_SAMPLE_FORMAT(av_stream, av_context) av_stream->codecpar->format
#define AV_GET_IMAGE_SIZE(pix_fmt, width, height) av_image_get_buffer_size(pix_fmt, width, height, 1)
#define AV_COPY_PICTURE_DATA(av_frame, buffer, pix_fmt, width, height) av_image_fill_arrays(av_frame->data, av_frame->linesize, buffer, pix_fmt, width, height, 1)
#define AV_GET_IMAGE_SIZE(pix_fmt, width, height) av_image_get_buffer_size(pix_fmt, width, height, 32)
#define AV_COPY_PICTURE_DATA(av_frame, buffer, pix_fmt, width, height) av_image_fill_arrays(av_frame->data, av_frame->linesize, buffer, pix_fmt, width, height, 32)
#define AV_OUTPUT_CONTEXT(output_context, path) avformat_alloc_output_context2( output_context, NULL, NULL, path)
#define AV_OPTION_FIND(priv_data, name) av_opt_find(priv_data, name, NULL, 0, 0)
#define AV_OPTION_SET( av_stream, priv_data, name, value, avcodec) av_opt_set(priv_data, name, value, 0); avcodec_parameters_from_context(av_stream->codecpar, avcodec);
Expand Down
2 changes: 1 addition & 1 deletion include/Frame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


/// Add (or replace) pixel data to the frame
void AddImage(std::shared_ptr<QImage> new_image);
Expand Down
4 changes: 2 additions & 2 deletions src/FFmpegReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1161,7 +1161,7 @@ bool FFmpegReader::GetAVFrame() {
// Use only the first frame like avcodec_decode_video2
if (frameFinished == 0 ) {
frameFinished = 1;
av_image_alloc(pFrame->data, pFrame->linesize, info.width, info.height, (AVPixelFormat)(pStream->codecpar->format), 1);
av_image_alloc(pFrame->data, pFrame->linesize, info.width, info.height, (AVPixelFormat)(pStream->codecpar->format), 32);
av_image_copy(pFrame->data, pFrame->linesize, (const uint8_t**)next_frame->data, next_frame->linesize,
(AVPixelFormat)(pStream->codecpar->format), info.width, info.height);
}
Expand Down Expand Up @@ -1372,7 +1372,7 @@ void FFmpegReader::ProcessVideoPacket(int64_t requested_frame) {
std::shared_ptr<Frame> f = CreateFrame(current_frame);

// Add Image data to frame
f->AddImage(width, height, 4, QImage::Format_RGBA8888, buffer);
f->AddImage(width, height, 4, pFrameRGB->linesize[0], QImage::Format_RGBA8888, buffer);

// Update working cache
working_cache.Add(f);
Expand Down
6 changes: 3 additions & 3 deletions src/Frame.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -759,11 +759,11 @@ void Frame::AddColor(int new_width, int new_height, std::string new_color)
}

// Add (or replace) pixel data to the frame
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 •
edited
Loading

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

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

qbuffer = new unsigned char[buffer_size]();

// Copy buffer data
Expand All @@ -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:

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.

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!


// Always convert to RGBA8888 (if different)
if (image->format() != QImage::Format_RGBA8888)
Expand Down