Fix a bug where an uninitalized search_site is used
BUG=aomedia:3348
Change-Id: I0a7c84b61f6a4588b1eda63fff259fa158ddb58d
diff --git a/av1/encoder/mcomp.c b/av1/encoder/mcomp.c
index 2dbea2b..dcf98cb 100644
--- a/av1/encoder/mcomp.c
+++ b/av1/encoder/mcomp.c
@@ -94,11 +94,10 @@
void av1_make_default_fullpel_ms_params(
FULLPEL_MOTION_SEARCH_PARAMS *ms_params, const struct AV1_COMP *cpi,
- const MACROBLOCK *x, BLOCK_SIZE bsize, const MV *ref_mv,
+ MACROBLOCK *x, BLOCK_SIZE bsize, const MV *ref_mv,
const search_site_config search_sites[NUM_DISTINCT_SEARCH_METHODS],
int fine_search_interval) {
const MV_SPEED_FEATURES *mv_sf = &cpi->sf.mv_sf;
- const int sf_blk_search_method = mv_sf->use_bsize_dependent_search_method;
// High level params
ms_params->bsize = bsize;
@@ -107,14 +106,25 @@
init_ms_buffers(&ms_params->ms_buffers, x);
SEARCH_METHODS search_method = mv_sf->search_method;
+ const int sf_blk_search_method = mv_sf->use_bsize_dependent_search_method;
const int min_dim = AOMMIN(block_size_wide[bsize], block_size_high[bsize]);
- if (sf_blk_search_method >= 2) {
- const int qband = x->qindex >> (QINDEX_BITS - 2);
- if (min_dim >= 16 && x->content_state_sb.source_sad_nonrd <= kMedSad &&
- qband < 3)
- search_method = get_faster_search_method(search_method);
- } else if (sf_blk_search_method >= 1 && min_dim >= 32) {
+ const int qband = x->qindex >> (QINDEX_BITS - 2);
+ const bool use_faster_search_method =
+ (sf_blk_search_method == 1 && min_dim >= 32) ||
+ (sf_blk_search_method >= 2 && min_dim >= 16 &&
+ x->content_state_sb.source_sad_nonrd <= kMedSad && qband < 3);
+
+ if (use_faster_search_method) {
search_method = get_faster_search_method(search_method);
+
+ // We might need to update the search site config since search_method
+ // is changed here.
+ const int ref_stride = ms_params->ms_buffers.ref->stride;
+ if (ref_stride != search_sites[search_method].stride) {
+ av1_refresh_search_site_config(x->search_site_cfg_buf, search_method,
+ ref_stride);
+ search_sites = x->search_site_cfg_buf;
+ }
}
av1_set_mv_search_method(ms_params, search_sites, search_method);
diff --git a/av1/encoder/mcomp.h b/av1/encoder/mcomp.h
index 5aba8d9..1e8bbab 100644
--- a/av1/encoder/mcomp.h
+++ b/av1/encoder/mcomp.h
@@ -144,7 +144,7 @@
void av1_make_default_fullpel_ms_params(
FULLPEL_MOTION_SEARCH_PARAMS *ms_params, const struct AV1_COMP *cpi,
- const MACROBLOCK *x, BLOCK_SIZE bsize, const MV *ref_mv,
+ MACROBLOCK *x, BLOCK_SIZE bsize, const MV *ref_mv,
const search_site_config search_sites[NUM_DISTINCT_SEARCH_METHODS],
int fine_search_interval);
@@ -201,6 +201,17 @@
BIGDIA // VFAST_DIAMOND
};
+// Reinitialize the search site config.
+static AOM_INLINE void av1_refresh_search_site_config(
+ search_site_config *ss_cfg_buf, SEARCH_METHODS search_method,
+ const int ref_stride) {
+ const int level =
+ search_method == NSTEP_8PT || search_method == CLAMPED_DIAMOND;
+ search_method = search_method_lookup[search_method];
+ av1_init_motion_compensation[search_method](&ss_cfg_buf[search_method],
+ ref_stride, level);
+}
+
// Mv beyond the range do not produce new/different prediction block.
static INLINE void av1_set_mv_search_method(
FULLPEL_MOTION_SEARCH_PARAMS *ms_params,
diff --git a/av1/encoder/motion_search_facade.c b/av1/encoder/motion_search_facade.c
index d232f27..e452b02 100644
--- a/av1/encoder/motion_search_facade.c
+++ b/av1/encoder/motion_search_facade.c
@@ -221,9 +221,8 @@
// MotionVectorSearchParams::search_site_cfg. When this happens, we need to
// readjust the stride.
const SEARCH_METHODS search_method = cpi->sf.mv_sf.search_method;
- const int ref_stride = xd->plane[0].pre[0].stride;
- const search_site_config *src_search_site_cfg = av1_get_search_site_config(
- x->search_site_cfg_buf, mv_search_params, search_method, ref_stride);
+ const search_site_config *src_search_site_cfg =
+ av1_get_search_site_config(cpi, x, search_method);
// Further reduce the search range.
if (search_range < INT_MAX) {
@@ -597,10 +596,8 @@
// Make motion search params
FULLPEL_MOTION_SEARCH_PARAMS full_ms_params;
const SEARCH_METHODS search_method = cpi->sf.mv_sf.search_method;
- const int ref_stride = xd->plane[0].pre[0].stride;
- const search_site_config *src_search_sites = av1_get_search_site_config(
- x->search_site_cfg_buf, &cpi->mv_search_params, search_method,
- ref_stride);
+ const search_site_config *src_search_sites =
+ av1_get_search_site_config(cpi, x, search_method);
av1_make_default_fullpel_ms_params(&full_ms_params, cpi, x, bsize,
&ref_mv[id].as_mv, src_search_sites,
/*fine_search_interval=*/0);
@@ -752,10 +749,8 @@
// Make motion search params
FULLPEL_MOTION_SEARCH_PARAMS full_ms_params;
const SEARCH_METHODS search_method = cpi->sf.mv_sf.search_method;
- const int ref_stride = xd->plane[0].pre[0].stride;
const search_site_config *src_search_sites =
- av1_get_search_site_config(x->search_site_cfg_buf, &cpi->mv_search_params,
- search_method, ref_stride);
+ av1_get_search_site_config(cpi, x, search_method);
av1_make_default_fullpel_ms_params(&full_ms_params, cpi, x, bsize,
&ref_mv.as_mv, src_search_sites,
/*fine_search_interval=*/0);
@@ -974,10 +969,8 @@
const int fine_search_interval = use_fine_search_interval(cpi);
FULLPEL_MOTION_SEARCH_PARAMS full_ms_params;
const SEARCH_METHODS search_method = cpi->sf.mv_sf.search_method;
- const int ref_stride = xd->plane[0].pre[0].stride;
const search_site_config *src_search_sites =
- av1_get_search_site_config(x->search_site_cfg_buf, &cpi->mv_search_params,
- search_method, ref_stride);
+ av1_get_search_site_config(cpi, x, search_method);
av1_make_default_fullpel_ms_params(&full_ms_params, cpi, x, bsize, &ref_mv,
src_search_sites, fine_search_interval);
diff --git a/av1/encoder/motion_search_facade.h b/av1/encoder/motion_search_facade.h
index bc69a65..4d76287 100644
--- a/av1/encoder/motion_search_facade.h
+++ b/av1/encoder/motion_search_facade.h
@@ -71,9 +71,13 @@
unsigned int *sse, unsigned int *var);
static AOM_INLINE const search_site_config *av1_get_search_site_config(
- search_site_config *ss_cfg_buf,
- const MotionVectorSearchParams *mv_search_params,
- SEARCH_METHODS search_method, const int ref_stride) {
+ const AV1_COMP *cpi, MACROBLOCK *x, SEARCH_METHODS search_method) {
+ const int ref_stride = x->e_mbd.plane[0].pre[0].stride;
+
+ // AV1_COMP::mv_search_params.search_site_config is a compressor level cache
+ // that's shared by multiple threads. In most cases where all frames have the
+ // same resolution, the cache contains the search site config that we need.
+ const MotionVectorSearchParams *mv_search_params = &cpi->mv_search_params;
if (ref_stride == mv_search_params->search_site_cfg[SS_CFG_SRC]->stride) {
return mv_search_params->search_site_cfg[SS_CFG_SRC];
} else if (ref_stride ==
@@ -81,15 +85,18 @@
return mv_search_params->search_site_cfg[SS_CFG_LOOKAHEAD];
}
- if (ref_stride != ss_cfg_buf[search_method].stride) {
- const int level =
- search_method == NSTEP_8PT || search_method == CLAMPED_DIAMOND;
- search_method = search_method_lookup[search_method];
- av1_init_motion_compensation[search_method](&ss_cfg_buf[search_method],
- ref_stride, level);
+ // If the cache does not contain the correct stride, then we will need to rely
+ // on the thread level config MACROBLOCK::search_site_cfg_buf. If even the
+ // thread level config doesn't match, then we need to update it.
+ search_method = search_method_lookup[search_method];
+ assert(search_method_lookup[search_method] == search_method &&
+ "The search_method_lookup table should be idempotent.");
+ if (ref_stride != x->search_site_cfg_buf[search_method].stride) {
+ av1_refresh_search_site_config(x->search_site_cfg_buf, search_method,
+ ref_stride);
}
- return ss_cfg_buf;
+ return x->search_site_cfg_buf;
}
#ifdef __cplusplus
diff --git a/av1/encoder/nonrd_pickmode.c b/av1/encoder/nonrd_pickmode.c
index 3bb32ca..497ac52 100644
--- a/av1/encoder/nonrd_pickmode.c
+++ b/av1/encoder/nonrd_pickmode.c
@@ -252,10 +252,8 @@
center_mv = tmp_mv->as_mv;
const SEARCH_METHODS search_method = sf->mv_sf.search_method;
- const MotionVectorSearchParams *mv_search_params = &cpi->mv_search_params;
- const int ref_stride = xd->plane[AOM_PLANE_Y].pre[0].stride;
- const search_site_config *src_search_sites = av1_get_search_site_config(
- x->search_site_cfg_buf, mv_search_params, search_method, ref_stride);
+ const search_site_config *src_search_sites =
+ av1_get_search_site_config(cpi, x, search_method);
FULLPEL_MOTION_SEARCH_PARAMS full_ms_params;
av1_make_default_fullpel_ms_params(&full_ms_params, cpi, x, bsize, ¢er_mv,
src_search_sites,
diff --git a/av1/encoder/temporal_filter.c b/av1/encoder/temporal_filter.c
index 4cfb4e5..a1e4430 100644
--- a/av1/encoder/temporal_filter.c
+++ b/av1/encoder/temporal_filter.c
@@ -110,9 +110,6 @@
// Parameters used for motion search.
FULLPEL_MOTION_SEARCH_PARAMS full_ms_params;
SUBPEL_MOTION_SEARCH_PARAMS ms_params;
- const SEARCH_METHODS search_method = NSTEP;
- const search_site_config *search_site_cfg = av1_get_search_site_config(
- mb->search_site_cfg_buf, &cpi->mv_search_params, search_method, y_stride);
const int step_param = av1_init_search_range(
AOMMAX(frame_to_filter->y_crop_width, frame_to_filter->y_crop_height));
const SUBPEL_SEARCH_TYPE subpel_search_type = USE_8_TAPS;
@@ -132,6 +129,11 @@
mb->plane[0].src.stride = y_stride;
mbd->plane[0].pre[0].buf = ref_frame->y_buffer + y_offset;
mbd->plane[0].pre[0].stride = y_stride;
+
+ const SEARCH_METHODS search_method = NSTEP;
+ const search_site_config *search_site_cfg =
+ av1_get_search_site_config(cpi, mb, search_method);
+
// Unused intermediate results for motion search.
unsigned int sse, error;
int distortion;
diff --git a/test/avif_progressive_test.cc b/test/avif_progressive_test.cc
index 457891c..cf94fd5 100644
--- a/test/avif_progressive_test.cc
+++ b/test/avif_progressive_test.cc
@@ -103,9 +103,7 @@
// This test emulates how libavif calls libaom functions to encode a
// progressive AVIF image in libavif's ProgressiveTest.DimensionChange test.
-// TODO(https://crbug.com/aomedia/3348): Fix the assertion failure at
-// av1/encoder/mcomp.c:1760 and enable this test.
-TEST(AVIFProgressiveTest, DISABLED_DimensionChange) {
+TEST(AVIFProgressiveTest, DimensionChange) {
constexpr int kWidth = 256;
constexpr int kHeight = 256;
// Dummy buffer of neutral gray samples.
