Work around a spec bug in gm_get_motion_vector()
For TRANSLATION type global warp models, gm_get_motion_vector()
returns an incorrect motion vector. As this is a bug in the
specification, we cannot change this logic, but we can work around
it as follows:
* Add a comment explaining the bug
* Change the encoder logic so that we never select a TRANSLATION
type global motion model. If the global motion search process
produces a TRANSLATION type model, we reset to an IDENTITY
type model.
The impact of this change is ~neutral overall, with no video
changing by more than +/- 0.2% BDRATE.
Overall impact on 150 frames, speed 1, const q mode:
lowres2: -0.002% BDRATE, -0.003% instr count
midres2: +0.005% BDRATE, +0.005% instr count
hdres2: -0.001% BDRATE, -0.035% instr count
STATS_CHANGED
Bug: aomedia:3328
Change-Id: Ibbbefa5107739de0c8cdb867901994ef554daee7
diff --git a/av1/common/mv.h b/av1/common/mv.h
index c7eaf76..e70ce90 100644
--- a/av1/common/mv.h
+++ b/av1/common/mv.h
@@ -282,6 +282,17 @@
// After the right shifts, there are 3 fractional bits of precision. If
// allow_hp is false, the bottom bit is always zero (so we don't need a
// call to convert_to_trans_prec here)
+ //
+ // Note: There is an AV1 specification bug here:
+ //
+ // gm->wmmat[0] is supposed to be the horizontal translation, and so should
+ // go into res.as_mv.col, and gm->wmmat[1] is supposed to be the vertical
+ // translation and so should go into res.as_mv.row
+ //
+ // However, in the spec, these assignments are accidentally reversed, and so
+ // we must keep this incorrect logic to match the spec.
+ //
+ // See also: https://crbug.com/aomedia/3328
res.as_mv.row = gm->wmmat[0] >> GM_TRANS_ONLY_PREC_DIFF;
res.as_mv.col = gm->wmmat[1] >> GM_TRANS_ONLY_PREC_DIFF;
assert(IMPLIES(1 & (res.as_mv.row | res.as_mv.col), allow_hp));
diff --git a/av1/encoder/bitstream.c b/av1/encoder/bitstream.c
index 8b23e64..a9e9fdc 100644
--- a/av1/encoder/bitstream.c
+++ b/av1/encoder/bitstream.c
@@ -2681,6 +2681,12 @@
struct aom_write_bit_buffer *wb, int allow_hp) {
const TransformationType type = params->wmtype;
+ // As a workaround for an AV1 spec bug, we avoid choosing TRANSLATION
+ // type models. Check here that we don't accidentally pick one somehow.
+ // See comments in gm_get_motion_vector() for details on the bug we're
+ // working around here
+ assert(type != TRANSLATION);
+
aom_wb_write_bit(wb, type != IDENTITY);
if (type != IDENTITY) {
aom_wb_write_bit(wb, type == ROTZOOM);
diff --git a/av1/encoder/global_motion_facade.c b/av1/encoder/global_motion_facade.c
index 323b4e8..5cddc7e 100644
--- a/av1/encoder/global_motion_facade.c
+++ b/av1/encoder/global_motion_facade.c
@@ -133,6 +133,16 @@
params_this_motion = params_by_motion[i].params;
av1_convert_model_to_params(params_this_motion, &tmp_wm_params);
+ // Work around a bug in the AV1 specification
+ //
+ // For TRANSLATION type global motion models, gm_get_motion_vector() gives
+ // the wrong motion vector (see comments in that function for details).
+ // As translation-type models do not give much gain, we can avoid this bug
+ // by never choosing a TRANSLATION type model
+ if (tmp_wm_params.wmtype == TRANSLATION) {
+ continue;
+ }
+
if (tmp_wm_params.wmtype != IDENTITY) {
av1_compute_feature_segmentation_map(
segment_map, segment_map_w, segment_map_h,
@@ -154,6 +164,12 @@
GM_REFINEMENT_COUNT, best_warp_error, segment_map, segment_map_w,
erroradv_threshold);
+ // av1_refine_integerized_param() can return a TRANSLATION type model
+ // even if its input is some other type, so we have to skip those too
+ if (tmp_wm_params.wmtype == TRANSLATION) {
+ continue;
+ }
+
if (warp_error < best_warp_error) {
best_warp_error = warp_error;
// Save the wm_params modified by
@@ -168,6 +184,8 @@
if (!av1_get_shear_params(&cm->global_motion[frame]))
cm->global_motion[frame] = default_warp_params;
+#if 0
+ // We never choose translational models, so this code is disabled
if (cm->global_motion[frame].wmtype == TRANSLATION) {
cm->global_motion[frame].wmmat[0] =
convert_to_trans_prec(cm->features.allow_high_precision_mv,
@@ -178,6 +196,7 @@
cm->global_motion[frame].wmmat[1]) *
GM_TRANS_ONLY_DECODE_FACTOR;
}
+#endif
if (cm->global_motion[frame].wmtype == IDENTITY) continue;
