From b7076549a393f3e1e87dfc750b40246fd21e08d7 Mon Sep 17 00:00:00 2001 From: Gleb Mazovetskiy Date: Sun, 24 Jan 2021 06:11:44 +0000 Subject: [PATCH 1/5] osg-ffmpeg-videoplayer: Fix crash on ARM osg-ffmpeg-videoplayer handled frame allocation incorrectly. It used a `vector` as its buffer, meaning the addresses could did not respect alignment. Instead, changes it to use `AVFrame` as buffers, allocated via `av_image_alloc`. We also now only allocate the buffer once, instead of on every frame, which should improve the framerate of videos. Fixes the following crash on startup on ARM: > Invalid address alignment (signal 7) Fixes #5807 --- extern/osg-ffmpeg-videoplayer/videostate.cpp | 45 ++++++++++++++------ extern/osg-ffmpeg-videoplayer/videostate.hpp | 3 +- 2 files changed, 33 insertions(+), 15 deletions(-) diff --git a/extern/osg-ffmpeg-videoplayer/videostate.cpp b/extern/osg-ffmpeg-videoplayer/videostate.cpp index 5858d985a..cdfdc8ad5 100644 --- a/extern/osg-ffmpeg-videoplayer/videostate.cpp +++ b/extern/osg-ffmpeg-videoplayer/videostate.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -49,7 +50,7 @@ VideoState::VideoState() , av_sync_type(AV_SYNC_DEFAULT) , audio_st(nullptr) , video_st(nullptr), frame_last_pts(0.0) - , video_clock(0.0), sws_context(nullptr), rgbaFrame(nullptr), pictq_size(0) + , video_clock(0.0), sws_context(nullptr), pictq_size(0) , pictq_rindex(0), pictq_windex(0) , mSeekRequested(false) , mSeekPos(0) @@ -220,7 +221,7 @@ void VideoState::video_display(VideoPicture *vp) osg::ref_ptr image = new osg::Image; image->setImage(this->video_ctx->width, this->video_ctx->height, - 1, GL_RGBA, GL_RGBA, GL_UNSIGNED_BYTE, &vp->data[0], osg::Image::NO_DELETE); + 1, GL_RGBA, GL_RGBA, GL_UNSIGNED_BYTE, vp->rgbaFrame->data[0], osg::Image::NO_DELETE); mTexture->setImage(image); } @@ -308,11 +309,8 @@ int VideoState::queue_picture(AVFrame *pFrame, double pts) } vp->pts = pts; - vp->data.resize(this->video_ctx->width * this->video_ctx->height * 4); - - uint8_t *dst[4] = { &vp->data[0], nullptr, nullptr, nullptr }; sws_scale(this->sws_context, pFrame->data, pFrame->linesize, - 0, this->video_ctx->height, dst, this->rgbaFrame->linesize); + 0, this->video_ctx->height, vp->rgbaFrame->data, vp->rgbaFrame->linesize); // now we inform our display thread that we have a pic ready this->pictq_windex = (this->pictq_windex+1) % VIDEO_PICTURE_ARRAY_SIZE; @@ -364,9 +362,6 @@ public: pFrame = av_frame_alloc(); - self->rgbaFrame = av_frame_alloc(); - av_image_alloc(self->rgbaFrame->data, self->rgbaFrame->linesize, self->video_ctx->width, self->video_ctx->height, AV_PIX_FMT_RGBA, 1); - while(self->videoq.get(packet, self) >= 0) { if(packet->data == flush_pkt.data) @@ -407,10 +402,7 @@ public: av_packet_unref(packet); - av_free(pFrame); - - av_freep(&self->rgbaFrame->data[0]); - av_free(self->rgbaFrame); + av_frame_free(&pFrame); } private: @@ -630,6 +622,25 @@ int VideoState::stream_open(int stream_index, AVFormatContext *pFormatCtx) return -1; } + // Allocate RGBA frame queue. + for (std::size_t i = 0; i < VIDEO_PICTURE_ARRAY_SIZE; ++i) { + AVFrame *frame = av_frame_alloc(); + if (frame == nullptr) { + std::cerr << "av_frame_alloc failed" << std::endl; + return -1; + } + + constexpr AVPixelFormat kPixFmt = AV_PIX_FMT_RGBA; + frame->format = kPixFmt; + frame->width = this->video_ctx->width; + frame->height = this->video_ctx->height; + if (av_image_alloc(frame->data, frame->linesize, frame->width, frame->height, kPixFmt, 1) < 0) { + std::cerr << "av_image_alloc failed" << std::endl; + return -1; + } + this->pictq[i].rgbaFrame = frame; + } + this->video_thread.reset(new VideoThread(this)); break; @@ -771,6 +782,14 @@ void VideoState::deinit() mTexture->setImage(nullptr); mTexture = nullptr; } + + // Dellocate RGBA frame queue. + for (std::size_t i = 0; i < VIDEO_PICTURE_ARRAY_SIZE; ++i) { + if (this->pictq[i].rgbaFrame == nullptr) continue; + av_freep(&this->pictq[i].rgbaFrame->data[0]); + av_frame_free(&this->pictq[i].rgbaFrame); + } + } double VideoState::get_external_clock() diff --git a/extern/osg-ffmpeg-videoplayer/videostate.hpp b/extern/osg-ffmpeg-videoplayer/videostate.hpp index 641fce04b..ed226bcf2 100644 --- a/extern/osg-ffmpeg-videoplayer/videostate.hpp +++ b/extern/osg-ffmpeg-videoplayer/videostate.hpp @@ -95,7 +95,7 @@ struct VideoPicture { VideoPicture() : pts(0.0) { } - std::vector data; + AVFrame* rgbaFrame = nullptr; double pts; }; @@ -160,7 +160,6 @@ struct VideoState { PacketQueue videoq; SwsContext* sws_context; VideoPicture pictq[VIDEO_PICTURE_ARRAY_SIZE]; - AVFrame* rgbaFrame; // used as buffer for the frame converted from its native format to RGBA int pictq_size, pictq_rindex, pictq_windex; std::mutex pictq_mutex; std::condition_variable pictq_cond; From eb93fdfbea6fc1c7a96c7b734a02fe2b7fb51464 Mon Sep 17 00:00:00 2001 From: Gleb Mazovetskiy Date: Sun, 7 Mar 2021 03:57:54 +0000 Subject: [PATCH 2/5] Use unique_ptr with custom deleter for VideoPicture::rgbaFrame --- extern/osg-ffmpeg-videoplayer/videostate.cpp | 20 ++++++++++++-------- extern/osg-ffmpeg-videoplayer/videostate.hpp | 6 +++++- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/extern/osg-ffmpeg-videoplayer/videostate.cpp b/extern/osg-ffmpeg-videoplayer/videostate.cpp index cdfdc8ad5..0380c82b0 100644 --- a/extern/osg-ffmpeg-videoplayer/videostate.cpp +++ b/extern/osg-ffmpeg-videoplayer/videostate.cpp @@ -156,6 +156,12 @@ void PacketQueue::clear() this->mutex.unlock (); } +void VideoPicture::AVFrameDeleter::operator()(AVFrame* frame) const +{ + av_freep(frame->data); + av_frame_free(&frame); +} + int VideoState::istream_read(void *user_data, uint8_t *buf, int buf_size) { try @@ -623,8 +629,9 @@ int VideoState::stream_open(int stream_index, AVFormatContext *pFormatCtx) } // Allocate RGBA frame queue. - for (std::size_t i = 0; i < VIDEO_PICTURE_ARRAY_SIZE; ++i) { - AVFrame *frame = av_frame_alloc(); + for (std::size_t i = 0; i < VIDEO_PICTURE_ARRAY_SIZE; ++i) + { + std::unique_ptr frame{av_frame_alloc()}; if (frame == nullptr) { std::cerr << "av_frame_alloc failed" << std::endl; return -1; @@ -638,7 +645,7 @@ int VideoState::stream_open(int stream_index, AVFormatContext *pFormatCtx) std::cerr << "av_image_alloc failed" << std::endl; return -1; } - this->pictq[i].rgbaFrame = frame; + this->pictq[i].rgbaFrame = std::move(frame); } this->video_thread.reset(new VideoThread(this)); @@ -784,11 +791,8 @@ void VideoState::deinit() } // Dellocate RGBA frame queue. - for (std::size_t i = 0; i < VIDEO_PICTURE_ARRAY_SIZE; ++i) { - if (this->pictq[i].rgbaFrame == nullptr) continue; - av_freep(&this->pictq[i].rgbaFrame->data[0]); - av_frame_free(&this->pictq[i].rgbaFrame); - } + for (std::size_t i = 0; i < VIDEO_PICTURE_ARRAY_SIZE; ++i) + this->pictq[i].rgbaFrame = nullptr; } diff --git a/extern/osg-ffmpeg-videoplayer/videostate.hpp b/extern/osg-ffmpeg-videoplayer/videostate.hpp index ed226bcf2..b54c03cb9 100644 --- a/extern/osg-ffmpeg-videoplayer/videostate.hpp +++ b/extern/osg-ffmpeg-videoplayer/videostate.hpp @@ -95,7 +95,11 @@ struct VideoPicture { VideoPicture() : pts(0.0) { } - AVFrame* rgbaFrame = nullptr; + struct AVFrameDeleter { + void operator()(AVFrame* frame) const; + }; + + std::unique_ptr rgbaFrame; double pts; }; From 58d33aa95b729d81c236a587b59352c6a7c7f017 Mon Sep 17 00:00:00 2001 From: Gleb Mazovetskiy Date: Mon, 8 Mar 2021 03:16:55 +0000 Subject: [PATCH 3/5] AV: Fix all memory leaks The most substantial memory leak came from `PacketQueue::get` not unreferencing its argument packet. Other leaks came from using `av_free` instead of type-specific free functions. Also modifies `PacketQueue::put` for readability. --- apps/openmw/mwsound/ffmpeg_decoder.cpp | 10 +++---- .../osg-ffmpeg-videoplayer/audiodecoder.cpp | 4 +-- extern/osg-ffmpeg-videoplayer/videostate.cpp | 28 +++++++++---------- 3 files changed, 19 insertions(+), 23 deletions(-) diff --git a/apps/openmw/mwsound/ffmpeg_decoder.cpp b/apps/openmw/mwsound/ffmpeg_decoder.cpp index 95ed9eeed..84ea30b3c 100644 --- a/apps/openmw/mwsound/ffmpeg_decoder.cpp +++ b/apps/openmw/mwsound/ffmpeg_decoder.cpp @@ -287,9 +287,9 @@ void FFmpeg_Decoder::close() mStream = nullptr; av_packet_unref(&mPacket); - av_freep(&mFrame); - swr_free(&mSwr); av_freep(&mDataBuf); + av_frame_free(&mFrame); + swr_free(&mSwr); if(mFormatCtx) { @@ -302,11 +302,9 @@ void FFmpeg_Decoder::close() // if (mFormatCtx->pb->buffer != nullptr) { - av_free(mFormatCtx->pb->buffer); - mFormatCtx->pb->buffer = nullptr; + av_freep(&mFormatCtx->pb->buffer); } - av_free(mFormatCtx->pb); - mFormatCtx->pb = nullptr; + avio_context_free(&mFormatCtx->pb); } avformat_close_input(&mFormatCtx); } diff --git a/extern/osg-ffmpeg-videoplayer/audiodecoder.cpp b/extern/osg-ffmpeg-videoplayer/audiodecoder.cpp index 337382679..c32794d2a 100644 --- a/extern/osg-ffmpeg-videoplayer/audiodecoder.cpp +++ b/extern/osg-ffmpeg-videoplayer/audiodecoder.cpp @@ -91,7 +91,7 @@ MovieAudioDecoder::~MovieAudioDecoder() if(mAudioContext) avcodec_free_context(&mAudioContext); - av_freep(&mFrame); + av_frame_free(&mFrame); av_freep(&mDataBuf); } @@ -222,7 +222,7 @@ int MovieAudioDecoder::audio_decode_frame(AVFrame *frame, int &sample_skip) return result; } - av_packet_unref(&mPacket); + av_packet_unref(pkt); mGetNextPacket = true; /* next packet */ diff --git a/extern/osg-ffmpeg-videoplayer/videostate.cpp b/extern/osg-ffmpeg-videoplayer/videostate.cpp index 0380c82b0..7e523e054 100644 --- a/extern/osg-ffmpeg-videoplayer/videostate.cpp +++ b/extern/osg-ffmpeg-videoplayer/videostate.cpp @@ -83,10 +83,11 @@ void PacketQueue::put(AVPacket *pkt) pkt1 = (AVPacketList*)av_malloc(sizeof(AVPacketList)); if(!pkt1) throw std::bad_alloc(); - if(pkt != &flush_pkt && !pkt->buf && av_packet_ref(&pkt1->pkt, pkt) < 0) - throw std::runtime_error("Failed to duplicate packet"); + if(pkt == &flush_pkt) + pkt1->pkt = *pkt; + else + av_packet_move_ref(&pkt1->pkt, pkt); - pkt1->pkt = *pkt; pkt1->next = nullptr; this->mutex.lock (); @@ -117,7 +118,8 @@ int PacketQueue::get(AVPacket *pkt, VideoState *is) this->nb_packets--; this->size -= pkt1->pkt.size; - *pkt = pkt1->pkt; + av_packet_unref(pkt); + av_packet_move_ref(pkt, &pkt1->pkt); av_free(pkt1); return 1; @@ -364,6 +366,7 @@ public: { VideoState* self = mVideoState; AVPacket pkt1, *packet = &pkt1; + av_init_packet(packet); AVFrame *pFrame; pFrame = av_frame_alloc(); @@ -436,6 +439,7 @@ public: AVFormatContext *pFormatCtx = self->format_ctx; AVPacket pkt1, *packet = &pkt1; + av_init_packet(packet); try { @@ -691,16 +695,13 @@ void VideoState::init(std::shared_ptr inputstream, const std::stri { if (this->format_ctx->pb != nullptr) { - av_free(this->format_ctx->pb->buffer); - this->format_ctx->pb->buffer = nullptr; - - av_free(this->format_ctx->pb); - this->format_ctx->pb = nullptr; + av_freep(&this->format_ctx->pb->buffer); + avio_context_free(&this->format_ctx->pb); } } // "Note that a user-supplied AVFormatContext will be freed on failure." this->format_ctx = nullptr; - av_free(ioCtx); + avio_context_free(&ioCtx); throw std::runtime_error("Failed to open video input"); } @@ -774,11 +775,8 @@ void VideoState::deinit() /// if (this->format_ctx->pb != nullptr) { - av_free(this->format_ctx->pb->buffer); - this->format_ctx->pb->buffer = nullptr; - - av_free(this->format_ctx->pb); - this->format_ctx->pb = nullptr; + av_freep(&this->format_ctx->pb->buffer); + avio_context_free(&this->format_ctx->pb); } avformat_close_input(&this->format_ctx); } From 36bac353df3ccf0b6d21d591a994ac4922b367e6 Mon Sep 17 00:00:00 2001 From: Gleb Mazovetskiy Date: Mon, 8 Mar 2021 04:00:11 +0000 Subject: [PATCH 4/5] AV: Handle varying video frame dimensions --- extern/osg-ffmpeg-videoplayer/videostate.cpp | 60 ++++++++++++-------- extern/osg-ffmpeg-videoplayer/videostate.hpp | 6 ++ 2 files changed, 43 insertions(+), 23 deletions(-) diff --git a/extern/osg-ffmpeg-videoplayer/videostate.cpp b/extern/osg-ffmpeg-videoplayer/videostate.cpp index 7e523e054..a3a1e4318 100644 --- a/extern/osg-ffmpeg-videoplayer/videostate.cpp +++ b/extern/osg-ffmpeg-videoplayer/videostate.cpp @@ -158,6 +158,33 @@ void PacketQueue::clear() this->mutex.unlock (); } +int VideoPicture::set_dimensions(int w, int h) { + if (this->rgbaFrame != nullptr && this->rgbaFrame->width == w && + this->rgbaFrame->height == h) { + return 0; + } + + std::unique_ptr frame{ + av_frame_alloc()}; + if (frame == nullptr) { + std::cerr << "av_frame_alloc failed" << std::endl; + return -1; + } + + constexpr AVPixelFormat kPixFmt = AV_PIX_FMT_RGBA; + frame->format = kPixFmt; + frame->width = w; + frame->height = h; + if (av_image_alloc(frame->data, frame->linesize, frame->width, frame->height, + kPixFmt, 1) < 0) { + std::cerr << "av_image_alloc failed" << std::endl; + return -1; + } + + this->rgbaFrame = std::move(frame); + return 0; +} + void VideoPicture::AVFrameDeleter::operator()(AVFrame* frame) const { av_freep(frame->data); @@ -305,18 +332,25 @@ int VideoState::queue_picture(AVFrame *pFrame, double pts) // Convert the image into RGBA format // TODO: we could do this in a pixel shader instead, if the source format // matches a commonly used format (ie YUV420P) - if(this->sws_context == nullptr) + const int w = pFrame->width; + const int h = pFrame->height; + if(this->sws_context == nullptr || this->sws_context_w != w || this->sws_context_h != h) { - int w = this->video_ctx->width; - int h = this->video_ctx->height; + if (this->sws_context != nullptr) + sws_freeContext(this->sws_context); this->sws_context = sws_getContext(w, h, this->video_ctx->pix_fmt, w, h, AV_PIX_FMT_RGBA, SWS_BICUBIC, nullptr, nullptr, nullptr); if(this->sws_context == nullptr) throw std::runtime_error("Cannot initialize the conversion context!\n"); + this->sws_context_w = w; + this->sws_context_h = h; } vp->pts = pts; + if (vp->set_dimensions(w, h) < 0) + return -1; + sws_scale(this->sws_context, pFrame->data, pFrame->linesize, 0, this->video_ctx->height, vp->rgbaFrame->data, vp->rgbaFrame->linesize); @@ -632,26 +666,6 @@ int VideoState::stream_open(int stream_index, AVFormatContext *pFormatCtx) return -1; } - // Allocate RGBA frame queue. - for (std::size_t i = 0; i < VIDEO_PICTURE_ARRAY_SIZE; ++i) - { - std::unique_ptr frame{av_frame_alloc()}; - if (frame == nullptr) { - std::cerr << "av_frame_alloc failed" << std::endl; - return -1; - } - - constexpr AVPixelFormat kPixFmt = AV_PIX_FMT_RGBA; - frame->format = kPixFmt; - frame->width = this->video_ctx->width; - frame->height = this->video_ctx->height; - if (av_image_alloc(frame->data, frame->linesize, frame->width, frame->height, kPixFmt, 1) < 0) { - std::cerr << "av_image_alloc failed" << std::endl; - return -1; - } - this->pictq[i].rgbaFrame = std::move(frame); - } - this->video_thread.reset(new VideoThread(this)); break; diff --git a/extern/osg-ffmpeg-videoplayer/videostate.hpp b/extern/osg-ffmpeg-videoplayer/videostate.hpp index b54c03cb9..015656084 100644 --- a/extern/osg-ffmpeg-videoplayer/videostate.hpp +++ b/extern/osg-ffmpeg-videoplayer/videostate.hpp @@ -99,6 +99,11 @@ struct VideoPicture { void operator()(AVFrame* frame) const; }; + // Sets frame dimensions. + // Must be called before writing to `rgbaFrame`. + // Return -1 on error. + int set_dimensions(int w, int h); + std::unique_ptr rgbaFrame; double pts; }; @@ -163,6 +168,7 @@ struct VideoState { double video_clock; /// Date: Mon, 8 Mar 2021 04:16:33 +0000 Subject: [PATCH 5/5] AV: Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e4b70780..97634cedf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -97,6 +97,7 @@ Bug #5739: Saving and loading the save a second or two before hitting the ground doesn't count fall damage Bug #5758: Paralyzed actors behavior is inconsistent with vanilla Bug #5762: Movement solver is insufficiently robust + Bug #5807: Video decoding crash on ARM Bug #5821: NPCs from mods getting removed if mod order was changed Bug #5835: OpenMW doesn't accept negative values for NPC's hello, alarm, fight, and flee Bug #5836: OpenMW dialogue/greeting/voice filter doesn't accept negative Ai values for NPC's hello, alarm, fight, and flee