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

Crash inside sws_scale with certain inputs #576

Closed
ckirmse opened this issue Oct 13, 2020 · 4 comments
Closed

Crash inside sws_scale with certain inputs #576

ckirmse opened this issue Oct 13, 2020 · 4 comments
Labels
stale This issue has not had any activity in 90 days :(

Comments

@ckirmse
Copy link

ckirmse commented Oct 13, 2020

It's easily reproducible to get both invalid reads and writes (causing a crash; valgrind shows more detail of what's wrong) by using an input video whose width is not a multiple of 4. The underlying issue is that the alignment passed to a bunch of libav functions is 1, but for sws_scale to work in general it is apparently supposed to be 32 (the answer here helped me a lot: https://stackoverflow.com/questions/31253485/how-do-you-resize-an-avframe/31270501#31270501 ).

@ferdnyc this seemed up your alley--I have a rough patch that fixes the problem. Would you be interested in me polishing it up and submitting it? I'd love a second set of eyes because this is tricky stuff.

@ferdnyc
Copy link
Contributor

ferdnyc commented Oct 13, 2020

@ckirmse

@ferdnyc this seemed up your alley

Oh, my no. Well, not by choice, and not in the sense that I have any particular knowledge of that code — @eisneinechse and @jonoomph would be the go-tos/primaries. But in the sense that I'd like to see this fixed and can help turn the wheels on getting a PR merged, yes definitely.

--I have a rough patch that fixes the problem. Would you be interested in me polishing it up and submitting it? I'd love a second set of eyes because this is tricky stuff.

Absolutely! Heck, I'd be interested in you submitting it unpolished, or polishing it along the way. If it's truly in an un-finished state, just throw WIP: at the start of the PR title.

It's always easiest to discuss code changes in the context of the actual code, and the simplest path to that is an open PR. While it's possible to view branches in forked repos, comment on commits, and discuss in-development changes that way, it's just not as convenient. (For starters, it makes it much harder for any third parties to find or jump into those conversations.)

It's easily reproducible to get both invalid reads and writes (causing a crash; valgrind shows more detail of what's wrong) by using an input video whose width is not a multiple of 4. The underlying issue is that the alignment passed to a bunch of libav functions is 1, but for sws_scale to work in general it is apparently supposed to be 32 (the answer here helped me a lot: https://stackoverflow.com/questions/31253485/how-do-you-resize-an-avframe/31270501#31270501 ).

Mmm, I see where we have alignment just hardcoded to 1:

#define AV_ALLOCATE_IMAGE(av_frame, pix_fmt, width, height) av_image_alloc(av_frame->data, av_frame->linesize, width, height, pix_fmt, 1)

...in multiple places, actually:
#define AV_GET_IMAGE_SIZE(pix_fmt, width, height) av_image_get_buffer_size(pix_fmt, width, height, 1)

I suspect that was done because that's how all of the avpicture_foo() functions were defined, after they were deprecated in favor of av_image_foo(). So for example, in the current FFmpeg avpicture.c:

int avpicture_layout(const AVPicture* src, enum AVPixelFormat pix_fmt, int width, int height,
                     unsigned char *dest, int dest_size)
{
    return av_image_copy_to_buffer(dest, dest_size,
                                   (const uint8_t * const*)src->data, src->linesize,
                                   pix_fmt, width, height, 1);
}

int avpicture_get_size(enum AVPixelFormat pix_fmt, int width, int height)
{
    return av_image_get_buffer_size(pix_fmt, width, height, 1);
}

int avpicture_alloc(AVPicture *picture,
                    enum AVPixelFormat pix_fmt, int width, int height)
{
    int ret = av_image_alloc(picture->data, picture->linesize,
                             width, height, pix_fmt, 1);

...But I can also see where, in terms of the modern FFmpeg API, that's just wrong, and was only done out of necessity for the avpicture_ functions because they have no ability to align the data.

(That was even a documented limitation of the AVPicture API, until it was deprecated completely.):

int avpicture_fill ( AVPicture * picture,
                     const uint8_t * ptr,
                     enum AVPixelFormat pix_fmt,
                     int width,
                     int height
                   )

Fill in the AVPicture fields, always assume a linesize alignment of 1.
See Also

av_image_fill_arrays()

Definition at line 34 of file avpicture.c.

Referenced by copy_frame(), raw_decode(), sdl_write_packet(), and xv_write_packet().

By hardcoding 1, we're letting the legacy code cripple the newer APIs the same way. In fact, if we're going to do that we could just continue to call avpicture_alloc() — but then we'd rightly get a deprecation warning. Effectively, our current #defines hide the deprecation warning, but still implement deprecated, unaligned APIs.

@ferdnyc
Copy link
Contributor

ferdnyc commented Oct 13, 2020

So, yes, align should be set. But to what?

I'll assume that we want to continue to allocate image buffers ourselves, rather than take advantage of the FFmpeg helper functions to handle that sort of thing. (Perhaps those helpers don't DTRT with respect to the rest of our code paths, especially where older API compatibility has to be preserved — a valid reason to call av_image_alloc() ourselves, but not to call it with arbitrary and bad values for align.)

Rather than just switching to hardcoding 32 instead of 1, one obvious contender for the Right Way in any FFmpeg 3.4+ (where it first appears) is av_cpu_max_align():

size_t av_cpu_max_align ( void )

Get the maximum data alignment that may be required by FFmpeg.

Note that this is affected by the build configuration and the CPU flags mask, so e.g. if the CPU supports AVX, but libavutil has been built with –disable-avx or the AV_CPU_FLAG_AVX flag has been disabled through av_set_cpu_flags_mask(), then this function will behave as if AVX is not present.

Definition at line 317 of file cpu.c.

So,

#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_GET_IMAGE_SIZE(pix_fmt, width, height) \
	av_image_get_buffer_size(pix_fmt, width, height, av_cpu_max_align())

...would be far superior to our current #defines, at least for newest-FFmpeg. Those won't work for our FFmpeg 3.2 code (which also uses av_image_alloc() and av_image_get_buffer_size()), because 3.2 doesn't have an av_cpu_max_align().

Digression: FFmpeg compatibility

This marks yet another way that FFmpeg 3.2 is especially problematic for us. (It's already lacking in Travis tests, because it isn't part of any still-supported Ubuntu LTS release. Xenial uses FFmpeg 2.8, and Bionic jumps right to 3.4.)

It may make sense at some point to realign our various revision "lanes" along a split of FFmpeg 4.0+ / FFmpeg 3.4+ / FFmpeg 2.x (with FFmpeg 3.0 and 3.2 explicitly not supported), in contrast with our current split which consists of:

  • FFmpeg 4.0+ (LIBAVFORMAT_VERSION_MAJOR >= 58)
    • (with an additional wrinkle of HAVE_HWACCEL corresponding to FFmpeg 3.4+; that could then be rolled into an FFmpeg 3.4+ path.)
  • FFmpeg 3.2+ (IS_FFMPEG_3_2 aka LIBAVCODEC_VERSION_INT >= AV_VERSION_INT(57, 64, 101))
  • FFmpeg 2.0+ (LIBAVFORMAT_VERSION_MAJOR >= 55)
  • FFmpeg < 2.0

There's no question in my mind that at least the last one should be dropped regardless. My open PR #455 proposes to do just that, and would make FFmpeg 2.4 our minimum compatible version — though I'd prefer to be even less conservative, and drop support for all FFmpeg < 3.4, if not < 4.0.

@ckirmse
Copy link
Author

ckirmse commented Oct 13, 2020

I don't personally see any value in supporting the old ffmpegs, and the pain associated with it is very large. I'd love to drop < 3.4 or even better, < 4.0

@stale
Copy link

stale bot commented Jan 11, 2021

Thank you so much for submitting an issue to help improve OpenShot Video Editor. We are sorry about this, but this particular issue has gone unnoticed for quite some time. To help keep the OpenShot GitHub Issue Tracker organized and focused, we must ensure that every issue is correctly labelled and triaged, to get the proper attention.

This issue will be closed, as it meets the following criteria:

  • No activity in the past 90 days
  • No one is assigned to this issue

We'd like to ask you to help us out and determine whether this issue should be reopened.

  • If this issue is reporting a bug, please can you attempt to reproduce on the latest daily build to help us to understand whether the bug still needs our attention.
  • If this issue is proposing a new feature, please can you verify whether the feature proposal is still relevant.

Thanks again for your help!

@stale stale bot added the stale This issue has not had any activity in 90 days :( label Jan 11, 2021
@stale stale bot closed this as completed Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale This issue has not had any activity in 90 days :(
Projects
None yet
Development

No branches or pull requests

2 participants