Cleanup aom_codec_control function

Add documentation, fix inconsistency with the sentinel value

BUG=aomedia:2651

Change-Id: I000712b5131658ed23ea64bfad29a47dd4ead412

aom/internal/aom_codec_internal.h

diff --git a/aom/internal/aom_codec_internal.h b/aom/internal/aom_codec_internal.h
index 0096003..efebd17 100644
--- a/aom/internal/aom_codec_internal.h
+++ b/aom/internal/aom_codec_internal.h
@@ -157,18 +157,24 @@
  *
  * This structure stores the mapping between control identifiers and
  * implementing functions. Each algorithm provides a list of these
- * mappings. This list is searched by the aom_codec_control() wrapper
+ * mappings. This list is searched by the aom_codec_control()
  * function to determine which function to invoke. The special
- * value {0, NULL} is used to indicate end-of-list, and must be
- * present. The special value {0, <non-null>} can be used as a catch-all
- * mapping. This implies that ctrl_id values chosen by the algorithm
- * \ref MUST be non-zero.
+ * value defined by CTRL_MAP_END is used to indicate end-of-list, and must be
+ * present. It can be tested with the at_ctrl_map_end function. Note that
+ * ctrl_id values \ref MUST be non-zero.
  */
 typedef const struct aom_codec_ctrl_fn_map {
   int ctrl_id;
   aom_codec_control_fn_t fn;
 } aom_codec_ctrl_fn_map_t;
 
+#define CTRL_MAP_END \
+  { 0, NULL }
+
+static AOM_INLINE int at_ctrl_map_end(aom_codec_ctrl_fn_map_t *e) {
+  return e->ctrl_id == 0 && e->fn == NULL;
+}
+
 /*!\brief decode data function pointer prototype
  *
  * Processes a buffer of coded data. This function is called by the generic

aom/src/aom_codec.c

diff --git a/aom/src/aom_codec.c b/aom/src/aom_codec.c
index 196ab83..d418463 100644
--- a/aom/src/aom_codec.c
+++ b/aom/src/aom_codec.c
@@ -22,8 +22,6 @@
 #include "aom/aom_integer.h"
 #include "aom/internal/aom_codec_internal.h"
 
-#define SAVE_STATUS(ctx, var) (ctx ? (ctx->err = var) : var)
-
 int aom_codec_version(void) { return VERSION_PACKED; }
 
 const char *aom_codec_version_str(void) { return VERSION_STRING_NOSP; }
@@ -67,22 +65,19 @@
 }
 
 aom_codec_err_t aom_codec_destroy(aom_codec_ctx_t *ctx) {
-  aom_codec_err_t res;
-
-  if (!ctx)
-    res = AOM_CODEC_INVALID_PARAM;
-  else if (!ctx->iface || !ctx->priv)
-    res = AOM_CODEC_ERROR;
-  else {
-    ctx->iface->destroy((aom_codec_alg_priv_t *)ctx->priv);
-
-    ctx->iface = NULL;
-    ctx->name = NULL;
-    ctx->priv = NULL;
-    res = AOM_CODEC_OK;
+  if (!ctx) {
+    return AOM_CODEC_INVALID_PARAM;
   }
-
-  return SAVE_STATUS(ctx, res);
+  if (!ctx->iface || !ctx->priv) {
+    ctx->err = AOM_CODEC_ERROR;
+    return AOM_CODEC_ERROR;
+  }
+  ctx->iface->destroy((aom_codec_alg_priv_t *)ctx->priv);
+  ctx->iface = NULL;
+  ctx->name = NULL;
+  ctx->priv = NULL;
+  ctx->err = AOM_CODEC_OK;
+  return AOM_CODEC_OK;
 }
 
 aom_codec_caps_t aom_codec_get_caps(aom_codec_iface_t *iface) {
@@ -90,30 +85,33 @@
 }
 
 aom_codec_err_t aom_codec_control(aom_codec_ctx_t *ctx, int ctrl_id, ...) {
-  aom_codec_err_t res;
-
-  if (!ctx || !ctrl_id)
-    res = AOM_CODEC_INVALID_PARAM;
-  else if (!ctx->iface || !ctx->priv || !ctx->iface->ctrl_maps)
-    res = AOM_CODEC_ERROR;
-  else {
-    aom_codec_ctrl_fn_map_t *entry;
-
-    res = AOM_CODEC_ERROR;
-
-    for (entry = ctx->iface->ctrl_maps; entry && entry->fn; entry++) {
-      if (!entry->ctrl_id || entry->ctrl_id == ctrl_id) {
-        va_list ap;
-
-        va_start(ap, ctrl_id);
-        res = entry->fn((aom_codec_alg_priv_t *)ctx->priv, ap);
-        va_end(ap);
-        break;
-      }
-    }
+  if (!ctx) {
+    return AOM_CODEC_INVALID_PARAM;
+  }
+  // Control ID must be non-zero.
+  if (!ctrl_id) {
+    ctx->err = AOM_CODEC_INVALID_PARAM;
+    return AOM_CODEC_INVALID_PARAM;
+  }
+  if (!ctx->iface || !ctx->priv || !ctx->iface->ctrl_maps) {
+    ctx->err = AOM_CODEC_ERROR;
+    return AOM_CODEC_ERROR;
   }
 
-  return SAVE_STATUS(ctx, res);
+  // "ctrl_maps" is an array of (control ID, function pointer) elements,
+  // with CTRL_MAP_END as a sentinel.
+  for (aom_codec_ctrl_fn_map_t *entry = ctx->iface->ctrl_maps;
+       !at_ctrl_map_end(entry); ++entry) {
+    if (entry->ctrl_id == ctrl_id) {
+      va_list ap;
+      va_start(ap, ctrl_id);
+      ctx->err = entry->fn((aom_codec_alg_priv_t *)ctx->priv, ap);
+      va_end(ap);
+      return ctx->err;
+    }
+  }
+  ctx->err = AOM_CODEC_ERROR;
+  return AOM_CODEC_ERROR;
 }
 
 void aom_internal_error(struct aom_internal_error_info *info,

av1/av1_cx_iface.c

diff --git a/av1/av1_cx_iface.c b/av1/av1_cx_iface.c
index ee4ab09..5d9bc1c 100644
--- a/av1/av1_cx_iface.c
+++ b/av1/av1_cx_iface.c
@@ -2783,7 +2783,8 @@
   { AV1E_SET_CHROMA_SUBSAMPLING_X, ctrl_set_chroma_subsampling_x },
   { AV1E_SET_CHROMA_SUBSAMPLING_Y, ctrl_set_chroma_subsampling_y },
   { AV1E_GET_SEQ_LEVEL_IDX, ctrl_get_seq_level_idx },
-  { -1, NULL },
+
+  CTRL_MAP_END,
 };
 
 static const aom_codec_enc_cfg_t encoder_usage_cfg[] = {

av1/av1_dx_iface.c

diff --git a/av1/av1_dx_iface.c b/av1/av1_dx_iface.c
index b5d856e..e621cd2 100644
--- a/av1/av1_dx_iface.c
+++ b/av1/av1_dx_iface.c
@@ -1362,7 +1362,7 @@
   { AV1D_GET_FRAME_HEADER_INFO, ctrl_get_frame_header_info },
   { AV1D_GET_TILE_DATA, ctrl_get_tile_data },
 
-  { -1, NULL },
+  CTRL_MAP_END,
 };
 
 // This data structure and function are exported in aom/aomdx.h

test/encode_api_test.cc

diff --git a/test/encode_api_test.cc b/test/encode_api_test.cc
index c1f2932..4b79f25 100644
--- a/test/encode_api_test.cc
+++ b/test/encode_api_test.cc
@@ -21,11 +21,6 @@
 namespace {
 
 TEST(EncodeAPI, InvalidParams) {
-  static aom_codec_iface_t *kCodecs[] = {
-#if CONFIG_AV1_ENCODER
-    aom_codec_av1_cx(),
-#endif
-  };
   uint8_t buf[1] = { 0 };
   aom_image_t img;
   aom_codec_ctx_t enc;
@@ -44,31 +39,35 @@
             aom_codec_enc_config_default(NULL, &cfg, 0));
   EXPECT_TRUE(aom_codec_error(NULL) != NULL);
 
-  for (aom_codec_iface_t *iface : kCodecs) {
-    SCOPED_TRACE(aom_codec_iface_name(iface));
-    EXPECT_EQ(AOM_CODEC_INVALID_PARAM,
-              aom_codec_enc_init(NULL, iface, NULL, 0));
-    EXPECT_EQ(AOM_CODEC_INVALID_PARAM,
-              aom_codec_enc_init(&enc, iface, NULL, 0));
-    EXPECT_EQ(AOM_CODEC_INVALID_PARAM,
-              aom_codec_enc_config_default(iface, &cfg, 2));
+  aom_codec_iface_t *iface = aom_codec_av1_cx();
+  SCOPED_TRACE(aom_codec_iface_name(iface));
+  EXPECT_EQ(AOM_CODEC_INVALID_PARAM, aom_codec_enc_init(NULL, iface, NULL, 0));
+  EXPECT_EQ(AOM_CODEC_INVALID_PARAM, aom_codec_enc_init(&enc, iface, NULL, 0));
+  EXPECT_EQ(AOM_CODEC_INVALID_PARAM,
+            aom_codec_enc_config_default(iface, &cfg, 2));
+  EXPECT_EQ(AOM_CODEC_OK, aom_codec_enc_config_default(iface, &cfg, 0));
+  EXPECT_EQ(AOM_CODEC_OK, aom_codec_enc_init(&enc, iface, &cfg, 0));
+  EXPECT_EQ(NULL, aom_codec_get_global_headers(NULL));
 
-    EXPECT_EQ(AOM_CODEC_OK, aom_codec_enc_config_default(iface, &cfg, 0));
-    EXPECT_EQ(AOM_CODEC_OK, aom_codec_enc_init(&enc, iface, &cfg, 0));
-
-    EXPECT_EQ(NULL, aom_codec_get_global_headers(NULL));
-
-    aom_fixed_buf_t *glob_headers = aom_codec_get_global_headers(&enc);
-    EXPECT_TRUE(glob_headers->buf != NULL);
-    if (glob_headers) {
-      free(glob_headers->buf);
-      free(glob_headers);
-    }
-
-    EXPECT_EQ(AOM_CODEC_OK, aom_codec_encode(&enc, NULL, 0, 0, 0));
-
-    EXPECT_EQ(AOM_CODEC_OK, aom_codec_destroy(&enc));
+  aom_fixed_buf_t *glob_headers = aom_codec_get_global_headers(&enc);
+  EXPECT_TRUE(glob_headers->buf != NULL);
+  if (glob_headers) {
+    free(glob_headers->buf);
+    free(glob_headers);
   }
+  EXPECT_EQ(AOM_CODEC_OK, aom_codec_encode(&enc, NULL, 0, 0, 0));
+  EXPECT_EQ(AOM_CODEC_OK, aom_codec_destroy(&enc));
+}
+
+TEST(EncodeAPI, InvalidControlId) {
+  aom_codec_iface_t *iface = aom_codec_av1_cx();
+  aom_codec_ctx_t enc;
+  aom_codec_enc_cfg_t cfg;
+  EXPECT_EQ(AOM_CODEC_OK, aom_codec_enc_config_default(iface, &cfg, 0));
+  EXPECT_EQ(AOM_CODEC_OK, aom_codec_enc_init(&enc, iface, &cfg, 0));
+  EXPECT_EQ(AOM_CODEC_ERROR, aom_codec_control(&enc, -1, 0));
+  EXPECT_EQ(AOM_CODEC_INVALID_PARAM, aom_codec_control(&enc, 0, 0));
+  EXPECT_EQ(AOM_CODEC_OK, aom_codec_destroy(&enc));
 }
 
 }  // namespace