From afd3391c734ec861d2be4f543e596ad98d6b557e Mon Sep 17 00:00:00 2001 From: "Steinar H. Gunderson" Date: Sun, 10 May 2020 18:14:10 +0200 Subject: [PATCH] Fix some issues with cards changing pixel format on-the-fly. If a card came back with another pixel format than before, we would still allocate frames with the same pixel format, which obviously didn't work. This is very unlikely to happen in the current situation (or even impossible?), but needs to be fixed for upcoming patches. --- nageru/mixer.cpp | 9 +++++ nageru/pbo_frame_allocator.cpp | 72 ++++++++++++++++++++++++++++++++-- nageru/pbo_frame_allocator.h | 23 ++++++++++- 3 files changed, 99 insertions(+), 5 deletions(-) diff --git a/nageru/mixer.cpp b/nageru/mixer.cpp index 465e967..76ab1ba 100644 --- a/nageru/mixer.cpp +++ b/nageru/mixer.cpp @@ -568,6 +568,15 @@ void Mixer::configure_card(unsigned card_index, CaptureInterface *capture, CardT card->capture->set_frame_callback(bind(&Mixer::bm_frame, this, card_index, _1, _2, _3, _4, _5, _6, _7)); if (card->frame_allocator == nullptr) { card->frame_allocator.reset(new PBOFrameAllocator(pixel_format, 8 << 20, global_flags.width, global_flags.height, card_index, mjpeg_encoder.get())); // 8 MB. + } else { + // The format could have changed, but we cannot reset the allocator + // and create a new one from scratch, since there may be allocated + // frames from it that expect to call release_frame() on it. + // Instead, ask the allocator to create new frames for us and discard + // any old ones as they come back. This takes the mutex while + // allocating, but nothing should really be sending frames in there + // right now anyway (start_bm_capture() has not been called yet). + card->frame_allocator->reconfigure(pixel_format, 8 << 20, global_flags.width, global_flags.height, card_index, mjpeg_encoder.get()); } card->capture->set_video_frame_allocator(card->frame_allocator.get()); if (card->surface == nullptr) { diff --git a/nageru/pbo_frame_allocator.cpp b/nageru/pbo_frame_allocator.cpp index 133b65f..f9b4601 100644 --- a/nageru/pbo_frame_allocator.cpp +++ b/nageru/pbo_frame_allocator.cpp @@ -29,11 +29,20 @@ void set_clamp_to_edge() } // namespace PBOFrameAllocator::PBOFrameAllocator(bmusb::PixelFormat pixel_format, size_t frame_size, GLuint width, GLuint height, unsigned card_index, MJPEGEncoder *mjpeg_encoder, size_t num_queued_frames, GLenum buffer, GLenum permissions, GLenum map_bits) - : card_index(card_index), mjpeg_encoder(mjpeg_encoder), pixel_format(pixel_format), buffer(buffer) + : card_index(card_index), + mjpeg_encoder(mjpeg_encoder), + pixel_format(pixel_format), + buffer(buffer), + frame_size(frame_size), + num_queued_frames(num_queued_frames), + width(width), + height(height), + permissions(permissions), + map_bits(map_bits) { userdata.reset(new Userdata[num_queued_frames]); for (size_t i = 0; i < num_queued_frames; ++i) { - init_frame(i, frame_size, width, height, permissions, map_bits); + init_frame(i, frame_size, width, height, permissions, map_bits, generation); } glBindBuffer(buffer, 0); check_error(); @@ -41,7 +50,7 @@ PBOFrameAllocator::PBOFrameAllocator(bmusb::PixelFormat pixel_format, size_t fra check_error(); } -void PBOFrameAllocator::init_frame(size_t frame_idx, size_t frame_size, GLuint width, GLuint height, GLenum permissions, GLenum map_bits) +void PBOFrameAllocator::init_frame(size_t frame_idx, size_t frame_size, GLuint width, GLuint height, GLenum permissions, GLenum map_bits, int generation) { GLuint pbo; glGenBuffers(1, &pbo); @@ -58,6 +67,7 @@ void PBOFrameAllocator::init_frame(size_t frame_idx, size_t frame_size, GLuint w frame.size = frame_size; Userdata *ud = &userdata[frame_idx]; frame.userdata = ud; + ud->generation = generation; ud->pbo = pbo; ud->pixel_format = pixel_format; ud->data_copy_malloc = new uint8_t[frame_size]; @@ -388,6 +398,60 @@ void PBOFrameAllocator::release_frame(Frame frame) } lock_guard lock(freelist_mutex); - freelist.push(frame); + Userdata *userdata = (Userdata *)frame.userdata; + if (userdata->generation == generation) { + freelist.push(frame); + } else { + destroy_frame(&frame); + } //--sumsum; } + +void PBOFrameAllocator::reconfigure(bmusb::PixelFormat pixel_format, + size_t frame_size, + GLuint width, GLuint height, + unsigned card_index, + MJPEGEncoder *mjpeg_encoder, + size_t num_queued_frames, + GLenum buffer, + GLenum permissions, + GLenum map_bits) +{ + if (pixel_format == this->pixel_format && + frame_size == this->frame_size && + width == this->width && height == this->height && + card_index == this->card_index && + mjpeg_encoder == this->mjpeg_encoder && + num_queued_frames == this->num_queued_frames && + buffer == this->buffer && + permissions == this->permissions && + map_bits == this->map_bits) { + return; + } + + this->pixel_format = pixel_format; + this->frame_size = frame_size; + this->width = width; + this->height = height; + this->card_index = card_index; + this->mjpeg_encoder = mjpeg_encoder; + this->num_queued_frames = num_queued_frames; + this->buffer = buffer; + this->permissions = permissions; + this->map_bits = map_bits; + + lock_guard lock(freelist_mutex); + while (!freelist.empty()) { + Frame frame = freelist.front(); + freelist.pop(); + destroy_frame(&frame); + } + ++generation; + for (size_t i = 0; i < num_queued_frames; ++i) { + init_frame(i, frame_size, width, height, permissions, map_bits, generation); + } + + // There may still be frames out with the old configuration + // (for instance, living in GLWidget); they will be destroyed + // when they come back in release_frame(). +} diff --git a/nageru/pbo_frame_allocator.h b/nageru/pbo_frame_allocator.h index 0080dd0..5ea0ffe 100644 --- a/nageru/pbo_frame_allocator.h +++ b/nageru/pbo_frame_allocator.h @@ -36,6 +36,17 @@ public: Frame create_frame(size_t width, size_t height, size_t stride) override; void release_frame(Frame frame) override; + // NOTE: Does not check the buffer types; they are just assumed to be compatible. + void reconfigure(bmusb::PixelFormat pixel_format, + size_t frame_size, + GLuint width, GLuint height, + unsigned card_index, + MJPEGEncoder *mjpeg_encoder = nullptr, + size_t num_queued_frames = 16, + GLenum buffer = GL_PIXEL_UNPACK_BUFFER_ARB, + GLenum permissions = GL_MAP_WRITE_BIT, + GLenum map_bits = GL_MAP_FLUSH_EXPLICIT_BIT); + struct Userdata { GLuint pbo; @@ -80,10 +91,12 @@ public: uint8_t *data_copy_malloc; MJPEGEncoder::VAResources va_resources; MJPEGEncoder::ReleaseVAResources va_resources_release; + + int generation; }; private: - void init_frame(size_t frame_idx, size_t frame_size, GLuint width, GLuint height, GLenum permissions, GLenum map_bits); + void init_frame(size_t frame_idx, size_t frame_size, GLuint width, GLuint height, GLenum permissions, GLenum map_bits, int generation); void destroy_frame(Frame *frame); unsigned card_index; @@ -93,6 +106,14 @@ private: std::queue freelist; GLenum buffer; std::unique_ptr userdata; + + // Used only for reconfigure(), to check whether we can do without. + size_t frame_size; + size_t num_queued_frames; + GLuint width, height; + GLenum permissions; + GLenum map_bits; + int generation = 0; // Under freelist_mutex. }; #endif // !defined(_PBO_FRAME_ALLOCATOR) -- 2.39.2