Skip to content

Commit 58eca1b

Browse files
wysaidwysaid
authored
fix: critical zero-copy and format conversion bugs (#52)
* fix: critical zero-copy and format conversion bugs Fixes 11 critical and medium-priority bugs across backends: CRITICAL (HIGH): - Fix double CVPixelBufferUnlockBaseAddress in Apple camera - Fix double CVPixelBufferUnlockBaseAddress in Apple file reader - Fix use-after-free in Windows file reader DANGLING POINTER (MEDIUM): - Fix dangling nativeHandle in Apple camera after conversion - Fix dangling nativeHandle in Apple file reader after conversion - Fix dangling nativeHandle in DirectShow backend FORMAT CONVERSION (MEDIUM): - Guard shouldConvert against Unknown output format in DShow/MSMF - Log warnings for unsupported YUV-to-different-YUV conversion - Log warnings for unsupported RGB-to-YUV conversion DOCUMENTATION (LOW): - Fix kPixelFormatBGRBit comment typo - Remove misleading @refitem from I420 docs - Clarify zero-copy requirements in PixelFormatOutput - Document Apple YUV subtype behavior All 907 functional tests pass with ASAN enabled. No regressions. * fix: handle Unknown output format and reduce per-frame log noise - Derive effectiveOutputFormat to avoid passing PixelFormat::Unknown to inplaceConvertFrame across all providers (DirectShow, MSMF, V4L2, AVFoundation) and file readers (Windows MF, Apple AVAssetReader). When outputPixelFormat is Unknown, treat it as 'keep input format' so isOutputYUV, shouldConvert, shouldFlip, and the conversion call all use the camera/input format instead. - Use log-once pattern for unsupported YUV-to-YUV (without libyuv) and RGB-to-YUV conversion warnings to prevent flooding stderr in long-running captures. - Update PixelFormatOutput documentation to clarify Unknown semantics and unsupported conversion limitations. * docs: clarify log suppression behavior in inplaceConvertFrame
1 parent af18e57 commit 58eca1b

10 files changed

Lines changed: 70 additions & 33 deletions

‎include/ccap_def.h‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ namespace ccap {
3535
enum PixelFormatConstants : uint32_t {
3636
/// `kPixelFormatRGBBit` indicates that the pixel format is RGB or RGBA.
3737
kPixelFormatRGBBit = 1 << 3,
38-
/// `kPixelFormatRGBBit` indicates that the pixel format is BGR or BGRA.
38+
/// `kPixelFormatBGRBit` indicates that the pixel format is BGR or BGRA.
3939
kPixelFormatBGRBit = 1 << 4,
4040

4141
/// Color Bit Mask
@@ -82,7 +82,6 @@ enum class PixelFormat : uint32_t {
8282
* In software design, you can implement a toggle option to allow users to choose whether
8383
* the received Frame is FullRange or VideoRange based on what they observe.
8484
* @note This format is also known by other names, such as YUV420P or IYUV.
85-
* @refitem #NV12
8685
*/
8786
I420 = 1 << 2 | kPixelFormatYUVColorBit,
8887

@@ -191,10 +190,14 @@ enum class PropertyName {
191190

192191
/**
193192
* @brief The output pixel format of ccap. Can be different from PixelFormatInternal.
194-
* @note If PixelFormatInternal is RGB(A), PixelFormatOutput cannot be set to a YUV format.
193+
* @note If PixelFormatInternal is RGB(A), PixelFormatOutput cannot be set to a YUV format (RGB->YUV conversion is not supported).
194+
* If PixelFormatInternal is YUV and PixelFormatOutput is a different YUV subtype, conversion requires libyuv;
195+
* without it the frame will keep the camera format and no conversion is performed.
195196
* If PixelFormatInternal is YUV and PixelFormatOutput is RGB(A), BT.601 will be used for conversion.
196-
* For other cases, there are no issues.
197-
* If PixelFormatInternal and PixelFormatOutput are the same format, data conversion will be skipped and the original data will be used directly.
197+
* If PixelFormatOutput is set to PixelFormat::Unknown (or not set), the camera's native format is used as-is
198+
* and no conversion is performed.
199+
* If PixelFormatInternal and PixelFormatOutput are the same format AND the camera natively supports
200+
* PixelFormatInternal, data conversion will be skipped and the original data will be used directly.
198201
* In general, setting both PixelFormatInternal and PixelFormatOutput to YUV formats can achieve better performance.
199202
*/
200203
PixelFormatOutput = 0x30002,

‎src/ccap_convert_frame.cpp‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
#include "ccap_convert.h"
1212
#include "ccap_imp.h"
13+
#include "ccap_utils.h"
1314

1415
#include <cassert>
1516
#include <cstring>
@@ -229,8 +230,25 @@ inline bool inplaceConvertFrameImp(VideoFrame* frame, PixelFormat toFormat, bool
229230
return inplaceConvertFrameYUV2YUV(frame, toFormat, verticalFlip);
230231
#endif
231232

233+
if (isInputYUV && isOutputYUV) {
234+
// Best-effort log suppression only; occasional duplicate warnings are acceptable.
235+
static bool sLoggedYuv2YuvUnsupported = false;
236+
if (!sLoggedYuv2YuvUnsupported) {
237+
CCAP_LOG_W("ccap: YUV to different YUV subtype conversion is not supported without libyuv, skipping conversion\n");
238+
sLoggedYuv2YuvUnsupported = true;
239+
}
240+
return false;
241+
}
242+
232243
if (isInputYUV) // yuv -> BGR
233244
return inplaceConvertFrameYUV2RGBColor(frame, toFormat, verticalFlip);
245+
246+
// Best-effort log suppression only; occasional duplicate warnings are acceptable.
247+
static bool sLoggedRgbToYuvUnsupported = false;
248+
if (!sLoggedRgbToYuvUnsupported) {
249+
CCAP_LOG_W("ccap: RGB to YUV conversion is not supported, skipping conversion\n");
250+
sLoggedRgbToYuvUnsupported = true;
251+
}
234252
return false; // no rgb -> yuv
235253
}
236254

‎src/ccap_file_reader_apple.mm‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -382,10 +382,11 @@ - (void)processFrame:(CMSampleBufferRef)sampleBuffer {
382382

383383
// Check if conversion or flip is needed
384384
auto& prop = _provider->getFrameProperty();
385-
bool isOutputYUV = (newFrame->pixelFormat & kPixelFormatYUVColorBit) != 0;
385+
PixelFormat effectiveOutputFormat = (prop.outputPixelFormat == PixelFormat::Unknown) ? newFrame->pixelFormat : prop.outputPixelFormat;
386+
bool isOutputYUV = (effectiveOutputFormat & kPixelFormatYUVColorBit) != 0;
386387
FrameOrientation targetOrientation = isOutputYUV ? FrameOrientation::TopToBottom : _provider->frameOrientation();
387388
bool shouldFlip = !isOutputYUV && (inputOrientation != targetOrientation);
388-
bool shouldConvert = newFrame->pixelFormat != prop.outputPixelFormat;
389+
bool shouldConvert = newFrame->pixelFormat != effectiveOutputFormat;
389390

390391
newFrame->orientation = targetOrientation;
391392

@@ -397,8 +398,11 @@ - (void)processFrame:(CMSampleBufferRef)sampleBuffer {
397398
newFrame->allocator = f ? f() : std::make_shared<DefaultAllocator>();
398399
}
399400

400-
zeroCopy = !inplaceConvertFrame(newFrame.get(), prop.outputPixelFormat, shouldFlip);
401-
CVPixelBufferUnlockBaseAddress(imageBuffer, kCVPixelBufferLock_ReadOnly);
401+
zeroCopy = !inplaceConvertFrame(newFrame.get(), effectiveOutputFormat, shouldFlip);
402+
if (!zeroCopy) {
403+
CVPixelBufferUnlockBaseAddress(imageBuffer, kCVPixelBufferLock_ReadOnly);
404+
newFrame->nativeHandle = nullptr;
405+
}
402406
}
403407

404408
if (zeroCopy) {

‎src/ccap_file_reader_windows.cpp‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -448,10 +448,11 @@ void FileReaderWindows::readLoop() {
448448

449449
// Check if conversion or flip is needed
450450
auto& prop = m_provider->getFrameProperty();
451-
bool isOutputYUV = (prop.outputPixelFormat & kPixelFormatYUVColorBit) != 0;
451+
PixelFormat effectiveOutputFormat = (prop.outputPixelFormat == PixelFormat::Unknown) ? newFrame->pixelFormat : prop.outputPixelFormat;
452+
bool isOutputYUV = (effectiveOutputFormat & kPixelFormatYUVColorBit) != 0;
452453
FrameOrientation targetOrientation = isOutputYUV ? FrameOrientation::TopToBottom : m_provider->frameOrientation();
453454
bool shouldFlip = !isOutputYUV && (inputOrientation != targetOrientation);
454-
bool shouldConvert = newFrame->pixelFormat != prop.outputPixelFormat;
455+
bool shouldConvert = newFrame->pixelFormat != effectiveOutputFormat;
455456

456457
newFrame->orientation = targetOrientation;
457458

@@ -462,7 +463,7 @@ void FileReaderWindows::readLoop() {
462463
auto&& f = m_provider->getAllocatorFactory();
463464
newFrame->allocator = f ? f() : std::make_shared<DefaultAllocator>();
464465
}
465-
inplaceConvertFrame(newFrame.get(), prop.outputPixelFormat, shouldFlip);
466+
zeroCopy = !inplaceConvertFrame(newFrame.get(), effectiveOutputFormat, shouldFlip);
466467
}
467468

468469
newFrame->frameIndex = m_currentFrameIndex;

‎src/ccap_imp_apple.mm‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -873,6 +873,9 @@ - (void)captureOutput:(AVCaptureOutput*)output
873873
CMTime timestamp = CMSampleBufferGetPresentationTimeStamp(sampleBuffer);
874874
auto internalFormat = _provider->getFrameProperty().cameraPixelFormat;
875875
auto outputFormat = _provider->getFrameProperty().outputPixelFormat;
876+
if (outputFormat == PixelFormat::Unknown) {
877+
outputFormat = internalFormat;
878+
}
876879

877880
newFrame->timestamp = (uint64_t)(CMTimeGetSeconds(timestamp) * 1e9);
878881
newFrame->width = (uint32_t)CVPixelBufferGetWidth(imageBuffer);
@@ -905,6 +908,8 @@ - (void)captureOutput:(AVCaptureOutput*)output
905908
}
906909

907910
/// iOS/macOS does not support i420, and we do not intend to support nv12 to i420 conversion here.
911+
/// When both internal and output formats are YUV, zeroCopy is used regardless of subtype differences
912+
/// (e.g., NV12 vs I420). The frame will carry the actual camera format, not the requested output format.
908913
bool zeroCopy = ((internalFormat & kPixelFormatYUVColorBit) && (outputFormat & kPixelFormatYUVColorBit)) ||
909914
(internalFormat == outputFormat && _provider->frameOrientation() == kDefaultFrameOrientation);
910915

@@ -924,7 +929,10 @@ - (void)captureOutput:(AVCaptureOutput*)output
924929

925930
zeroCopy = !inplaceConvertFrame(newFrame.get(), outputFormat, (int)(newFrame->orientation != kDefaultFrameOrientation));
926931

927-
CVPixelBufferUnlockBaseAddress(imageBuffer, kCVPixelBufferLock_ReadOnly);
932+
if (!zeroCopy) {
933+
CVPixelBufferUnlockBaseAddress(imageBuffer, kCVPixelBufferLock_ReadOnly);
934+
newFrame->nativeHandle = nullptr;
935+
}
928936

929937
if (verboseLogEnabled()) {
930938
#ifdef DEBUG

‎src/ccap_imp_linux.cpp‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -549,16 +549,16 @@ bool ProviderV4L2::readFrame() {
549549

550550
// Check input/output format types and orientations
551551
bool isInputYUV = (frame->pixelFormat & kPixelFormatYUVColorBit) != 0;
552-
bool isOutputYUV = (m_frameProp.outputPixelFormat & kPixelFormatYUVColorBit) != 0;
552+
PixelFormat effectiveOutputFormat = (m_frameProp.outputPixelFormat == PixelFormat::Unknown) ? frame->pixelFormat : m_frameProp.outputPixelFormat;
553+
bool isOutputYUV = (effectiveOutputFormat & kPixelFormatYUVColorBit) != 0;
553554
auto inputOrientation = FrameOrientation::TopToBottom; // V4L2 always provides TopToBottom
554555

555556
// Set output orientation based on format type
556557
frame->orientation = isOutputYUV ? FrameOrientation::TopToBottom : m_frameOrientation;
557558

558559
// Check if we need conversion or flipping
559560
bool shouldFlip = frame->orientation != inputOrientation && !isOutputYUV;
560-
bool shouldConvert = (m_frameProp.outputPixelFormat != PixelFormat::Unknown &&
561-
m_frameProp.outputPixelFormat != frame->pixelFormat);
561+
bool shouldConvert = (effectiveOutputFormat != frame->pixelFormat);
562562
bool zeroCopy = !shouldConvert && !shouldFlip;
563563

564564
uint8_t* bufferData = static_cast<uint8_t*>(m_buffers[buf.index].start);
@@ -614,7 +614,7 @@ bool ProviderV4L2::readFrame() {
614614

615615
std::chrono::steady_clock::time_point startTime = std::chrono::steady_clock::now();
616616

617-
zeroCopy = !inplaceConvertFrame(frame.get(), m_frameProp.outputPixelFormat, shouldFlip);
617+
zeroCopy = !inplaceConvertFrame(frame.get(), effectiveOutputFormat, shouldFlip);
618618

619619
double durInMs = (std::chrono::steady_clock::now() - startTime).count() / 1.e6;
620620
static double s_allCostTime = 0;
@@ -630,10 +630,10 @@ bool ProviderV4L2::readFrame() {
630630

631631
CCAP_LOG_V(
632632
"ccap: inplaceConvertFrame requested pixel format: %s, actual pixel format: %s, flip: %s, cost time %s: (cur %g ms, avg %g ms)\n",
633-
pixelFormatToString(m_frameProp.outputPixelFormat).data(), pixelFormatToString(m_frameProp.cameraPixelFormat).data(),
633+
pixelFormatToString(effectiveOutputFormat).data(), pixelFormatToString(m_frameProp.cameraPixelFormat).data(),
634634
shouldFlip ? "YES" : "NO", mode, durInMs, s_allCostTime / s_frames);
635635
} else {
636-
zeroCopy = !inplaceConvertFrame(frame.get(), m_frameProp.outputPixelFormat, shouldFlip);
636+
zeroCopy = !inplaceConvertFrame(frame.get(), effectiveOutputFormat, shouldFlip);
637637
}
638638
}
639639

‎src/ccap_imp_linux.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,12 +101,12 @@ class ProviderV4L2 : public ProviderImp {
101101
bool m_isStreaming = false;
102102

103103
// V4L2 device capabilities
104-
struct v4l2_capability m_caps{};
104+
struct v4l2_capability m_caps {};
105105
std::vector<V4L2Format> m_supportedFormats;
106106
std::vector<DeviceInfo::Resolution> m_supportedResolutions;
107107

108108
// Current format
109-
struct v4l2_format m_currentFormat{};
109+
struct v4l2_format m_currentFormat {};
110110

111111
// Buffer management
112112
std::vector<V4L2Buffer> m_buffers;

‎src/ccap_imp_windows.cpp‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -843,16 +843,17 @@ HRESULT STDMETHODCALLTYPE ProviderDirectShow::SampleCB(double sampleTime, IMedia
843843

844844
uint32_t bufferLen = mediaSample->GetActualDataLength();
845845
bool isInputYUV = (m_frameProp.cameraPixelFormat & kPixelFormatYUVColorBit);
846-
bool isOutputYUV = (m_frameProp.outputPixelFormat & kPixelFormatYUVColorBit);
846+
PixelFormat effectiveOutputFormat = (m_frameProp.outputPixelFormat == PixelFormat::Unknown) ? m_frameProp.cameraPixelFormat : m_frameProp.outputPixelFormat;
847+
bool isOutputYUV = (effectiveOutputFormat & kPixelFormatYUVColorBit);
847848

848849
newFrame->pixelFormat = m_frameProp.cameraPixelFormat;
849850
newFrame->width = m_frameProp.width;
850851
newFrame->height = m_frameProp.height;
851852
newFrame->orientation = isOutputYUV ? FrameOrientation::TopToBottom : m_frameOrientation;
852-
newFrame->nativeHandle = mediaSample;
853+
newFrame->nativeHandle = nullptr;
853854

854855
bool shouldFlip = newFrame->orientation != m_inputOrientation && !isOutputYUV;
855-
bool shouldConvert = m_frameProp.cameraPixelFormat != m_frameProp.outputPixelFormat;
856+
bool shouldConvert = m_frameProp.cameraPixelFormat != effectiveOutputFormat;
856857
bool zeroCopy = !shouldConvert && !shouldFlip;
857858

858859
if (isInputYUV) {
@@ -920,7 +921,7 @@ HRESULT STDMETHODCALLTYPE ProviderDirectShow::SampleCB(double sampleTime, IMedia
920921

921922
std::chrono::steady_clock::time_point startTime = std::chrono::steady_clock::now();
922923

923-
zeroCopy = !inplaceConvertFrame(newFrame.get(), m_frameProp.outputPixelFormat, shouldFlip);
924+
zeroCopy = !inplaceConvertFrame(newFrame.get(), effectiveOutputFormat, shouldFlip);
924925

925926
double durInMs = (std::chrono::steady_clock::now() - startTime).count() / 1.e6;
926927
static double s_allCostTime = 0;
@@ -936,10 +937,10 @@ HRESULT STDMETHODCALLTYPE ProviderDirectShow::SampleCB(double sampleTime, IMedia
936937

937938
CCAP_LOG_V(
938939
"ccap: inplaceConvertFrame requested pixel format: %s, actual pixel format: %s, flip: %s, cost time %s: (cur %g ms, avg %g ms)\n",
939-
pixelFormatToString(m_frameProp.outputPixelFormat).data(), pixelFormatToString(m_frameProp.cameraPixelFormat).data(),
940+
pixelFormatToString(effectiveOutputFormat).data(), pixelFormatToString(m_frameProp.cameraPixelFormat).data(),
940941
shouldFlip ? "YES" : "NO", mode, durInMs, s_allCostTime / s_frames);
941942
} else {
942-
zeroCopy = !inplaceConvertFrame(newFrame.get(), m_frameProp.outputPixelFormat, shouldFlip);
943+
zeroCopy = !inplaceConvertFrame(newFrame.get(), effectiveOutputFormat, shouldFlip);
943944
}
944945

945946
newFrame->sizeInBytes = newFrame->stride[0] * newFrame->height + (newFrame->stride[1] + newFrame->stride[2]) * newFrame->height / 2;
@@ -949,6 +950,7 @@ HRESULT STDMETHODCALLTYPE ProviderDirectShow::SampleCB(double sampleTime, IMedia
949950
// Conversion may fail. If conversion fails, fall back to zero-copy mode.
950951
// In this case, the returned format is the original camera input format.
951952
newFrame->sizeInBytes = bufferLen;
953+
newFrame->nativeHandle = mediaSample;
952954

953955
mediaSample->AddRef(); // Ensure data lifecycle
954956
auto manager = std::make_shared<FakeFrame>([newFrame, mediaSample]() mutable {
@@ -1001,7 +1003,7 @@ HRESULT STDMETHODCALLTYPE ProviderDirectShow::BufferCB(double SampleTime, BYTE*
10011003
return S_OK;
10021004
}
10031005

1004-
HRESULT STDMETHODCALLTYPE ProviderDirectShow::QueryInterface(REFIID riid, _COM_Outptr_ void __RPC_FAR * __RPC_FAR * ppvObject) {
1006+
HRESULT STDMETHODCALLTYPE ProviderDirectShow::QueryInterface(REFIID riid, _COM_Outptr_ void __RPC_FAR* __RPC_FAR* ppvObject) {
10051007
static constexpr const IID IID_ISampleGrabberCB = { 0x0579154A, 0x2B53, 0x4994, { 0xB0, 0xD0, 0xE7, 0x73, 0x14, 0x8E, 0xFF, 0x85 } };
10061008

10071009
if (riid == IID_IUnknown) {
@@ -1166,7 +1168,7 @@ void ProviderDirectShow::close() {
11661168
bool ProviderDirectShow::start() {
11671169
if (!m_isOpened) return false;
11681170

1169-
// File mode
1171+
// File mode
11701172
#ifdef CCAP_ENABLE_FILE_PLAYBACK
11711173
if (m_isFileMode && m_fileReader) {
11721174
return m_fileReader->start();

‎src/ccap_imp_windows.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ class ProviderDirectShow : public ProviderImp, public ISampleGrabberCB {
9393
inline FrameOrientation frameOrientation() const { return m_frameOrientation; }
9494

9595
private:
96-
HRESULT STDMETHODCALLTYPE QueryInterface(REFIID riid, _COM_Outptr_ void __RPC_FAR * __RPC_FAR * ppvObject) override;
96+
HRESULT STDMETHODCALLTYPE QueryInterface(REFIID riid, _COM_Outptr_ void __RPC_FAR* __RPC_FAR* ppvObject) override;
9797
ULONG STDMETHODCALLTYPE AddRef(void) override;
9898
ULONG STDMETHODCALLTYPE Release(void) override;
9999

‎src/ccap_imp_windows_msmf.cpp‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -747,7 +747,8 @@ void ProviderMSMF::readLoop() {
747747
newFrame->height = m_activeHeight;
748748
newFrame->nativeHandle = nullptr;
749749

750-
bool isOutputYUV = (m_frameProp.outputPixelFormat & kPixelFormatYUVColorBit) != 0;
750+
PixelFormat effectiveOutputFormat = (m_frameProp.outputPixelFormat == PixelFormat::Unknown) ? m_activePixelFormat : m_frameProp.outputPixelFormat;
751+
bool isOutputYUV = (effectiveOutputFormat & kPixelFormatYUVColorBit) != 0;
751752
FrameOrientation targetOrientation = isOutputYUV ? FrameOrientation::TopToBottom : m_frameOrientation;
752753
newFrame->orientation = targetOrientation;
753754

@@ -801,15 +802,15 @@ void ProviderMSMF::readLoop() {
801802
}
802803

803804
bool shouldFlip = !isOutputYUV && targetOrientation != m_inputOrientation;
804-
bool shouldConvert = newFrame->pixelFormat != m_frameProp.outputPixelFormat;
805+
bool shouldConvert = newFrame->pixelFormat != effectiveOutputFormat;
805806
bool zeroCopy = !shouldConvert && !shouldFlip;
806807

807808
if (!zeroCopy) {
808809
if (!newFrame->allocator) {
809810
newFrame->allocator = m_allocatorFactory ? m_allocatorFactory() : std::make_shared<DefaultAllocator>();
810811
}
811812

812-
zeroCopy = !inplaceConvertFrame(newFrame.get(), m_frameProp.outputPixelFormat, shouldFlip);
813+
zeroCopy = !inplaceConvertFrame(newFrame.get(), effectiveOutputFormat, shouldFlip);
813814
newFrame->sizeInBytes = newFrame->stride[0] * newFrame->height +
814815
(newFrame->stride[1] + newFrame->stride[2]) * newFrame->height / 2;
815816
}

0 commit comments

Comments
 (0)