Validate position and dimensions of TplBlockStats Also added convenience functions ok(), operator*(), and operator->() to Status/StatusOr. Now that CreateTplFrameDepStatsWithoutPropagation crops blocks to their overlap with the frame, it seems a good idea to check that the dimensions are valid before cropping. Change-Id: I7d0c968cfc2d3bd5925938dfe33f74a432b3e977
diff --git a/av1/ratectrl_qmode.cc b/av1/ratectrl_qmode.cc index 83e09cb..e12f52e 100644 --- a/av1/ratectrl_qmode.cc +++ b/av1/ratectrl_qmode.cc
@@ -903,10 +903,40 @@ return dep_stats; } -TplFrameDepStats CreateTplFrameDepStatsWithoutPropagation( +namespace { +Status ValidateBlockStats(const TplFrameStats &frame_stats, + const TplBlockStats &block_stats, + int min_block_size) { + if (block_stats.col >= frame_stats.frame_width || + block_stats.row >= frame_stats.frame_height) { + std::ostringstream error_message; + error_message << "Block position (" << block_stats.col << ", " + << block_stats.row + << ") is out of range; frame dimensions are " + << frame_stats.frame_width << " x " + << frame_stats.frame_height; + return { AOM_CODEC_INVALID_PARAM, error_message.str() }; + } + if (block_stats.col % min_block_size != 0 || + block_stats.row % min_block_size != 0 || + block_stats.width % min_block_size != 0 || + block_stats.height % min_block_size != 0) { + std::ostringstream error_message; + error_message + << "Invalid block position or dimension, must be a multiple of " + << min_block_size << "; col = " << block_stats.col + << ", row = " << block_stats.row << ", width = " << block_stats.width + << ", height = " << block_stats.height; + return { AOM_CODEC_INVALID_PARAM, error_message.str() }; + } + return { AOM_CODEC_OK, "" }; +} +} // namespace + +StatusOr<TplFrameDepStats> CreateTplFrameDepStatsWithoutPropagation( const TplFrameStats &frame_stats) { if (frame_stats.block_stats_list.empty()) { - return {}; + return TplFrameDepStats(); } const int min_block_size = frame_stats.min_block_size; const int unit_rows = @@ -916,6 +946,11 @@ TplFrameDepStats frame_dep_stats = CreateTplFrameDepStats( frame_stats.frame_height, frame_stats.frame_width, min_block_size); for (const TplBlockStats &block_stats : frame_stats.block_stats_list) { + Status status = + ValidateBlockStats(frame_stats, block_stats, min_block_size); + if (!status.ok()) { + return status; + } const int block_unit_row = block_stats.row / min_block_size; const int block_unit_col = block_stats.col / min_block_size; // The block must start within the frame boundaries, but it may extend past @@ -1101,7 +1136,7 @@ return ref_frame_table_list; } -TplGopDepStats ComputeTplGopDepStats( +StatusOr<TplGopDepStats> ComputeTplGopDepStats( const TplGopStats &tpl_gop_stats, const std::vector<RefFrameTable> &ref_frame_table_list) { const int frame_count = @@ -1109,10 +1144,16 @@ // Create the struct to store TPL dependency stats TplGopDepStats tpl_gop_dep_stats; + tpl_gop_dep_stats.frame_dep_stats_list.reserve(frame_count); for (int coding_idx = 0; coding_idx < frame_count; coding_idx++) { - tpl_gop_dep_stats.frame_dep_stats_list.push_back( + const StatusOr<TplFrameDepStats> tpl_frame_dep_stats = CreateTplFrameDepStatsWithoutPropagation( - tpl_gop_stats.frame_stats_list[coding_idx])); + tpl_gop_stats.frame_stats_list[coding_idx]); + if (!tpl_frame_dep_stats.ok()) { + return tpl_frame_dep_stats.status(); + } + tpl_gop_dep_stats.frame_dep_stats_list.push_back( + std::move(*tpl_frame_dep_stats)); } // Back propagation @@ -1149,8 +1190,11 @@ GopEncodeInfo gop_encode_info; gop_encode_info.final_snapshot = ref_frame_table_list.back(); - TplGopDepStats gop_dep_stats = + StatusOr<TplGopDepStats> gop_dep_stats = ComputeTplGopDepStats(tpl_gop_stats, ref_frame_table_list); + if (!gop_dep_stats.ok()) { + return gop_dep_stats.status(); + } const int frame_count = static_cast<int>(tpl_gop_stats.frame_stats_list.size()); for (int i = 0; i < frame_count; i++) { @@ -1162,7 +1206,7 @@ param.q_index = rc_param_.base_q_index; } else { const TplFrameDepStats &frame_dep_stats = - gop_dep_stats.frame_dep_stats_list[i]; + gop_dep_stats->frame_dep_stats_list[i]; const double cost_without_propagation = TplFrameDepStatsAccumulateIntraCost(frame_dep_stats); const double cost_with_propagation =
diff --git a/av1/ratectrl_qmode.h b/av1/ratectrl_qmode.h index bca86c2..7a59687 100644 --- a/av1/ratectrl_qmode.h +++ b/av1/ratectrl_qmode.h
@@ -69,7 +69,7 @@ TplUnitDepStats TplBlockStatsToDepStats(const TplBlockStats &block_stats, int unit_count); -TplFrameDepStats CreateTplFrameDepStatsWithoutPropagation( +StatusOr<TplFrameDepStats> CreateTplFrameDepStatsWithoutPropagation( const TplFrameStats &frame_stats); std::vector<int> GetKeyFrameList(const FirstpassInfo &first_pass_info); @@ -85,7 +85,7 @@ int GetBlockOverlapArea(int r0, int c0, int r1, int c1, int size); -TplGopDepStats ComputeTplGopDepStats( +StatusOr<TplGopDepStats> ComputeTplGopDepStats( const TplGopStats &tpl_gop_stats, const std::vector<RefFrameTable> &ref_frame_table_list);
diff --git a/av1/ratectrl_qmode_interface.h b/av1/ratectrl_qmode_interface.h index 5d5d830..316a699 100644 --- a/av1/ratectrl_qmode_interface.h +++ b/av1/ratectrl_qmode_interface.h
@@ -85,6 +85,7 @@ struct Status { aom_codec_err_t code; std::string message; // Empty if code == AOM_CODEC_OK. + bool ok() const { return code == AOM_CODEC_OK; } }; // A very simple imitation of absl::StatusOr, this is conceptually a union of a @@ -99,11 +100,38 @@ StatusOr(Status status) : status_(std::move(status)) { assert(status_.code != AOM_CODEC_OK); } - const T &value() const & { return value_; } - T &value() & { return value_; } - const T &&value() const && { return value_; } - T &&value() && { return std::move(value_); } + const Status &status() const { return status_; } + bool ok() const { return status().ok(); } + + // operator* returns the value; it should only be called after checking that + // ok() returns true. + const T &operator*() const & { return value_; } + T &operator*() & { return value_; } + const T &&operator*() const && { return value_; } + T &&operator*() && { return std::move(value_); } + + // sor->field is equivalent to (*sor).field. + const T *operator->() const & { return &value_; } + T *operator->() & { return &value_; } + + // value() is equivalent to operator*, but asserts that ok() is true. + const T &value() const & { + assert(ok()); + return value_; + } + T &value() & { + assert(ok()); + return value_; + } + const T &&value() const && { + assert(ok()); + return value_; + } + T &&value() && { + assert(ok()); + return std::move(value_); + } private: T value_; // This could be std::optional<T> if it were available.
diff --git a/test/ratectrl_qmode_test.cc b/test/ratectrl_qmode_test.cc index 13d9d50..ae516e9 100644 --- a/test/ratectrl_qmode_test.cc +++ b/test/ratectrl_qmode_test.cc
@@ -29,8 +29,16 @@ namespace { +using ::testing::HasSubstr; + constexpr int kRefFrameTableSize = 7; +MATCHER(IsOkStatus, "") { + *result_listener << "with code " << arg.code + << " and message: " << arg.message; + return arg.ok(); +} + // Reads a whitespace-delimited string from stream, and parses it as a double. // Returns an empty string if the entire string was successfully parsed as a // double, or an error messaage if not. @@ -431,21 +439,38 @@ TEST(RateControlQModeTest, CreateTplFrameDepStats) { TplFrameStats frame_stats = CreateToyTplFrameStatsWithDiffSizes(8, 16); - TplFrameDepStats frame_dep_stats = + StatusOr<TplFrameDepStats> frame_dep_stats = CreateTplFrameDepStatsWithoutPropagation(frame_stats); - EXPECT_EQ(frame_stats.min_block_size, frame_dep_stats.unit_size); - const int unit_rows = static_cast<int>(frame_dep_stats.unit_stats.size()); - const int unit_cols = static_cast<int>(frame_dep_stats.unit_stats[0].size()); - EXPECT_EQ(frame_stats.frame_height, unit_rows * frame_dep_stats.unit_size); - EXPECT_EQ(frame_stats.frame_width, unit_cols * frame_dep_stats.unit_size); + ASSERT_THAT(frame_dep_stats.status(), IsOkStatus()); + EXPECT_EQ(frame_stats.min_block_size, frame_dep_stats->unit_size); + const int unit_rows = static_cast<int>(frame_dep_stats->unit_stats.size()); + const int unit_cols = static_cast<int>(frame_dep_stats->unit_stats[0].size()); + EXPECT_EQ(frame_stats.frame_height, unit_rows * frame_dep_stats->unit_size); + EXPECT_EQ(frame_stats.frame_width, unit_cols * frame_dep_stats->unit_size); const double intra_cost_sum = - TplFrameDepStatsAccumulateIntraCost(frame_dep_stats); + TplFrameDepStatsAccumulateIntraCost(*frame_dep_stats); const double expected_intra_cost_sum = TplFrameStatsAccumulateIntraCost(frame_stats); EXPECT_NEAR(intra_cost_sum, expected_intra_cost_sum, kErrorEpsilon); } +TEST(RateControlQModeTest, BlockRowNotAMultipleOfMinBlockSizeError) { + TplFrameStats frame_stats = CreateToyTplFrameStatsWithDiffSizes(8, 16); + frame_stats.block_stats_list.back().row = 1; + auto result = CreateTplFrameDepStatsWithoutPropagation(frame_stats); + EXPECT_FALSE(result.ok()); + EXPECT_THAT(result.status().message, HasSubstr("must be a multiple of 8")); +} + +TEST(RateControlQModeTest, BlockPositionOutOfRangeError) { + TplFrameStats frame_stats = CreateToyTplFrameStatsWithDiffSizes(8, 16); + frame_stats.block_stats_list.back().row += 8; + auto result = CreateTplFrameDepStatsWithoutPropagation(frame_stats); + EXPECT_FALSE(result.ok()); + EXPECT_THAT(result.status().message, HasSubstr("out of range")); +} + TEST(RateControlQModeTest, GetBlockOverlapArea) { const int size = 8; const int r0 = 8; @@ -490,9 +515,10 @@ gop_dep_stats.frame_dep_stats_list.push_back(frame_dep_stats0); // cur frame with coding_idx 1 - const TplFrameDepStats frame_dep_stats1 = + const StatusOr<TplFrameDepStats> frame_dep_stats1 = CreateTplFrameDepStatsWithoutPropagation(frame_stats); - gop_dep_stats.frame_dep_stats_list.push_back(frame_dep_stats1); + ASSERT_THAT(frame_dep_stats1.status(), IsOkStatus()); + gop_dep_stats.frame_dep_stats_list.push_back(std::move(*frame_dep_stats1)); const RefFrameTable ref_frame_table = CreateToyRefFrameTable(frame_count); TplFrameDepStatsPropagate(/*coding_idx=*/1, ref_frame_table, &gop_dep_stats); @@ -532,9 +558,10 @@ gop_dep_stats.frame_dep_stats_list.push_back(frame_dep_stats1); // cur frame with coding_idx 2 - const TplFrameDepStats frame_dep_stats2 = + const StatusOr<TplFrameDepStats> frame_dep_stats2 = CreateTplFrameDepStatsWithoutPropagation(frame_stats); - gop_dep_stats.frame_dep_stats_list.push_back(frame_dep_stats2); + ASSERT_THAT(frame_dep_stats2.status(), IsOkStatus()); + gop_dep_stats.frame_dep_stats_list.push_back(std::move(*frame_dep_stats2)); const RefFrameTable ref_frame_table = CreateToyRefFrameTable(frame_count); TplFrameDepStatsPropagate(/*coding_idx=*/2, ref_frame_table, &gop_dep_stats); @@ -578,8 +605,10 @@ frame_stats.min_block_size)); // cur frame with coding_idx 1 - gop_dep_stats.frame_dep_stats_list.push_back( - CreateTplFrameDepStatsWithoutPropagation(frame_stats)); + const StatusOr<TplFrameDepStats> frame_dep_stats = + CreateTplFrameDepStatsWithoutPropagation(frame_stats); + ASSERT_THAT(frame_dep_stats.status(), IsOkStatus()); + gop_dep_stats.frame_dep_stats_list.push_back(std::move(*frame_dep_stats)); const RefFrameTable ref_frame_table = CreateToyRefFrameTable(frame_count); TplFrameDepStatsPropagate(/*coding_idx=*/1, ref_frame_table, &gop_dep_stats); @@ -627,8 +656,9 @@ ref_frame_table_list.push_back(CreateToyRefFrameTable(i)); } - const TplGopDepStats &gop_dep_stats = + const StatusOr<TplGopDepStats> gop_dep_stats = ComputeTplGopDepStats(tpl_gop_stats, ref_frame_table_list); + ASSERT_THAT(gop_dep_stats.status(), IsOkStatus()); double expected_sum = 0; for (int i = 2; i >= 0; i--) { @@ -637,7 +667,7 @@ expected_sum += TplFrameStatsAccumulateIntraCost(tpl_gop_stats.frame_stats_list[i]); const double sum = - TplFrameDepStatsAccumulate(gop_dep_stats.frame_dep_stats_list[i]); + TplFrameDepStatsAccumulate(gop_dep_stats->frame_dep_stats_list[i]); EXPECT_NEAR(sum, expected_sum, kErrorEpsilon); break; } @@ -909,12 +939,6 @@ return frame; } -MATCHER(IsOkStatus, "") { - *result_listener << "with code " << arg.code - << " and message: " << arg.message; - return arg.code == AOM_CODEC_OK; -} - TEST(RateControlQModeTest, TestSetRcParamErrorChecking) { // Default constructed RateControlParam should not be valid. RateControlParam rc_param = {}; @@ -1005,7 +1029,7 @@ const auto gop_info = rc.DetermineGopInfo(firstpass_info); ASSERT_THAT(gop_info.status(), IsOkStatus()); std::vector<int> gop_interval_list; - std::transform(gop_info.value().begin(), gop_info.value().end(), + std::transform(gop_info->begin(), gop_info->end(), std::back_inserter(gop_interval_list), [](GopStruct const &x) { return x.show_frame_count; }); EXPECT_THAT(gop_interval_list, @@ -1028,7 +1052,7 @@ ASSERT_THAT(rc.SetRcParam(rc_param), IsOkStatus()); const auto gop_info = rc.DetermineGopInfo(firstpass_info); ASSERT_THAT(gop_info.status(), IsOkStatus()); - const GopStructList &gop_list = gop_info.value(); + const GopStructList &gop_list = *gop_info; // Read TPL stats std::vector<TplGopStats> tpl_gop_list; ASSERT_NO_FATAL_FAILURE(ReadTplInfo("tpl_stats", gop_list, &tpl_gop_list)); @@ -1044,10 +1068,10 @@ const auto gop_encode_info = rc.GetGopEncodeInfo( gop_list[gop_idx], tpl_gop_list[tpl_gop_idx], ref_frame_table); ASSERT_THAT(gop_encode_info.status(), IsOkStatus()); - for (auto &frame_param : gop_encode_info.value().param_list) { + for (auto &frame_param : gop_encode_info->param_list) { std::cout << frame_param.q_index << std::endl; } - ref_frame_table = gop_encode_info.value().final_snapshot; + ref_frame_table = gop_encode_info->final_snapshot; } }