Skip to content

Commit

Permalink
fix: Avoid occasional crash when changing video devices or closing qTox.
Browse files Browse the repository at this point in the history
There was an AV race condition caused by clearing the frame buffer while
the VideoFrame is in flight towards toxav. The fix is heavy-handed in
that we're now just copying the frame every time, even though we almost
never have this problem (only when changing devices). So we pay the cost
all the time even though it's rarely needed. However, this is a serious
crashing bug (heap-use-after-free), so I'd rather have it fixed.
  • Loading branch information
iphydf committed Dec 31, 2024
1 parent e473566 commit 2bd629f
Show file tree
Hide file tree
Showing 6 changed files with 21 additions and 9 deletions.
1 change: 1 addition & 0 deletions .ci-scripts/macos_install_deps.sh
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,5 @@ install_deps() {
popd
rm -rf "external/$dep"
done
rmdir external
}
2 changes: 2 additions & 0 deletions .github/workflows/build-test-deploy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ jobs:
run: .ci-scripts/import-developer-keys.sh
- name: Verify signatures
if: github.event_name == 'pull_request'
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: tools/verify-release-assets.py

update-nightly-tag:
Expand Down
4 changes: 2 additions & 2 deletions src/core/coreav.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -408,8 +408,8 @@ void CoreAV::sendCallVideo(uint32_t callId, std::shared_ptr<VideoFrame> vframe)
Toxav_Err_Send_Frame err;
int retries = 0;
do {
if (!toxav_video_send_frame(toxav.get(), callId, frame.width, frame.height, frame.y,
frame.u, frame.v, &err)) {
if (!toxav_video_send_frame(toxav.get(), callId, frame.width, frame.height, frame.y.data(),
frame.u.data(), frame.v.data(), &err)) {
if (err == TOXAV_ERR_SEND_FRAME_SYNC) {
++retries;
QThread::usleep(500);
Expand Down
1 change: 1 addition & 0 deletions src/platform/camera/avfoundation.mm
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
QVector<QPair<QString, QString>> result;

#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
CGRequestScreenCaptureAccess();
const AVAuthorizationStatus authStatus =
[AVCaptureDevice authorizationStatusForMediaType:AVMediaTypeVideo];
if (authStatus != AVAuthorizationStatusDenied && authStatus != AVAuthorizationStatusNotDetermined) {
Expand Down
15 changes: 11 additions & 4 deletions src/video/videoframe.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -335,12 +335,19 @@ ToxYUVFrame VideoFrame::toToxYUVFrame(QSize frameSize)

return toGenericObject(frameSize, AV_PIX_FMT_YUV420P, true, [frameSize](AVFrame* const frame) {
// Converter function (constructs ToxAVFrame out of AVFrame*)
// TODO(iphydf): Be more efficient (less copying).
return ToxYUVFrame{
static_cast<std::uint16_t>(frameSize.width()),
static_cast<std::uint16_t>(frameSize.height()),
frame->data[0],
frame->data[1],
frame->data[2],
// width * height
std::vector<uint8_t>(frame->data[0],
frame->data[0] + frameSize.width() * frameSize.height()),
// (width/2) * (height/2)
std::vector<uint8_t>(frame->data[1],
frame->data[1] + (frameSize.width() / 2) * (frameSize.height() / 2)),
// (width/2) * (height/2)
std::vector<uint8_t>(frame->data[2],
frame->data[2] + (frameSize.width() / 2) * (frameSize.height() / 2)),
};
});
}
Expand Down Expand Up @@ -704,7 +711,7 @@ VideoFrame::toGenericObject(const QSize& dimensions, int pixelFormat, bool requi
QReadLocker frameReadLock{&frameLock};

// We return empty if the VideoFrame is no longer valid
if (frameBuffer.size() == 0) {
if (frameBuffer.empty()) {
return {};
}

Expand Down
7 changes: 4 additions & 3 deletions src/video/videoframe.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ extern "C"
#include <functional>
#include <memory>
#include <unordered_map>
#include <vector>

struct ToxYUVFrame
{
Expand All @@ -34,9 +35,9 @@ struct ToxYUVFrame
const std::uint16_t width;
const std::uint16_t height;

const uint8_t* y;
const uint8_t* u;
const uint8_t* v;
const std::vector<uint8_t> y;
const std::vector<uint8_t> u;
const std::vector<uint8_t> v;
};

class VideoFrame
Expand Down

0 comments on commit 2bd629f

Please sign in to comment.