vp9 encoder: fix row-mt crash w/thread config change
previously row-mt would allocate thread data once, so increasing the
number of threads with a config change would cause a heap overflow.
Bug: chromium:1261415
Bug: chromium:1270689
Change-Id: I3c5ec8444ae91964fa34a19dd780bd2cbb0368bf
diff --git a/test/encode_api_test.cc b/test/encode_api_test.cc
index 6f940b6..6f61c77 100644
--- a/test/encode_api_test.cc
+++ b/test/encode_api_test.cc
@@ -10,10 +10,12 @@
#include <climits>
#include <cstring>
+#include <initializer_list>
#include "third_party/googletest/src/include/gtest/gtest.h"
#include "./vpx_config.h"
+#include "test/video_source.h"
#include "vpx/vp8cx.h"
#include "vpx/vpx_encoder.h"
@@ -300,4 +302,62 @@
}
}
+void InitCodec(const vpx_codec_iface_t &iface, int width, int height,
+ vpx_codec_ctx_t *enc, vpx_codec_enc_cfg_t *cfg) {
+ ASSERT_EQ(vpx_codec_enc_config_default(&iface, cfg, 0), VPX_CODEC_OK);
+ cfg->g_w = width;
+ cfg->g_h = height;
+ cfg->g_lag_in_frames = 0;
+ cfg->g_pass = VPX_RC_ONE_PASS;
+ ASSERT_EQ(vpx_codec_enc_init(enc, &iface, cfg, 0), VPX_CODEC_OK);
+
+ ASSERT_EQ(vpx_codec_control_(enc, VP8E_SET_CPUUSED, 2), VPX_CODEC_OK);
+}
+
+// Encodes 1 frame of size |cfg.g_w| x |cfg.g_h| setting |enc|'s configuration
+// to |cfg|.
+void EncodeWithConfig(const vpx_codec_enc_cfg_t &cfg, vpx_codec_ctx_t *enc) {
+ libvpx_test::DummyVideoSource video;
+ video.SetSize(cfg.g_w, cfg.g_h);
+ video.Begin();
+ EXPECT_EQ(vpx_codec_enc_config_set(enc, &cfg), VPX_CODEC_OK)
+ << vpx_codec_error_detail(enc);
+
+ EXPECT_EQ(vpx_codec_encode(enc, video.img(), video.pts(), video.duration(),
+ /*flags=*/0, VPX_DL_GOOD_QUALITY),
+ VPX_CODEC_OK)
+ << vpx_codec_error_detail(enc);
+}
+
+TEST(EncodeAPI, ConfigChangeThreadCount) {
+ constexpr int kWidth = 1920;
+ constexpr int kHeight = 1080;
+
+ for (const auto *iface : kCodecIfaces) {
+ SCOPED_TRACE(vpx_codec_iface_name(iface));
+ for (int i = 0; i < (IsVP9(iface) ? 2 : 1); ++i) {
+ vpx_codec_enc_cfg_t cfg;
+ struct Encoder {
+ ~Encoder() { EXPECT_EQ(vpx_codec_destroy(&ctx), VPX_CODEC_OK); }
+ vpx_codec_ctx_t ctx = {};
+ } enc;
+
+ EXPECT_NO_FATAL_FAILURE(
+ InitCodec(*iface, kWidth, kHeight, &enc.ctx, &cfg));
+ if (IsVP9(iface)) {
+ EXPECT_EQ(vpx_codec_control_(&enc.ctx, VP9E_SET_TILE_COLUMNS, 6),
+ VPX_CODEC_OK);
+ EXPECT_EQ(vpx_codec_control_(&enc.ctx, VP9E_SET_ROW_MT, i),
+ VPX_CODEC_OK);
+ }
+
+ for (const auto threads : { 1, 4, 8, 6, 2, 1 }) {
+ cfg.g_threads = threads;
+ EXPECT_NO_FATAL_FAILURE(EncodeWithConfig(cfg, &enc.ctx))
+ << "iteration: " << i << " threads: " << threads;
+ }
+ }
+ }
+}
+
} // namespace
diff --git a/vp9/encoder/vp9_encoder.c b/vp9/encoder/vp9_encoder.c
index 7e80835..8fdd869 100644
--- a/vp9/encoder/vp9_encoder.c
+++ b/vp9/encoder/vp9_encoder.c
@@ -2676,7 +2676,6 @@
void vp9_remove_compressor(VP9_COMP *cpi) {
VP9_COMMON *cm;
unsigned int i;
- int t;
if (!cpi) return;
@@ -2789,28 +2788,10 @@
free_tpl_buffer(cpi);
- for (t = 0; t < cpi->num_workers; ++t) {
- VPxWorker *const worker = &cpi->workers[t];
- EncWorkerData *const thread_data = &cpi->tile_thr_data[t];
-
- // Deallocate allocated threads.
- vpx_get_worker_interface()->end(worker);
-
- // Deallocate allocated thread data.
- if (t < cpi->num_workers - 1) {
- vpx_free(thread_data->td->counts);
- vp9_free_pc_tree(thread_data->td);
- vpx_free(thread_data->td);
- }
- }
- vpx_free(cpi->tile_thr_data);
- vpx_free(cpi->workers);
+ vp9_loop_filter_dealloc(&cpi->lf_row_sync);
+ vp9_bitstream_encode_tiles_buffer_dealloc(cpi);
vp9_row_mt_mem_dealloc(cpi);
-
- if (cpi->num_workers > 1) {
- vp9_loop_filter_dealloc(&cpi->lf_row_sync);
- vp9_bitstream_encode_tiles_buffer_dealloc(cpi);
- }
+ vp9_encode_free_mt_data(cpi);
#if !CONFIG_REALTIME_ONLY
vp9_alt_ref_aq_destroy(cpi->alt_ref_aq);
diff --git a/vp9/encoder/vp9_ethread.c b/vp9/encoder/vp9_ethread.c
index e7f8a53..453fe2e 100644
--- a/vp9/encoder/vp9_ethread.c
+++ b/vp9/encoder/vp9_ethread.c
@@ -8,6 +8,8 @@
* be found in the AUTHORS file in the root of the source tree.
*/
+#include "vp9/common/vp9_thread_common.h"
+#include "vp9/encoder/vp9_bitstream.h"
#include "vp9/encoder/vp9_encodeframe.h"
#include "vp9/encoder/vp9_encoder.h"
#include "vp9/encoder/vp9_ethread.h"
@@ -79,60 +81,59 @@
VP9_COMMON *const cm = &cpi->common;
const VPxWorkerInterface *const winterface = vpx_get_worker_interface();
int i;
+ // While using SVC, we need to allocate threads according to the highest
+ // resolution. When row based multithreading is enabled, it is OK to
+ // allocate more threads than the number of max tile columns.
+ if (cpi->use_svc && !cpi->row_mt) {
+ int max_tile_cols = get_max_tile_cols(cpi);
+ num_workers = VPXMIN(cpi->oxcf.max_threads, max_tile_cols);
+ }
+ assert(num_workers > 0);
+ if (num_workers == cpi->num_workers) return;
+ vp9_loop_filter_dealloc(&cpi->lf_row_sync);
+ vp9_bitstream_encode_tiles_buffer_dealloc(cpi);
+ vp9_encode_free_mt_data(cpi);
- // Only run once to create threads and allocate thread data.
- if (cpi->num_workers == 0) {
- int allocated_workers = num_workers;
+ CHECK_MEM_ERROR(cm, cpi->workers,
+ vpx_malloc(num_workers * sizeof(*cpi->workers)));
- // While using SVC, we need to allocate threads according to the highest
- // resolution. When row based multithreading is enabled, it is OK to
- // allocate more threads than the number of max tile columns.
- if (cpi->use_svc && !cpi->row_mt) {
- int max_tile_cols = get_max_tile_cols(cpi);
- allocated_workers = VPXMIN(cpi->oxcf.max_threads, max_tile_cols);
+ CHECK_MEM_ERROR(cm, cpi->tile_thr_data,
+ vpx_calloc(num_workers, sizeof(*cpi->tile_thr_data)));
+
+ for (i = 0; i < num_workers; i++) {
+ VPxWorker *const worker = &cpi->workers[i];
+ EncWorkerData *thread_data = &cpi->tile_thr_data[i];
+
+ ++cpi->num_workers;
+ winterface->init(worker);
+
+ if (i < num_workers - 1) {
+ thread_data->cpi = cpi;
+
+ // Allocate thread data.
+ CHECK_MEM_ERROR(cm, thread_data->td,
+ vpx_memalign(32, sizeof(*thread_data->td)));
+ vp9_zero(*thread_data->td);
+
+ // Set up pc_tree.
+ thread_data->td->leaf_tree = NULL;
+ thread_data->td->pc_tree = NULL;
+ vp9_setup_pc_tree(cm, thread_data->td);
+
+ // Allocate frame counters in thread data.
+ CHECK_MEM_ERROR(cm, thread_data->td->counts,
+ vpx_calloc(1, sizeof(*thread_data->td->counts)));
+
+ // Create threads
+ if (!winterface->reset(worker))
+ vpx_internal_error(&cm->error, VPX_CODEC_ERROR,
+ "Tile encoder thread creation failed");
+ } else {
+ // Main thread acts as a worker and uses the thread data in cpi.
+ thread_data->cpi = cpi;
+ thread_data->td = &cpi->td;
}
-
- CHECK_MEM_ERROR(cm, cpi->workers,
- vpx_malloc(allocated_workers * sizeof(*cpi->workers)));
-
- CHECK_MEM_ERROR(cm, cpi->tile_thr_data,
- vpx_calloc(allocated_workers, sizeof(*cpi->tile_thr_data)));
-
- for (i = 0; i < allocated_workers; i++) {
- VPxWorker *const worker = &cpi->workers[i];
- EncWorkerData *thread_data = &cpi->tile_thr_data[i];
-
- ++cpi->num_workers;
- winterface->init(worker);
-
- if (i < allocated_workers - 1) {
- thread_data->cpi = cpi;
-
- // Allocate thread data.
- CHECK_MEM_ERROR(cm, thread_data->td,
- vpx_memalign(32, sizeof(*thread_data->td)));
- vp9_zero(*thread_data->td);
-
- // Set up pc_tree.
- thread_data->td->leaf_tree = NULL;
- thread_data->td->pc_tree = NULL;
- vp9_setup_pc_tree(cm, thread_data->td);
-
- // Allocate frame counters in thread data.
- CHECK_MEM_ERROR(cm, thread_data->td->counts,
- vpx_calloc(1, sizeof(*thread_data->td->counts)));
-
- // Create threads
- if (!winterface->reset(worker))
- vpx_internal_error(&cm->error, VPX_CODEC_ERROR,
- "Tile encoder thread creation failed");
- } else {
- // Main thread acts as a worker and uses the thread data in cpi.
- thread_data->cpi = cpi;
- thread_data->td = &cpi->td;
- }
- winterface->sync(worker);
- }
+ winterface->sync(worker);
}
}
@@ -169,6 +170,27 @@
}
}
+void vp9_encode_free_mt_data(struct VP9_COMP *cpi) {
+ int t;
+ for (t = 0; t < cpi->num_workers; ++t) {
+ VPxWorker *const worker = &cpi->workers[t];
+ EncWorkerData *const thread_data = &cpi->tile_thr_data[t];
+
+ // Deallocate allocated threads.
+ vpx_get_worker_interface()->end(worker);
+
+ // Deallocate allocated thread data.
+ if (t < cpi->num_workers - 1) {
+ vpx_free(thread_data->td->counts);
+ vp9_free_pc_tree(thread_data->td);
+ vpx_free(thread_data->td);
+ }
+ }
+ vpx_free(cpi->tile_thr_data);
+ vpx_free(cpi->workers);
+ cpi->num_workers = 0;
+}
+
void vp9_encode_tiles_mt(VP9_COMP *cpi) {
VP9_COMMON *const cm = &cpi->common;
const int tile_cols = 1 << cm->log2_tile_cols;
diff --git a/vp9/encoder/vp9_ethread.h b/vp9/encoder/vp9_ethread.h
index cda0293..4c192da 100644
--- a/vp9/encoder/vp9_ethread.h
+++ b/vp9/encoder/vp9_ethread.h
@@ -42,6 +42,11 @@
int rows;
} VP9RowMTSync;
+// Frees EncWorkerData related allocations made by vp9_encode_*_mt().
+// row_mt specific data is freed with vp9_row_mt_mem_dealloc() and is not
+// called by this function.
+void vp9_encode_free_mt_data(struct VP9_COMP *cpi);
+
void vp9_encode_tiles_mt(struct VP9_COMP *cpi);
void vp9_encode_tiles_row_mt(struct VP9_COMP *cpi);