COMP: Migrate OpenCV Video Bridge from legacy C API to C++ API#5994
Conversation
d0d56ba to
982662e
Compare
| using ITKDataType = itk::Matrix<T, VRows, VColumns>; | ||
| using ITKDataType = itk::Matrix<T, (unsigned int)VRows, (unsigned int)VColumns>; |
There was a problem hiding this comment.
Please don't use C-style cast! Is an explicit conversion really necessary? (If so, you may consider unsigned{VRows} and unsigned{VColumns}, which are 100% type safe.)
There was a problem hiding this comment.
Good catch — replaced both C-style casts with static_cast<unsigned int>(VRows) and static_cast<unsigned int>(VColumns). The explicit conversion is necessary here: the template parameter VRows is deduced as int (to match cv::Matx), but itk::Matrix requires unsigned int, so a cast in the specialization pattern is required for deduction to succeed. Squashed into the same commit.
There was a problem hiding this comment.
Thanks, but are you sure that the explicit cast is necessary? Do you get a warning, otherwise?
itk::Matrix is declared as follows:
template <typename T, unsigned int VRows = 3, unsigned int VColumns = 3>
class ITK_TEMPLATE_EXPORT Matrix
ITK/Modules/Core/Common/include/itkMatrix.h
Lines 51 to 52 in 2b7e6d4
I think it should be perfectly fine to pass a signed integer as argument for VRows or VColumns, for example itk::Matrix<float, 2, 2>. (2 is a signed int.)
There was a problem hiding this comment.
I did get a failure that this fixed. It is hard to test this code in all scenarios, so I figured that being explicit is a "won't hurt" condition.
There was a problem hiding this comment.
Well, there is a guideline: Avoid casts
So I would only add static_cast when it is really necessary 🤷 That's why I suggested to just remove the cast. Or otherwise use modern T{ x } conversion.
cv::Matx uses int template params; itk::Matrix uses unsigned int. When the same non-type parameter appeared in both sides of the partial specialization pattern, the compiler could not unify the deduction from itk::Matrix<T, uint(2), uint(3)> (unsigned int) with cv::Matx<T, 2, 3> (int), causing the specialization to be dropped and the empty primary template to be selected instead. Fix: use int for VRows/VColumns (matching OpenCV) as the deduction variable, and place a static_cast<unsigned int> in the ITK Matrix side of the pattern. C++17 allows integral-cast constant expressions in partial specialization patterns, and the compiler can invert a simple integral cast during deduction. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace CV_CAP_PROP_* macros with cv::CAP_PROP_* scoped enums, CV_FOURCC with cv::VideoWriter::fourcc(), CvCapture*/cvCaptureFromCAM/ cvReleaseCapture with cv::VideoCapture, and remove legacy C API headers (videoio_c.h, imgproc_c.h, highgui.h) that were removed in OpenCV 4.
982662e to
9cb4845
Compare
|
@dzenanz This looks like perhaps it is something that supports Kitware projects. Do you know a good candidate to review this PR? |
|
| Filename | Overview |
|---|---|
| Modules/Video/BridgeOpenCV/include/itkOpenCVBasicTypeBridge.h | Template parameters changed from unsigned int to int (matching cv::Matx) with static_cast<unsigned int> for the itk::Matrix partial specialization; correctly resolves deduction mismatch between OpenCV and ITK template conventions. |
| Modules/Video/BridgeOpenCV/include/itkOpenCVVideoCapture.hxx | Removes all legacy IplImage/C-header includes; replaces CV_FOURCC/CV_CAP_PROP_* macros with C++ equivalents; retrieve() now wraps the ITK buffer in a non-owning cv::Mat (correct rows/cols ordering) and uses cv::cvtColor directly — clean migration with correct dimension mapping. |
| Modules/Video/BridgeOpenCV/include/itkOpenCVVideoIO.h | Cleans up version-guarded legacy includes; replaces IplImage*/CvCapture*/CvVideoWriter* raw pointer members with RAII cv::Mat/cv::VideoCapture/cv::VideoWriter value members; also removes the duplicate private: label. |
| Modules/Video/BridgeOpenCV/src/itkOpenCVVideoIO.cxx | Full C→C++ API migration: removes manual cvRelease* calls, adds explicit RGBA read path, replaces memcpy with std::copy_n; OpenWriter() sets m_WriterOpen = true unconditionally without checking m_Writer.isOpened() — silent data loss if writer fails to open. |
| Modules/Video/BridgeOpenCV/test/itkOpenCVVideoIOTest.cxx | Migrates cross-validation capture from CvCapture*/cvQueryFrame to cv::VideoCapture/operator>>; adds isContinuous() guard around memcmp that silently skips comparison when false rather than failing the test. |
| Modules/Video/BridgeOpenCV/test/itkOpenCVVideoCaptureTest.cxx | Drops legacy videoio_c.h/imgproc_c.h conditional includes; replaces all CV_CAP_PROP_*/CV_FOURCC macros with cv::CAP_PROP_*/cv::VideoWriter::fourcc — straightforward, correct migration. |
| Modules/Video/BridgeOpenCV/test/itkOpenCVVideoIOFactoryTest.cxx | Replaces CvCapture*/cvCaptureFromCAM/cvReleaseCapture with cv::VideoCapture/isOpened()/release() — clean, correct migration. |
Sequence Diagram
sequenceDiagram
participant Caller
participant OpenCVVideoIO
participant cv_VideoCapture as cv::VideoCapture
participant cv_VideoWriter as cv::VideoWriter
Note over OpenCVVideoIO: Read path (C++ API)
Caller->>OpenCVVideoIO: ReadImageInformation()
OpenCVVideoIO->>cv_VideoCapture: open(filename) / open(cameraIndex)
cv_VideoCapture-->>OpenCVVideoIO: isOpened()
OpenCVVideoIO->>cv_VideoCapture: operator>> (tempFrame)
OpenCVVideoIO->>cv_VideoCapture: get(CAP_PROP_FRAME_COUNT/FPS/WIDTH/HEIGHT)
cv_VideoCapture-->>OpenCVVideoIO: metadata
Caller->>OpenCVVideoIO: Read(buffer)
OpenCVVideoIO->>cv_VideoCapture: operator>> (tempIm)
OpenCVVideoIO->>OpenCVVideoIO: cvtColor BGR→RGB (or BGRA→RGBA)
OpenCVVideoIO->>OpenCVVideoIO: std::copy_n(m_CVImage.data → buffer)
Note over OpenCVVideoIO: Write path (C++ API)
Caller->>OpenCVVideoIO: SetWriterParameters(fps, dim, fourCC, nChannels)
OpenCVVideoIO->>OpenCVVideoIO: VideoWriter::fourcc(...)
Caller->>OpenCVVideoIO: Write(buffer)
OpenCVVideoIO->>cv_VideoWriter: open(filename, fourcc, fps, size)
OpenCVVideoIO->>OpenCVVideoIO: cvtColor (GRAY/RGB → BGR)
OpenCVVideoIO->>cv_VideoWriter: write(m_CVImage)
Note over OpenCVVideoIO: Cleanup (RAII)
Caller->>OpenCVVideoIO: FinishReadingOrWriting()
OpenCVVideoIO->>cv_VideoCapture: release()
OpenCVVideoIO->>cv_VideoWriter: release()
Reviews (2): Last reviewed commit: "COMP: Replace legacy OpenCV C API in Vid..." | Re-trigger Greptile
Pre-existing condition: OpenWriter() set m_WriterOpen = true unconditionally after calling cvCreateVideoWriter() (C API) without checking for a null return. The C++ API migration in the previous commit preserved this bug by not calling m_Writer.isOpened() after m_Writer.open(). OpenReader() already guards m_ReaderOpen = true with an isOpened() check and throws on failure; align OpenWriter() to the same pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dzenanz
left a comment
There was a problem hiding this comment.
Looks good. I have some program which uses Video\BridgeOpenCV\include\itkOpenCVBasicTypeBridge.h, and it keeps working after these changes.
01aa5e1
into
InsightSoftwareConsortium:main
Summary
Migrates
Modules/Video/BridgeOpenCVfrom the OpenCV legacy C API to themodern C++ API. The C API (
IplImage*,CvCapture*,CvVideoWriter*,cvCaptureFromFile,cvQueryFrame,CV_CAP_PROP_*, etc.) was deprecated inOpenCV 2.x, moved to optional headers in OpenCV 3.x, and removed entirely in
OpenCV 4.0, causing build failures on any system with OpenCV 4+.
Changes per commit
1. Fix
OpenCVBasicTypeBridgetemplate deduction (OpenCV 3/4)cv::Matx<T, VRows, VColumns>specialization usedunsigned inttemplateparameters but
cv::Matxusesint. Cast(unsigned int)VRowsin the partialspecialization to allow the compiler to deduce both sides correctly.
2. Migrate
OpenCVVideoIOto C++ APIIplImageDeleter,CvCaptureDeleter, etc.)and all
std::unique_ptrwrappers around C API types.cv::Mat,cv::VideoCapture,cv::VideoWriter— these manage their own lifetime with no custom cleanup needed.cv::VideoCapture::isOpened(),operator>>for frame reads,cv::VideoWriter::write(),cv::cvtColor(),cv::VideoWriter::fourcc().memcpywithstd::copy_nfor type-safe buffer copy.opencv2/videoio.hppandopencv2/imgproc.hpp.3. Migrate
OpenCVVideoCaptureand test files to C++ APIIplImage*frame handling inretrieve()withcv::Matnon-owningconstructor wrapping the ITK frame buffer directly (no copy needed).
CV_FOURCC(...)macro withcv::VideoWriter::fourcc(...)(×2 inconstructors).
CV_CAP_PROP_*macros withcv::CAP_PROP_*scoped enums inset()/get()methods.CvCapture*/cvCaptureFromCAM/cvReleaseCapturewithcv::VideoCapture; replace all remainingCV_CAP_PROP_*/CV_FOURCCmacrousage; remove legacy C headers (
videoio_c.h,imgproc_c.h,highgui.h).Backward Compatibility
All C++ API replacements used have been available since OpenCV 2.0 or earlier.
This PR does not raise the minimum required OpenCV version.
IplImage*/cvCreateImageHeadercv::Mat(rows, cols, type, ptr)CvCapture*/cvCaptureFromFilecv::VideoCapture(filename)CvCapture*/cvCaptureFromCAMcv::VideoCapture(index)cvQueryFrame/cvGetNextImagecv::VideoCapture::operator>>cvGetCapturePropertycv::VideoCapture::get(propId)cvSetCapturePropertycv::VideoCapture::set(propId, val)CvVideoWriter*/cvCreateVideoWritercv::VideoWriter(file, fourcc, fps, size)cvWriteFramecv::VideoWriter::write(frame)cvReleaseCapture/cvReleaseVideoWriter.release())cvCvtColor/CV_BGR2RGBcv::cvtColor/cv::COLOR_BGR2RGBCV_FOURCC('M','P','4','2')macrocv::VideoWriter::fourcc('M','P','4','2')CV_CAP_PROP_POS_FRAMESmacrocv::CAP_PROP_POS_FRAMESscoped enumCV_CAP_PROP_FPSmacrocv::CAP_PROP_FPSscoped enumCV_CAP_PROP_FOURCCmacrocv::CAP_PROP_FOURCCscoped enumCV_CAP_PROP_FRAME_WIDTH/HEIGHTmacroscv::CAP_PROP_FRAME_WIDTH/HEIGHTenumsCV_CAP_PROP_POS_AVI_RATIOmacrocv::CAP_PROP_POS_AVI_RATIOscoped enumCV_CAP_PROP_POS_MSECmacrocv::CAP_PROP_POS_MSECscoped enumTest plan
ctest -R OpenCV)🤖 Generated with Claude Code