-
Notifications
You must be signed in to change notification settings - Fork 44
Make C++ compilation strict, treat warnings as errors #452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 14 commits
64d1a75
c228588
07ef579
0b8e9e0
22c6957
5a3aaba
9c1ec44
36f6321
4883b03
a4df539
fef1ea0
75b39d6
c85f9e6
0667dd7
b846335
3097e74
bf68d42
1883af5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ set(CMAKE_CXX_STANDARD 17) | |
set(CMAKE_CXX_STANDARD_REQUIRED ON) | ||
|
||
find_package(Torch REQUIRED) | ||
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${TORCH_CXX_FLAGS}") | ||
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${TORCH_CXX_FLAGS} -Wall -Wextra -pedantic -Werror") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I wonder if we should add those before |
||
find_package(Python3 ${PYTHON_VERSION} EXACT COMPONENTS Development) | ||
|
||
function(make_torchcodec_library library_name ffmpeg_target) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,7 @@ int64_t getDuration(const UniqueAVFrame& frame) { | |
} | ||
|
||
int64_t getDuration(const AVFrame* frame) { | ||
#if LIBAVUTIL_VERSION_MAJOR < 59 | ||
#if LIBAVUTIL_VERSION_MAJOR < 58 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we just had this wrong. It was deprecated in 58. |
||
return frame->pkt_duration; | ||
#else | ||
return frame->duration; | ||
|
@@ -75,7 +75,8 @@ int AVIOBytesContext::read(void* opaque, uint8_t* buf, int buf_size) { | |
bufferData->current, | ||
", size=", | ||
bufferData->size); | ||
buf_size = FFMIN(buf_size, bufferData->size - bufferData->current); | ||
buf_size = | ||
FFMIN(buf_size, static_cast<int>(bufferData->size - bufferData->current)); | ||
TORCH_CHECK( | ||
buf_size >= 0, | ||
"Tried to read negative bytes: buf_size=", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -206,7 +206,8 @@ VideoDecoder::BatchDecodedOutput::BatchDecodedOutput( | |
|
||
bool VideoDecoder::DecodedFrameContext::operator==( | ||
const VideoDecoder::DecodedFrameContext& other) { | ||
return decodedWidth == other.decodedWidth && decodedHeight == decodedHeight && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that the stricter compilation revealed a real logic error here, as we had the code |
||
return decodedWidth == other.decodedWidth && | ||
decodedHeight == other.decodedHeight && | ||
decodedFormat == other.decodedFormat && | ||
expectedWidth == other.expectedWidth && | ||
expectedHeight == other.expectedHeight; | ||
|
@@ -249,12 +250,12 @@ void VideoDecoder::initializeDecoder() { | |
getFFMPEGErrorStringFromErrorCode(ffmpegStatus)); | ||
} | ||
|
||
for (int i = 0; i < formatContext_->nb_streams; i++) { | ||
for (unsigned int i = 0; i < formatContext_->nb_streams; i++) { | ||
AVStream* stream = formatContext_->streams[i]; | ||
StreamMetadata meta; | ||
|
||
TORCH_CHECK( | ||
i == stream->index, | ||
static_cast<int>(i) == stream->index, | ||
"Our stream index, " + std::to_string(i) + | ||
", does not match AVStream's index, " + | ||
std::to_string(stream->index) + "."); | ||
|
@@ -577,7 +578,7 @@ void VideoDecoder::scanFileAndUpdateMetadataAndIndex() { | |
} | ||
streams_[streamIndex].allFrames.push_back(frameInfo); | ||
} | ||
for (int i = 0; i < containerMetadata_.streams.size(); ++i) { | ||
for (size_t i = 0; i < containerMetadata_.streams.size(); ++i) { | ||
auto& streamMetadata = containerMetadata_.streams[i]; | ||
auto stream = formatContext_->streams[i]; | ||
if (streamMetadata.minPtsFromScan.has_value()) { | ||
|
@@ -610,7 +611,7 @@ void VideoDecoder::scanFileAndUpdateMetadataAndIndex() { | |
return frameInfo1.pts < frameInfo2.pts; | ||
}); | ||
|
||
for (int i = 0; i < stream.allFrames.size(); ++i) { | ||
for (size_t i = 0; i < stream.allFrames.size(); ++i) { | ||
if (i + 1 < stream.allFrames.size()) { | ||
stream.allFrames[i].nextPts = stream.allFrames[i + 1].pts; | ||
} | ||
|
@@ -1050,8 +1051,8 @@ VideoDecoder::DecodedOutput VideoDecoder::getFramePlayedAtTimestampNoDemux( | |
return output; | ||
} | ||
|
||
void VideoDecoder::validateUserProvidedStreamIndex(uint64_t streamIndex) { | ||
size_t streamsSize = containerMetadata_.streams.size(); | ||
void VideoDecoder::validateUserProvidedStreamIndex(int streamIndex) { | ||
int streamsSize = static_cast<int>(containerMetadata_.streams.size()); | ||
TORCH_CHECK( | ||
streamIndex >= 0 && streamIndex < streamsSize, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we had declared |
||
"Invalid stream index=" + std::to_string(streamIndex) + | ||
|
@@ -1074,7 +1075,7 @@ void VideoDecoder::validateFrameIndex( | |
const StreamInfo& stream, | ||
int64_t frameIndex) { | ||
TORCH_CHECK( | ||
frameIndex >= 0 && frameIndex < stream.allFrames.size(), | ||
frameIndex >= 0 && frameIndex < static_cast<int>(stream.allFrames.size()), | ||
"Invalid frame index=" + std::to_string(frameIndex) + | ||
" for streamIndex=" + std::to_string(stream.streamIndex) + | ||
" numFrames=" + std::to_string(stream.allFrames.size())); | ||
|
@@ -1134,10 +1135,11 @@ VideoDecoder::BatchDecodedOutput VideoDecoder::getFramesAtIndices( | |
BatchDecodedOutput output(frameIndices.size(), options, streamMetadata); | ||
|
||
auto previousIndexInVideo = -1; | ||
for (auto f = 0; f < frameIndices.size(); ++f) { | ||
for (size_t f = 0; f < frameIndices.size(); ++f) { | ||
auto indexInOutput = indicesAreSorted ? f : argsort[f]; | ||
auto indexInVideo = frameIndices[indexInOutput]; | ||
if (indexInVideo < 0 || indexInVideo >= stream.allFrames.size()) { | ||
if (indexInVideo < 0 || | ||
indexInVideo >= static_cast<int>(stream.allFrames.size())) { | ||
throw std::runtime_error( | ||
"Invalid frame index=" + std::to_string(indexInVideo)); | ||
} | ||
|
@@ -1180,7 +1182,7 @@ VideoDecoder::BatchDecodedOutput VideoDecoder::getFramesPlayedByTimestamps( | |
double maxSeconds = streamMetadata.maxPtsSecondsFromScan.value(); | ||
|
||
std::vector<int64_t> frameIndices(timestamps.size()); | ||
for (auto i = 0; i < timestamps.size(); ++i) { | ||
for (size_t i = 0; i < timestamps.size(); ++i) { | ||
auto framePts = timestamps[i]; | ||
TORCH_CHECK( | ||
framePts >= minSeconds && framePts < maxSeconds, | ||
|
@@ -1215,7 +1217,7 @@ VideoDecoder::BatchDecodedOutput VideoDecoder::getFramesInRange( | |
TORCH_CHECK( | ||
start >= 0, "Range start, " + std::to_string(start) + " is less than 0."); | ||
TORCH_CHECK( | ||
stop <= stream.allFrames.size(), | ||
stop <= static_cast<int64_t>(stream.allFrames.size()), | ||
"Range stop, " + std::to_string(stop) + | ||
", is more than the number of frames, " + | ||
std::to_string(stream.allFrames.size())); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed doing this one last week, and it only just now started failing.