AAPT2: Multi APK generator by version

- Added an additional axis for generating a multi-apk split by minimum
Android SDK version. This removes any resources that will not be used
for the desired minimum SDK version. If there are multiple resources
that would be valid for any version newer than the requested minimum,
then all would be kept so that the best match can be found.

- Added a context wrapper to set the appropriate Android SDK version for
each generated artifact.

- Split out the FilterTable method to allow it to be directly tested
without the need to mock the APK writing steps.

Test: Unit tests
Test: manually run optimize command

Change-Id: I7e6018df081af9ed5d9e8aaf40ed216c1275f138

11 files changed, 439 insertions, 129 deletions

diff --git a/tools/aapt2/cmd/Optimize.cpp b/tools/aapt2/cmd/Optimize.cpp
index 887803e147da..33a1d3a704f1 100644
--- a/tools/aapt2/cmd/Optimize.cpp
+++ b/tools/aapt2/cmd/Optimize.cpp
@@ -194,8 +194,10 @@ class OptimizeCommand {
 
     if (options_.configuration && options_.output_dir) {
       MultiApkGenerator generator{apk.get(), context_};
-      if (!generator.FromBaseApk(options_.output_dir.value(), options_.configuration.value(),
-                                 options_.table_flattener_options)) {
+      MultiApkGeneratorOptions generator_options = {options_.output_dir.value(),
+                                                    options_.configuration.value(),
+                                                    options_.table_flattener_options};
+      if (!generator.FromBaseApk(generator_options)) {
         return 1;
       }
     }
diff --git a/tools/aapt2/configuration/ConfigurationParser.cpp b/tools/aapt2/configuration/ConfigurationParser.cpp
index 1735a504e553..9d6d3286f0ef 100644
--- a/tools/aapt2/configuration/ConfigurationParser.cpp
+++ b/tools/aapt2/configuration/ConfigurationParser.cpp
@@ -488,8 +488,8 @@ ConfigurationParser::ActionHandler ConfigurationParser::android_sdk_group_handle
     return false;
   }
 
-  auto& group = config->android_sdk_groups[label];
   bool valid = true;
+  bool found = false;
 
   for (auto* child : root_element->GetChildElements()) {
     if (child->name != "android-sdk") {
@@ -520,7 +520,11 @@ ConfigurationParser::ActionHandler ConfigurationParser::android_sdk_group_handle
         }
       }
 
-      group.push_back(entry);
+      config->android_sdk_groups[label] = entry;
+      if (found) {
+        valid = false;
+      }
+      found = true;
     }
   }
 
diff --git a/tools/aapt2/configuration/ConfigurationParser.h b/tools/aapt2/configuration/ConfigurationParser.h
index a58685e3b8a6..9bc9081b8ae4 100644
--- a/tools/aapt2/configuration/ConfigurationParser.h
+++ b/tools/aapt2/configuration/ConfigurationParser.h
@@ -33,6 +33,10 @@ namespace configuration {
 template<class T>
 using Group = std::unordered_map<std::string, std::vector<T>>;
 
+/** A mapping of group label to a single configuration item. */
+template <class T>
+using Entry = std::unordered_map<std::string, T>;
+
 /** Output artifact configuration options. */
 struct Artifact {
   /** Name to use for output of processing foo.apk -> foo.<name>.apk. */
@@ -104,6 +108,12 @@ struct AndroidSdk {
   Maybe<std::string> max_sdk_version;
   Maybe<AndroidManifest> manifest;
 
+  static AndroidSdk ForMinSdk(std::string min_sdk) {
+    AndroidSdk sdk;
+    sdk.min_sdk_version = {std::move(min_sdk)};
+    return sdk;
+  }
+
   inline friend bool operator==(const AndroidSdk& lhs, const AndroidSdk& rhs) {
     return lhs.min_sdk_version == rhs.min_sdk_version &&
         lhs.target_sdk_version == rhs.target_sdk_version &&
@@ -134,7 +144,7 @@ struct PostProcessingConfiguration {
   Group<Abi> abi_groups;
   Group<ConfigDescription> screen_density_groups;
   Group<ConfigDescription> locale_groups;
-  Group<AndroidSdk> android_sdk_groups;
+  Entry<AndroidSdk> android_sdk_groups;
   Group<DeviceFeature> device_feature_groups;
   Group<GlTexture> gl_texture_groups;
 
diff --git a/tools/aapt2/configuration/ConfigurationParser_test.cpp b/tools/aapt2/configuration/ConfigurationParser_test.cpp
index d3bfd330fe9d..7ffb3d515079 100644
--- a/tools/aapt2/configuration/ConfigurationParser_test.cpp
+++ b/tools/aapt2/configuration/ConfigurationParser_test.cpp
@@ -74,11 +74,11 @@ constexpr const char* kValidConfig = R"(<?xml version="1.0" encoding="utf-8" ?>
       <locale>es-rMX</locale>
       <locale>fr-rCA</locale>
     </locale-group>
-    <android-sdk-group label="19">
+    <android-sdk-group label="v19">
       <android-sdk
-          minSdkVersion="19"
-          targetSdkVersion="24"
-          maxSdkVersion="25">
+          minSdkVersion="v19"
+          targetSdkVersion="v24"
+          maxSdkVersion="v25">
         <manifest>
           <!--- manifest additions here XSLT? TODO -->
         </manifest>
@@ -102,7 +102,7 @@ constexpr const char* kValidConfig = R"(<?xml version="1.0" encoding="utf-8" ?>
         abi-group="arm"
         screen-density-group="large"
         locale-group="europe"
-        android-sdk-group="19"
+        android-sdk-group="v19"
         gl-texture-group="dxt1"
         device-feature-group="low-latency"/>
     <artifact
@@ -110,7 +110,7 @@ constexpr const char* kValidConfig = R"(<?xml version="1.0" encoding="utf-8" ?>
         abi-group="other"
         screen-density-group="alldpi"
         locale-group="north-america"
-        android-sdk-group="19"
+        android-sdk-group="v19"
         gl-texture-group="dxt1"
         device-feature-group="low-latency"/>
   </artifacts>
@@ -155,7 +155,8 @@ TEST_F(ConfigurationParserTest, ValidateFile) {
   EXPECT_EQ(3ul, value.locale_groups["north-america"].size());
 
   EXPECT_EQ(1ul, value.android_sdk_groups.size());
-  EXPECT_EQ(1ul, value.android_sdk_groups["19"].size());
+  EXPECT_TRUE(value.android_sdk_groups["v19"].min_sdk_version);
+  EXPECT_EQ("v19", value.android_sdk_groups["v19"].min_sdk_version.value());
 
   EXPECT_EQ(1ul, value.gl_texture_groups.size());
   EXPECT_EQ(1ul, value.gl_texture_groups["dxt1"].size());
@@ -178,7 +179,7 @@ TEST_F(ConfigurationParserTest, ArtifactAction) {
         abi-group="arm"
         screen-density-group="large"
         locale-group="europe"
-        android-sdk-group="19"
+        android-sdk-group="v19"
         gl-texture-group="dxt1"
         device-feature-group="low-latency"/>)xml";
 
@@ -195,7 +196,7 @@ TEST_F(ConfigurationParserTest, ArtifactAction) {
   EXPECT_EQ("arm", artifact.abi_group.value());
   EXPECT_EQ("large", artifact.screen_density_group.value());
   EXPECT_EQ("europe", artifact.locale_group.value());
-  EXPECT_EQ("19", artifact.android_sdk_group.value());
+  EXPECT_EQ("v19", artifact.android_sdk_group.value());
   EXPECT_EQ("dxt1", artifact.gl_texture_group.value());
   EXPECT_EQ("low-latency", artifact.device_feature_group.value());
 
@@ -205,7 +206,7 @@ TEST_F(ConfigurationParserTest, ArtifactAction) {
         abi-group="other"
         screen-density-group="large"
         locale-group="europe"
-        android-sdk-group="19"
+        android-sdk-group="v19"
         gl-texture-group="dxt1"
         device-feature-group="low-latency"/>)xml";
   doc = test::BuildXmlDom(second);
@@ -318,11 +319,11 @@ TEST_F(ConfigurationParserTest, LocaleGroupAction) {
 
 TEST_F(ConfigurationParserTest, AndroidSdkGroupAction) {
   static constexpr const char* xml = R"xml(
-    <android-sdk-group label="19">
+    <android-sdk-group label="v19">
       <android-sdk
-          minSdkVersion="19"
-          targetSdkVersion="24"
-          maxSdkVersion="25">
+          minSdkVersion="v19"
+          targetSdkVersion="v24"
+          maxSdkVersion="v25">
         <manifest>
           <!--- manifest additions here XSLT? TODO -->
         </manifest>
@@ -336,18 +337,17 @@ TEST_F(ConfigurationParserTest, AndroidSdkGroupAction) {
   ASSERT_TRUE(ok);
 
   ASSERT_EQ(1ul, config.android_sdk_groups.size());
-  ASSERT_EQ(1u, config.android_sdk_groups.count("19"));
+  ASSERT_EQ(1u, config.android_sdk_groups.count("v19"));
 
-  auto& out = config.android_sdk_groups["19"];
+  auto& out = config.android_sdk_groups["v19"];
 
   AndroidSdk sdk;
-  sdk.min_sdk_version = std::string("19");
-  sdk.target_sdk_version = std::string("24");
-  sdk.max_sdk_version = std::string("25");
+  sdk.min_sdk_version = std::string("v19");
+  sdk.target_sdk_version = std::string("v24");
+  sdk.max_sdk_version = std::string("v25");
   sdk.manifest = AndroidManifest();
 
-  ASSERT_EQ(1ul, out.size());
-  ASSERT_EQ(sdk, out[0]);
+  ASSERT_EQ(sdk, out);
 }
 
 TEST_F(ConfigurationParserTest, GlTextureGroupAction) {
@@ -454,41 +454,41 @@ TEST(ArtifactTest, Complex) {
   artifact.device_feature_group = {"df1"};
   artifact.gl_texture_group = {"glx1"};
   artifact.locale_group = {"en-AU"};
-  artifact.android_sdk_group = {"26"};
+  artifact.android_sdk_group = {"v26"};
 
   {
     auto result = artifact.ToArtifactName(
-        "app.${density}_${locale}_${feature}_${gl}.sdk${sdk}.${abi}.apk", "", &diag);
+        "app.${density}_${locale}_${feature}_${gl}.${sdk}.${abi}.apk", "", &diag);
     ASSERT_TRUE(result);
-    EXPECT_EQ(result.value(), "app.ldpi_en-AU_df1_glx1.sdk26.mips64.apk");
+    EXPECT_EQ(result.value(), "app.ldpi_en-AU_df1_glx1.v26.mips64.apk");
   }
 
   {
     auto result = artifact.ToArtifactName(
-        "app.${density}_${locale}_${feature}_${gl}.sdk${sdk}.${abi}.apk", "app.apk", &diag);
+        "app.${density}_${locale}_${feature}_${gl}.${sdk}.${abi}.apk", "app.apk", &diag);
     ASSERT_TRUE(result);
-    EXPECT_EQ(result.value(), "app.ldpi_en-AU_df1_glx1.sdk26.mips64.apk");
+    EXPECT_EQ(result.value(), "app.ldpi_en-AU_df1_glx1.v26.mips64.apk");
   }
 
   {
     auto result = artifact.ToArtifactName(
-        "${basename}.${density}_${locale}_${feature}_${gl}.sdk${sdk}.${abi}.apk", "app.apk", &diag);
+        "${basename}.${density}_${locale}_${feature}_${gl}.${sdk}.${abi}.apk", "app.apk", &diag);
     ASSERT_TRUE(result);
-    EXPECT_EQ(result.value(), "app.ldpi_en-AU_df1_glx1.sdk26.mips64.apk");
+    EXPECT_EQ(result.value(), "app.ldpi_en-AU_df1_glx1.v26.mips64.apk");
   }
 
   {
     auto result = artifact.ToArtifactName(
-        "app.${density}_${locale}_${feature}_${gl}.sdk${sdk}.${abi}.${ext}", "app.apk", &diag);
+        "app.${density}_${locale}_${feature}_${gl}.${sdk}.${abi}.${ext}", "app.apk", &diag);
     ASSERT_TRUE(result);
-    EXPECT_EQ(result.value(), "app.ldpi_en-AU_df1_glx1.sdk26.mips64.apk");
+    EXPECT_EQ(result.value(), "app.ldpi_en-AU_df1_glx1.v26.mips64.apk");
   }
 
   {
     auto result = artifact.ToArtifactName(
-        "${basename}.${density}_${locale}_${feature}_${gl}.sdk${sdk}.${abi}", "app.apk", &diag);
+        "${basename}.${density}_${locale}_${feature}_${gl}.${sdk}.${abi}", "app.apk", &diag);
     ASSERT_TRUE(result);
-    EXPECT_EQ(result.value(), "app.ldpi_en-AU_df1_glx1.sdk26.mips64.apk");
+    EXPECT_EQ(result.value(), "app.ldpi_en-AU_df1_glx1.v26.mips64.apk");
   }
 }
 
diff --git a/tools/aapt2/optimize/MultiApkGenerator.cpp b/tools/aapt2/optimize/MultiApkGenerator.cpp
index e7a4f8578529..5ff890832371 100644
--- a/tools/aapt2/optimize/MultiApkGenerator.cpp
+++ b/tools/aapt2/optimize/MultiApkGenerator.cpp
@@ -26,25 +26,77 @@
 #include "filter/AbiFilter.h"
 #include "filter/Filter.h"
 #include "flatten/Archive.h"
+#include "optimize/VersionCollapser.h"
 #include "process/IResourceTableConsumer.h"
 #include "split/TableSplitter.h"
 #include "util/Files.h"
 
 namespace aapt {
 
+using ::aapt::configuration::AndroidSdk;
 using ::aapt::configuration::Artifact;
 using ::aapt::configuration::PostProcessingConfiguration;
 using ::android::StringPiece;
 
+/**
+ * Context wrapper that allows the min Android SDK value to be overridden.
+ */
+class ContextWrapper : public IAaptContext {
+ public:
+  explicit ContextWrapper(IAaptContext* context)
+      : context_(context), min_sdk_(context_->GetMinSdkVersion()) {
+  }
+
+  PackageType GetPackageType() override {
+    return context_->GetPackageType();
+  }
+
+  SymbolTable* GetExternalSymbols() override {
+    return context_->GetExternalSymbols();
+  }
+
+  IDiagnostics* GetDiagnostics() override {
+    return context_->GetDiagnostics();
+  }
+
+  const std::string& GetCompilationPackage() override {
+    return context_->GetCompilationPackage();
+  }
+
+  uint8_t GetPackageId() override {
+    return context_->GetPackageId();
+  }
+
+  NameMangler* GetNameMangler() override {
+    return context_->GetNameMangler();
+  }
+
+  bool IsVerbose() override {
+    return context_->IsVerbose();
+  }
+
+  int GetMinSdkVersion() override {
+    return min_sdk_;
+  }
+
+  void SetMinSdkVersion(int min_sdk) {
+    min_sdk_ = min_sdk;
+  }
+
+ private:
+  IAaptContext* context_;
+
+  int min_sdk_ = -1;
+};
+
 MultiApkGenerator::MultiApkGenerator(LoadedApk* apk, IAaptContext* context)
     : apk_(apk), context_(context) {
 }
 
-bool MultiApkGenerator::FromBaseApk(const std::string& out_dir,
-                                    const PostProcessingConfiguration& config,
-                                    const TableFlattenerOptions& table_flattener_options) {
+bool MultiApkGenerator::FromBaseApk(const MultiApkGeneratorOptions& options) {
   // TODO(safarmer): Handle APK version codes for the generated APKs.
   IDiagnostics* diag = context_->GetDiagnostics();
+  const PostProcessingConfiguration& config = options.config;
 
   const std::string& apk_name = file::GetFilename(apk_->GetSource().path).to_string();
   const StringPiece ext = file::GetExtension(apk_name);
@@ -53,8 +105,6 @@ bool MultiApkGenerator::FromBaseApk(const std::string& out_dir,
   // For now, just write out the stripped APK since ABI splitting doesn't modify anything else.
   for (const Artifact& artifact : config.artifacts) {
     FilterChain filters;
-    TableSplitterOptions splits;
-    AxisConfigFilter axis_filter;
 
     if (!artifact.name && !config.artifact_format) {
       diag->Error(
@@ -71,74 +121,131 @@ bool MultiApkGenerator::FromBaseApk(const std::string& out_dir,
       return false;
     }
 
-    if (artifact.abi_group) {
-      const std::string& group_name = artifact.abi_group.value();
+    std::unique_ptr<ResourceTable> table =
+        FilterTable(artifact, config, *apk_->GetResourceTable(), &filters);
+    if (!table) {
+      return false;
+    }
+
+    std::string out = options.out_dir;
+    if (!file::mkdirs(out)) {
+      context_->GetDiagnostics()->Warn(DiagMessage() << "could not create out dir: " << out);
+    }
+    file::AppendPath(&out, artifact_name.value());
 
-      auto group = config.abi_groups.find(group_name);
-      // TODO: Remove validation when configuration parser ensures referential integrity.
-      if (group == config.abi_groups.end()) {
-        diag->Error(DiagMessage() << "could not find referenced ABI group '" << group_name << "'");
-        return false;
-      }
-      filters.AddFilter(AbiFilter::FromAbiList(group->second));
+    if (context_->IsVerbose()) {
+      context_->GetDiagnostics()->Note(DiagMessage() << "Generating split: " << out);
     }
 
-    if (artifact.screen_density_group) {
-      const std::string& group_name = artifact.screen_density_group.value();
+    std::unique_ptr<IArchiveWriter> writer =
+        CreateZipFileArchiveWriter(context_->GetDiagnostics(), out);
 
-      auto group = config.screen_density_groups.find(group_name);
-      // TODO: Remove validation when configuration parser ensures referential integrity.
-      if (group == config.screen_density_groups.end()) {
-        diag->Error(DiagMessage() << "could not find referenced group '" << group_name << "'");
-        return false;
-      }
+    if (context_->IsVerbose()) {
+      diag->Note(DiagMessage() << "Writing output: " << out);
+    }
 
-      const std::vector<ConfigDescription>& densities = group->second;
-      std::for_each(densities.begin(), densities.end(), [&](const ConfigDescription& c) {
-        splits.preferred_densities.push_back(c.density);
-      });
+    if (!apk_->WriteToArchive(context_, table.get(), options.table_flattener_options, &filters,
+                              writer.get())) {
+      return false;
     }
+  }
 
-    if (artifact.locale_group) {
-      const std::string& group_name = artifact.locale_group.value();
-      auto group = config.locale_groups.find(group_name);
-      // TODO: Remove validation when configuration parser ensures referential integrity.
-      if (group == config.locale_groups.end()) {
-        diag->Error(DiagMessage() << "could not find referenced group '" << group_name << "'");
-        return false;
-      }
-
-      const std::vector<ConfigDescription>& locales = group->second;
-      std::for_each(locales.begin(), locales.end(),
-                    [&](const ConfigDescription& c) { axis_filter.AddConfig(c); });
-      splits.config_filter = &axis_filter;
+  return true;
+}
+
+std::unique_ptr<ResourceTable> MultiApkGenerator::FilterTable(
+    const configuration::Artifact& artifact,
+    const configuration::PostProcessingConfiguration& config, const ResourceTable& old_table,
+    FilterChain* filters) {
+  TableSplitterOptions splits;
+  AxisConfigFilter axis_filter;
+  ContextWrapper wrappedContext{context_};
+
+  if (artifact.abi_group) {
+    const std::string& group_name = artifact.abi_group.value();
+
+    auto group = config.abi_groups.find(group_name);
+    // TODO: Remove validation when configuration parser ensures referential integrity.
+    if (group == config.abi_groups.end()) {
+      context_->GetDiagnostics()->Error(DiagMessage() << "could not find referenced ABI group '"
+                                                      << group_name << "'");
+      return {};
     }
+    filters->AddFilter(AbiFilter::FromAbiList(group->second));
+  }
 
-    std::unique_ptr<ResourceTable> table = apk_->GetResourceTable()->Clone();
+  if (artifact.screen_density_group) {
+    const std::string& group_name = artifact.screen_density_group.value();
 
+    auto group = config.screen_density_groups.find(group_name);
+    // TODO: Remove validation when configuration parser ensures referential integrity.
+    if (group == config.screen_density_groups.end()) {
+      context_->GetDiagnostics()->Error(DiagMessage() << "could not find referenced group '"
+                                                      << group_name << "'");
+      return {};
+    }
 
-    TableSplitter splitter{{}, splits};
-    splitter.SplitTable(table.get());
+    const std::vector<ConfigDescription>& densities = group->second;
+    for(const auto& density_config : densities) {
+      splits.preferred_densities.push_back(density_config.density);
+    }
+  }
 
-    std::string out = out_dir;
-    if (!file::mkdirs(out)) {
-      context_->GetDiagnostics()->Warn(DiagMessage() << "could not create out dir: " << out);
+  if (artifact.locale_group) {
+    const std::string& group_name = artifact.locale_group.value();
+    auto group = config.locale_groups.find(group_name);
+    // TODO: Remove validation when configuration parser ensures referential integrity.
+    if (group == config.locale_groups.end()) {
+      context_->GetDiagnostics()->Error(DiagMessage() << "could not find referenced group '"
+                                                      << group_name << "'");
+      return {};
     }
-    file::AppendPath(&out, artifact_name.value());
 
-    std::unique_ptr<IArchiveWriter> writer = CreateZipFileArchiveWriter(diag, out);
+    const std::vector<ConfigDescription>& locales = group->second;
+    for (const auto& locale : locales) {
+      axis_filter.AddConfig(locale);
+    }
+    splits.config_filter = &axis_filter;
+  }
 
-    if (context_->IsVerbose()) {
-      diag->Note(DiagMessage() << "Writing output: " << out);
+  if (artifact.android_sdk_group) {
+    const std::string& group_name = artifact.android_sdk_group.value();
+    auto group = config.android_sdk_groups.find(group_name);
+    // TODO: Remove validation when configuration parser ensures referential integrity.
+    if (group == config.android_sdk_groups.end()) {
+      context_->GetDiagnostics()->Error(DiagMessage() << "could not find referenced group '"
+                                                      << group_name << "'");
+      return {};
     }
 
-    if (!apk_->WriteToArchive(context_, table.get(), table_flattener_options, &filters,
-                              writer.get())) {
-      return false;
+    const AndroidSdk& sdk = group->second;
+    if (!sdk.min_sdk_version) {
+      context_->GetDiagnostics()->Error(DiagMessage()
+                                        << "skipping SDK version. No min SDK: " << group_name);
+      return {};
     }
+
+    ConfigDescription c;
+    const std::string& version = sdk.min_sdk_version.value();
+    if (!ConfigDescription::Parse(version, &c)) {
+      context_->GetDiagnostics()->Error(DiagMessage() << "could not parse min SDK: " << version);
+      return {};
+    }
+
+    wrappedContext.SetMinSdkVersion(c.sdkVersion);
   }
 
-  return true;
+  std::unique_ptr<ResourceTable> table = old_table.Clone();
+
+  VersionCollapser collapser;
+  if (!collapser.Consume(context_, table.get())) {
+    context_->GetDiagnostics()->Error(DiagMessage() << "Failed to strip versioned resources");
+    return {};
+  }
+
+  TableSplitter splitter{{}, splits};
+  splitter.SplitTable(table.get());
+  return table;
 }
 
 }  // namespace aapt
diff --git a/tools/aapt2/optimize/MultiApkGenerator.h b/tools/aapt2/optimize/MultiApkGenerator.h
index f325d83053f4..b06440014be6 100644
--- a/tools/aapt2/optimize/MultiApkGenerator.h
+++ b/tools/aapt2/optimize/MultiApkGenerator.h
@@ -23,6 +23,12 @@
 
 namespace aapt {
 
+struct MultiApkGeneratorOptions {
+  std::string out_dir;
+  configuration::PostProcessingConfiguration config;
+  TableFlattenerOptions table_flattener_options;
+};
+
 /**
  * Generates a set of APKs that are a subset of the original base APKs. Each of the new APKs contain
  * only the resources and assets for an artifact in the configuration file.
@@ -35,9 +41,13 @@ class MultiApkGenerator {
    * Writes a set of APKs to the provided output directory. Each APK is a subset fo the base APK and
    * represents an artifact in the post processing configuration.
    */
-  bool FromBaseApk(const std::string& out_dir,
-                   const configuration::PostProcessingConfiguration& config,
-                   const TableFlattenerOptions& table_flattener_options);
+  bool FromBaseApk(const MultiApkGeneratorOptions& options);
+
+ protected:
+  virtual std::unique_ptr<ResourceTable> FilterTable(
+      const configuration::Artifact& artifact,
+      const configuration::PostProcessingConfiguration& config, const ResourceTable& old_table,
+      FilterChain* chain);
 
  private:
   IDiagnostics* GetDiagnostics() {
diff --git a/tools/aapt2/optimize/MultiApkGenerator_test.cpp b/tools/aapt2/optimize/MultiApkGenerator_test.cpp
index 6c928d94e879..23f573cd52a5 100644
--- a/tools/aapt2/optimize/MultiApkGenerator_test.cpp
+++ b/tools/aapt2/optimize/MultiApkGenerator_test.cpp
@@ -35,13 +35,22 @@ namespace aapt {
 namespace {
 
 using ::aapt::configuration::Abi;
+using ::aapt::configuration::AndroidSdk;
 using ::aapt::configuration::Artifact;
 using ::aapt::configuration::PostProcessingConfiguration;
-
+using ::aapt::test::GetValue;
+using ::aapt::test::GetValueForConfig;
+using ::aapt::test::ParseConfigOrDie;
 using ::testing::Eq;
+using ::testing::IsNull;
+using ::testing::Not;
+using ::testing::NotNull;
+using ::testing::PrintToString;
 using ::testing::Return;
+using ::testing::Test;
 using ::testing::_;
 
+/** Subclass the LoadedApk class so that we can mock the WriteToArchive method. */
 class MockApk : public LoadedApk {
  public:
   MockApk(std::unique_ptr<ResourceTable> table) : LoadedApk({"test.apk"}, {}, std::move(table)){};
@@ -49,19 +58,61 @@ class MockApk : public LoadedApk {
                                     FilterChain*, IArchiveWriter*));
 };
 
-TEST(MultiApkGeneratorTest, FromBaseApk) {
-  std::unique_ptr<ResourceTable> table =
-      test::ResourceTableBuilder()
-          .AddFileReference("android:drawable/icon", "res/drawable-mdpi/icon.png",
-                            test::ParseConfigOrDie("mdpi"))
-          .AddFileReference("android:drawable/icon", "res/drawable-hdpi/icon.png",
-                            test::ParseConfigOrDie("hdpi"))
-          .AddFileReference("android:drawable/icon", "res/drawable-xhdpi/icon.png",
-                            test::ParseConfigOrDie("xhdpi"))
-          .AddFileReference("android:drawable/icon", "res/drawable-xxhdpi/icon.png",
-                            test::ParseConfigOrDie("xxhdpi"))
-          .AddSimple("android:string/one")
-          .Build();
+/**
+ * Subclass the MultiApkGenerator class so that we can access the protected FilterTable method to
+ * directly test table filter.
+ */
+class MultiApkGeneratorWrapper : public MultiApkGenerator {
+ public:
+  MultiApkGeneratorWrapper(LoadedApk* apk, IAaptContext* context)
+      : MultiApkGenerator(apk, context) {
+  }
+
+  std::unique_ptr<ResourceTable> FilterTable(
+      const configuration::Artifact& artifact,
+      const configuration::PostProcessingConfiguration& config, const ResourceTable& old_table,
+      FilterChain* pChain) override {
+    return MultiApkGenerator::FilterTable(artifact, config, old_table, pChain);
+  }
+};
+
+/** MultiApkGenerator test fixture. */
+class MultiApkGeneratorTest : public ::testing::Test {
+ public:
+  std::unique_ptr<ResourceTable> BuildTable() {
+    return test::ResourceTableBuilder()
+        .AddFileReference(kResourceName, "res/drawable-mdpi/icon.png", mdpi_)
+        .AddFileReference(kResourceName, "res/drawable-hdpi/icon.png", hdpi_)
+        .AddFileReference(kResourceName, "res/drawable-xhdpi/icon.png", xhdpi_)
+        .AddFileReference(kResourceName, "res/drawable-xxhdpi/icon.png", xxhdpi_)
+        .AddFileReference(kResourceName, "res/drawable-v19/icon.xml", v19_)
+        .AddFileReference(kResourceName, "res/drawable-v21/icon.xml", v21_)
+        .AddSimple("android:string/one")
+        .Build();
+  }
+
+  inline FileReference* ValueForConfig(ResourceTable* table, const ConfigDescription& config) {
+    return GetValueForConfig<FileReference>(table, kResourceName, config);
+  };
+
+  void SetUp() override {
+  }
+
+ protected:
+  static constexpr const char* kResourceName = "android:drawable/icon";
+
+  ConfigDescription default_ = ParseConfigOrDie("").CopyWithoutSdkVersion();
+  ConfigDescription mdpi_ = ParseConfigOrDie("mdpi").CopyWithoutSdkVersion();
+  ConfigDescription hdpi_ = ParseConfigOrDie("hdpi").CopyWithoutSdkVersion();
+  ConfigDescription xhdpi_ = ParseConfigOrDie("xhdpi").CopyWithoutSdkVersion();
+  ConfigDescription xxhdpi_ = ParseConfigOrDie("xxhdpi").CopyWithoutSdkVersion();
+  ConfigDescription xxxhdpi_ = ParseConfigOrDie("xxxhdpi").CopyWithoutSdkVersion();
+  ConfigDescription v19_ = ParseConfigOrDie("v19");
+  ConfigDescription v21_ = ParseConfigOrDie("v21");
+};
+
+TEST_F(MultiApkGeneratorTest, FromBaseApk) {
+  std::unique_ptr<ResourceTable> table = BuildTable();
 
   MockApk apk{std::move(table)};
 
@@ -72,7 +123,7 @@ TEST(MultiApkGeneratorTest, FromBaseApk) {
   TableFlattenerOptions table_flattener_options;
 
   MultiApkGenerator generator{&apk, &ctx};
-  EXPECT_TRUE(generator.FromBaseApk("out", empty_config, table_flattener_options));
+  EXPECT_TRUE(generator.FromBaseApk({"out", empty_config, table_flattener_options}));
 
   Artifact x64 = test::ArtifactBuilder()
                      .SetName("${basename}.x64.apk")
@@ -101,7 +152,122 @@ TEST(MultiApkGeneratorTest, FromBaseApk) {
 
   // Called once for each artifact.
   EXPECT_CALL(apk, WriteToArchive(Eq(&ctx), _, _, _, _)).Times(2).WillRepeatedly(Return(true));
-  EXPECT_TRUE(generator.FromBaseApk("out", config, table_flattener_options));
+  EXPECT_TRUE(generator.FromBaseApk({"out", config, table_flattener_options}));
+}
+
+TEST_F(MultiApkGeneratorTest, VersionFilterNewerVersion) {
+  std::unique_ptr<ResourceTable> table = BuildTable();
+
+  MockApk apk{std::move(table)};
+  std::unique_ptr<IAaptContext> ctx = test::ContextBuilder().SetMinSdkVersion(19).Build();
+  PostProcessingConfiguration empty_config;
+  TableFlattenerOptions table_flattener_options;
+  FilterChain chain;
+
+  Artifact x64 = test::ArtifactBuilder()
+                     .SetName("${basename}.${sdk}.apk")
+                     .SetDensityGroup("xhdpi")
+                     .SetAndroidSdk("v23")
+                     .Build();
+
+  auto config = test::PostProcessingConfigurationBuilder()
+                    .SetLocaleGroup("en", {"en"})
+                    .SetAbiGroup("x64", {Abi::kX86_64})
+                    .SetDensityGroup("xhdpi", {"xhdpi"})
+                    .SetAndroidSdk("v23", AndroidSdk::ForMinSdk("v23"))
+                    .AddArtifact(x64)
+                    .Build();
+
+  MultiApkGeneratorWrapper generator{&apk, ctx.get()};
+  std::unique_ptr<ResourceTable> split =
+      generator.FilterTable(x64, config, *apk.GetResourceTable(), &chain);
+
+  ResourceTable* new_table = split.get();
+  EXPECT_THAT(ValueForConfig(new_table, mdpi_), IsNull());
+  EXPECT_THAT(ValueForConfig(new_table, hdpi_), IsNull());
+  EXPECT_THAT(ValueForConfig(new_table, xxhdpi_), IsNull());
+  EXPECT_THAT(ValueForConfig(new_table, xxxhdpi_), IsNull());
+  EXPECT_THAT(ValueForConfig(new_table, v19_), IsNull());
+
+  // xhdpi directly matches one of the required dimensions.
+  EXPECT_THAT(ValueForConfig(new_table, xhdpi_), NotNull());
+  // drawable-v21 was converted to drawable.
+  EXPECT_THAT(ValueForConfig(new_table, default_), NotNull());
+  EXPECT_THAT(GetValue<Id>(new_table, "android:string/one"), NotNull());
+}
+
+TEST_F(MultiApkGeneratorTest, VersionFilterOlderVersion) {
+  std::unique_ptr<ResourceTable> table = BuildTable();
+
+  MockApk apk{std::move(table)};
+  std::unique_ptr<IAaptContext> ctx = test::ContextBuilder().SetMinSdkVersion(1).Build();
+  PostProcessingConfiguration empty_config;
+  TableFlattenerOptions table_flattener_options;
+  FilterChain chain;
+
+  Artifact x64 = test::ArtifactBuilder()
+                     .SetName("${basename}.${sdk}.apk")
+                     .SetDensityGroup("xhdpi")
+                     .SetAndroidSdk("v4")
+                     .Build();
+
+  auto config = test::PostProcessingConfigurationBuilder()
+                    .SetLocaleGroup("en", {"en"})
+                    .SetAbiGroup("x64", {Abi::kX86_64})
+                    .SetDensityGroup("xhdpi", {"xhdpi"})
+                    .SetAndroidSdk("v4", AndroidSdk::ForMinSdk("v4"))
+                    .AddArtifact(x64)
+                    .Build();
+
+  MultiApkGeneratorWrapper generator{&apk, ctx.get()};;
+  std::unique_ptr<ResourceTable> split =
+      generator.FilterTable(x64, config, *apk.GetResourceTable(), &chain);
+
+  ResourceTable* new_table = split.get();
+  EXPECT_THAT(ValueForConfig(new_table, mdpi_), IsNull());
+  EXPECT_THAT(ValueForConfig(new_table, hdpi_), IsNull());
+  EXPECT_THAT(ValueForConfig(new_table, xxhdpi_), IsNull());
+  EXPECT_THAT(ValueForConfig(new_table, xxxhdpi_), IsNull());
+
+  EXPECT_THAT(ValueForConfig(new_table, xhdpi_), NotNull());
+  EXPECT_THAT(ValueForConfig(new_table, v19_), NotNull());
+  EXPECT_THAT(ValueForConfig(new_table, v21_), NotNull());
+  EXPECT_THAT(GetValue<Id>(new_table, "android:string/one"), NotNull());
+}
+
+TEST_F(MultiApkGeneratorTest, VersionFilterNoVersion) {
+  std::unique_ptr<ResourceTable> table = BuildTable();
+
+  MockApk apk{std::move(table)};
+  std::unique_ptr<IAaptContext> ctx = test::ContextBuilder().SetMinSdkVersion(1).Build();
+  PostProcessingConfiguration empty_config;
+  TableFlattenerOptions table_flattener_options;
+  FilterChain chain;
+
+  Artifact x64 =
+      test::ArtifactBuilder().SetName("${basename}.${sdk}.apk").SetDensityGroup("xhdpi").Build();
+
+  auto config = test::PostProcessingConfigurationBuilder()
+                    .SetLocaleGroup("en", {"en"})
+                    .SetAbiGroup("x64", {Abi::kX86_64})
+                    .SetDensityGroup("xhdpi", {"xhdpi"})
+                    .AddArtifact(x64)
+                    .Build();
+
+  MultiApkGeneratorWrapper generator{&apk, ctx.get()};
+  std::unique_ptr<ResourceTable> split =
+      generator.FilterTable(x64, config, *apk.GetResourceTable(), &chain);
+
+  ResourceTable* new_table = split.get();
+  EXPECT_THAT(ValueForConfig(new_table, mdpi_), IsNull());
+  EXPECT_THAT(ValueForConfig(new_table, hdpi_), IsNull());
+  EXPECT_THAT(ValueForConfig(new_table, xxhdpi_), IsNull());
+  EXPECT_THAT(ValueForConfig(new_table, xxxhdpi_), IsNull());
+
+  EXPECT_THAT(ValueForConfig(new_table, xhdpi_), NotNull());
+  EXPECT_THAT(ValueForConfig(new_table, v19_), NotNull());
+  EXPECT_THAT(ValueForConfig(new_table, v21_), NotNull());
+  EXPECT_THAT(GetValue<Id>(new_table, "android:string/one"), NotNull());
 }
 
 }  // namespace
diff --git a/tools/aapt2/optimize/VersionCollapser.cpp b/tools/aapt2/optimize/VersionCollapser.cpp
index d941b487e439..cc1fc1e6910b 100644
--- a/tools/aapt2/optimize/VersionCollapser.cpp
+++ b/tools/aapt2/optimize/VersionCollapser.cpp
@@ -63,11 +63,9 @@ FilterIterator<Iterator, Pred> make_filter_iterator(Iterator begin,
 }
 
 /**
- * Every Configuration with an SDK version specified that is less than minSdk
- * will be removed.
- * The exception is when there is no exact matching resource for the minSdk. The
- * next smallest
- * one will be kept.
+ * Every Configuration with an SDK version specified that is less than minSdk will be removed. The
+ * exception is when there is no exact matching resource for the minSdk. The next smallest one will
+ * be kept.
  */
 static void CollapseVersions(int min_sdk, ResourceEntry* entry) {
   // First look for all sdks less than minSdk.
@@ -80,11 +78,9 @@ static void CollapseVersions(int min_sdk, ResourceEntry* entry) {
 
     const ConfigDescription& config = (*iter)->config;
     if (config.sdkVersion <= min_sdk) {
-      // This is the first configuration we've found with a smaller or equal SDK
-      // level
-      // to the minimum. We MUST keep this one, but remove all others we find,
-      // which get
-      // overridden by this one.
+      // This is the first configuration we've found with a smaller or equal SDK level to the
+      // minimum. We MUST keep this one, but remove all others we find, which get overridden by this
+      // one.
 
       ConfigDescription config_without_sdk = config.CopyWithoutSdkVersion();
       auto pred = [&](const std::unique_ptr<ResourceConfigValue>& val) -> bool {
@@ -115,11 +111,9 @@ static void CollapseVersions(int min_sdk, ResourceEntry* entry) {
                          -> bool { return val == nullptr; }),
       entry->values.end());
 
-  // Strip the version qualifiers for every resource with version <= minSdk.
-  // This will ensure
-  // that the resource entries are all packed together in the same ResTable_type
-  // struct
-  // and take up less space in the resources.arsc table.
+  // Strip the version qualifiers for every resource with version <= minSdk. This will ensure that
+  // the resource entries are all packed together in the same ResTable_type struct and take up less
+  // space in the resources.arsc table.
   bool modified = false;
   for (std::unique_ptr<ResourceConfigValue>& config_value : entry->values) {
     if (config_value->config.sdkVersion != 0 &&
@@ -137,8 +131,8 @@ static void CollapseVersions(int min_sdk, ResourceEntry* entry) {
   }
 
   if (modified) {
-    // We've modified the keys (ConfigDescription) by changing the sdkVersion to
-    // 0. We MUST re-sort to ensure ordering guarantees hold.
+    // We've modified the keys (ConfigDescription) by changing the sdkVersion to 0. We MUST re-sort
+    // to ensure ordering guarantees hold.
     std::sort(entry->values.begin(), entry->values.end(),
               [](const std::unique_ptr<ResourceConfigValue>& a,
                  const std::unique_ptr<ResourceConfigValue>& b) -> bool {
diff --git a/tools/aapt2/test/Builders.cpp b/tools/aapt2/test/Builders.cpp
index 57e3df442e3c..e658664d8653 100644
--- a/tools/aapt2/test/Builders.cpp
+++ b/tools/aapt2/test/Builders.cpp
@@ -23,6 +23,9 @@
 #include "test/Common.h"
 #include "util/Util.h"
 
+using ::aapt::configuration::Abi;
+using ::aapt::configuration::AndroidSdk;
+using ::aapt::configuration::Artifact;
 using ::aapt::io::StringInputStream;
 using ::android::StringPiece;
 
@@ -213,7 +216,7 @@ std::unique_ptr<xml::XmlResource> BuildXmlDomForPackageName(IAaptContext* contex
 }
 
 PostProcessingConfigurationBuilder& PostProcessingConfigurationBuilder::SetAbiGroup(
-    const std::string& name, const std::vector<configuration::Abi>& abis) {
+    const std::string& name, const std::vector<Abi>& abis) {
   config_.abi_groups[name] = abis;
   return *this;
 }
@@ -236,8 +239,14 @@ PostProcessingConfigurationBuilder& PostProcessingConfigurationBuilder::SetDensi
   return *this;
 }
 
+PostProcessingConfigurationBuilder& PostProcessingConfigurationBuilder::SetAndroidSdk(
+    const std::string& name, const AndroidSdk& sdk) {
+  config_.android_sdk_groups[name] = sdk;
+  return *this;
+}
+
 PostProcessingConfigurationBuilder& PostProcessingConfigurationBuilder::AddArtifact(
-    const configuration::Artifact& artifact) {
+    const Artifact& artifact) {
   config_.artifacts.push_back(artifact);
   return *this;
 }
@@ -266,6 +275,11 @@ ArtifactBuilder& ArtifactBuilder::SetLocaleGroup(const std::string& name) {
   return *this;
 }
 
+ArtifactBuilder& ArtifactBuilder::SetAndroidSdk(const std::string& name) {
+  artifact_.android_sdk_group = {name};
+  return *this;
+}
+
 configuration::Artifact ArtifactBuilder::Build() {
   return artifact_;
 }
diff --git a/tools/aapt2/test/Builders.h b/tools/aapt2/test/Builders.h
index e8cefc1853b6..263fb5562b25 100644
--- a/tools/aapt2/test/Builders.h
+++ b/tools/aapt2/test/Builders.h
@@ -161,6 +161,8 @@ class PostProcessingConfigurationBuilder {
                                                      const std::vector<std::string>& locales);
   PostProcessingConfigurationBuilder& SetDensityGroup(const std::string& name,
                                                       const std::vector<std::string>& densities);
+  PostProcessingConfigurationBuilder& SetAndroidSdk(const std::string& name,
+                                                    const configuration::AndroidSdk& sdk);
   PostProcessingConfigurationBuilder& AddArtifact(const configuration::Artifact& artifact);
   configuration::PostProcessingConfiguration Build();
 
@@ -176,6 +178,7 @@ class ArtifactBuilder {
   ArtifactBuilder& SetAbiGroup(const std::string& name);
   ArtifactBuilder& SetDensityGroup(const std::string& name);
   ArtifactBuilder& SetLocaleGroup(const std::string& name);
+  ArtifactBuilder& SetAndroidSdk(const std::string& name);
   configuration::Artifact Build();
 
  private:
diff --git a/tools/aapt2/test/Common.h b/tools/aapt2/test/Common.h
index 7482b8b627d3..61d056309a69 100644
--- a/tools/aapt2/test/Common.h
+++ b/tools/aapt2/test/Common.h
@@ -47,7 +47,7 @@ inline ResourceName ParseNameOrDie(const android::StringPiece& str) {
 
 inline ConfigDescription ParseConfigOrDie(const android::StringPiece& str) {
   ConfigDescription config;
-  CHECK(ConfigDescription::Parse(str, &config)) << "invalid configuration";
+  CHECK(ConfigDescription::Parse(str, &config)) << "invalid configuration: " << str;
   return config;
 }