Merge pull request #8302 from AdmiralCurtiss/gcmemcard-comments

GCMemcard: Read comments, banners, and icons via logical data offsets instead of physical ones.

3 files changed, 290 insertions, 214 deletions

diff --git a/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp b/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp
index 8bdb8262f0..75240765ca 100644
--- a/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp
+++ b/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp
@@ -579,43 +579,93 @@ u16 GCMemcard::DEntry_BlockCount(u8 index) const
   return blocks;
 }
 
-u32 GCMemcard::DEntry_CommentsAddress(u8 index) const
+std::optional<std::vector<u8>> GCMemcard::GetSaveDataBytes(u8 save_index, size_t offset,
+                                                           size_t length) const
 {
-  if (!m_valid || index >= DIRLEN)
-    return 0xFFFF;
+  if (!m_valid || save_index >= DIRLEN)
+    return std::nullopt;
 
-  return GetActiveDirectory().m_dir_entries[index].m_comments_address;
-}
+  const DEntry& entry = GetActiveDirectory().m_dir_entries[save_index];
+  const BlockAlloc& bat = GetActiveBat();
+  const u16 block_count = entry.m_block_count;
+  const u16 first_block = entry.m_first_block;
+  const size_t block_max = MC_FST_BLOCKS + m_data_blocks.size();
+  if (block_count == 0xFFFF || first_block < MC_FST_BLOCKS || first_block >= block_max)
+    return std::nullopt;
 
-std::string GCMemcard::GetSaveComment1(u8 index) const
-{
-  if (!m_valid || index >= DIRLEN)
-    return "";
+  const u32 file_size = block_count * BLOCK_SIZE;
+  if (offset >= file_size)
+    return std::nullopt;
 
-  u32 Comment1 = GetActiveDirectory().m_dir_entries[index].m_comments_address;
-  u32 DataBlock = GetActiveDirectory().m_dir_entries[index].m_first_block - MC_FST_BLOCKS;
-  if ((DataBlock > m_size_blocks) || (Comment1 == 0xFFFFFFFF))
+  const size_t bytes_to_copy = std::min(length, file_size - offset);
+  std::vector<u8> result;
+  result.reserve(bytes_to_copy);
+
+  u16 current_block = first_block;
+  size_t offset_in_current_block = offset;
+  size_t bytes_remaining = bytes_to_copy;
+
+  // skip unnecessary blocks at start
+  while (offset_in_current_block >= BLOCK_SIZE)
   {
-    return "";
+    offset_in_current_block -= BLOCK_SIZE;
+    current_block = bat.GetNextBlock(current_block);
+    if (current_block < MC_FST_BLOCKS || current_block >= block_max)
+      return std::nullopt;
   }
-  return std::string((const char*)m_data_blocks[DataBlock].m_block.data() + Comment1,
-                     DENTRY_STRLEN);
+
+  // then copy one block at a time into the result vector
+  while (true)
+  {
+    const GCMBlock& block = m_data_blocks[current_block - MC_FST_BLOCKS];
+    const size_t bytes_in_current_block_left = BLOCK_SIZE - offset_in_current_block;
+    const size_t bytes_in_current_block_left_to_copy =
+        std::min(bytes_remaining, bytes_in_current_block_left);
+
+    const auto data_to_copy_begin = block.m_block.begin() + offset_in_current_block;
+    const auto data_to_copy_end = data_to_copy_begin + bytes_in_current_block_left_to_copy;
+    result.insert(result.end(), data_to_copy_begin, data_to_copy_end);
+
+    bytes_remaining -= bytes_in_current_block_left_to_copy;
+    if (bytes_remaining == 0)
+      break;
+
+    offset_in_current_block = 0;
+    current_block = bat.GetNextBlock(current_block);
+    if (current_block < MC_FST_BLOCKS || current_block >= block_max)
+      return std::nullopt;
+  }
+
+  return std::make_optional(std::move(result));
 }
 
-std::string GCMemcard::GetSaveComment2(u8 index) const
+std::optional<std::pair<std::string, std::string>> GCMemcard::GetSaveComments(u8 index) const
 {
   if (!m_valid || index >= DIRLEN)
-    return "";
+    return std::nullopt;
 
-  u32 Comment1 = GetActiveDirectory().m_dir_entries[index].m_comments_address;
-  u32 Comment2 = Comment1 + DENTRY_STRLEN;
-  u32 DataBlock = GetActiveDirectory().m_dir_entries[index].m_first_block - MC_FST_BLOCKS;
-  if ((DataBlock > m_size_blocks) || (Comment1 == 0xFFFFFFFF))
-  {
-    return "";
-  }
-  return std::string((const char*)m_data_blocks[DataBlock].m_block.data() + Comment2,
-                     DENTRY_STRLEN);
+  const u32 address = GetActiveDirectory().m_dir_entries[index].m_comments_address;
+  if (address == 0xFFFFFFFF)
+    return std::nullopt;
+
+  const auto data = GetSaveDataBytes(index, address, DENTRY_STRLEN * 2);
+  if (!data || data->size() != DENTRY_STRLEN * 2)
+    return std::nullopt;
+
+  const auto string_decoder = IsShiftJIS() ? SHIFTJISToUTF8 : CP1252ToUTF8;
+  const auto strip_null = [](const std::string& s) {
+    auto offset = s.find('\0');
+    if (offset == std::string::npos)
+      offset = s.length();
+    return s.substr(0, offset);
+  };
+
+  const u8* address_1 = data->data();
+  const u8* address_2 = address_1 + DENTRY_STRLEN;
+  const std::string encoded_1(reinterpret_cast<const char*>(address_1), DENTRY_STRLEN);
+  const std::string encoded_2(reinterpret_cast<const char*>(address_2), DENTRY_STRLEN);
+  return std::make_pair(strip_null(string_decoder(encoded_1)),
+                        strip_null(string_decoder(encoded_2)));
 }
 
 std::optional<DEntry> GCMemcard::GetDEntry(u8 index) const
@@ -1136,181 +1186,188 @@ void GCMemcard::Gcs_SavConvert(DEntry& tempDEntry, int saveType, u64 length)
   }
 }
 
-bool GCMemcard::ReadBannerRGBA8(u8 index, u32* buffer) const
+std::optional<std::vector<u32>> GCMemcard::ReadBannerRGBA8(u8 index) const
 {
   if (!m_valid || index >= DIRLEN)
-    return false;
-
-  int flags = GetActiveDirectory().m_dir_entries[index].m_banner_and_icon_flags;
-  // Timesplitters 2 is the only game that I see this in
-  // May be a hack
-  if (flags == 0xFB)
-    flags = ~flags;
-
-  int bnrFormat = (flags & 3);
-
-  if (bnrFormat == 0)
-    return false;
+    return std::nullopt;
 
-  u32 DataOffset = GetActiveDirectory().m_dir_entries[index].m_image_offset;
-  u32 DataBlock = GetActiveDirectory().m_dir_entries[index].m_first_block - MC_FST_BLOCKS;
+  const u32 offset = GetActiveDirectory().m_dir_entries[index].m_image_offset;
+  if (offset == 0xFFFFFFFF)
+    return std::nullopt;
 
-  if ((DataBlock > m_size_blocks) || (DataOffset == 0xFFFFFFFF))
-  {
-    return false;
-  }
+  // See comment on m_banner_and_icon_flags for an explanation of these.
+  const u8 flags = GetActiveDirectory().m_dir_entries[index].m_banner_and_icon_flags;
+  const u8 format = (flags & 0b0000'0011);
+  if (format != MEMORY_CARD_BANNER_FORMAT_CI8 && format != MEMORY_CARD_BANNER_FORMAT_RGB5A3)
+    return std::nullopt;
 
-  const int pixels = 96 * 32;
+  constexpr u32 pixel_count = MEMORY_CARD_BANNER_WIDTH * MEMORY_CARD_BANNER_HEIGHT;
+  const size_t total_bytes = format == MEMORY_CARD_BANNER_FORMAT_CI8 ?
+                                 (pixel_count + MEMORY_CARD_CI8_PALETTE_ENTRIES * 2) :
+                                 (pixel_count * 2);
+  const auto data = GetSaveDataBytes(index, offset, total_bytes);
+  if (!data || data->size() != total_bytes)
+    return std::nullopt;
 
-  if (bnrFormat & 1)
+  std::vector<u32> rgba(pixel_count);
+  if (format == MEMORY_CARD_BANNER_FORMAT_CI8)
   {
-    u8* pxdata = (u8*)(m_data_blocks[DataBlock].m_block.data() + DataOffset);
-    u16* paldata = (u16*)(m_data_blocks[DataBlock].m_block.data() + DataOffset + pixels);
-
-    Common::DecodeCI8Image(buffer, pxdata, paldata, 96, 32);
+    const u8* pxdata = data->data();
+    std::array<u16, MEMORY_CARD_CI8_PALETTE_ENTRIES> paldata;
+    std::memcpy(paldata.data(), data->data() + pixel_count, MEMORY_CARD_CI8_PALETTE_ENTRIES * 2);
+    Common::DecodeCI8Image(rgba.data(), pxdata, paldata.data(), MEMORY_CARD_BANNER_WIDTH,
+                           MEMORY_CARD_BANNER_HEIGHT);
   }
   else
   {
-    u16* pxdata = (u16*)(m_data_blocks[DataBlock].m_block.data() + DataOffset);
-
-    Common::Decode5A3Image(buffer, pxdata, 96, 32);
+    std::array<u16, pixel_count> pxdata;
+    std::memcpy(pxdata.data(), data->data(), pixel_count * 2);
+    Common::Decode5A3Image(rgba.data(), pxdata.data(), MEMORY_CARD_BANNER_WIDTH,
+                           MEMORY_CARD_BANNER_HEIGHT);
   }
-  return true;
+
+  return rgba;
 }
 
-u32 GCMemcard::ReadAnimRGBA8(u8 index, u32* buffer, u8* delays) const
+std::optional<std::vector<GCMemcardAnimationFrameRGBA8>> GCMemcard::ReadAnimRGBA8(u8 index) const
 {
   if (!m_valid || index >= DIRLEN)
-    return 0;
-
-  // To ensure only one type of icon is used
-  // Sonic Heroes it the only game I have seen that tries to use a CI8 and RGB5A3 icon
-  // int fmtCheck = 0;
-
-  int formats = GetActiveDirectory().m_dir_entries[index].m_icon_format;
-  int fdelays = GetActiveDirectory().m_dir_entries[index].m_animation_speed;
-
-  int flags = GetActiveDirectory().m_dir_entries[index].m_banner_and_icon_flags;
-  // Timesplitters 2 and 3 is the only game that I see this in
-  // May be a hack
-  // if (flags == 0xFB) flags = ~flags;
-  // Batten Kaitos has 0x65 as flag too. Everything but the first 3 bytes seems irrelevant.
-  // Something similar happens with Wario Ware Inc. AnimSpeed
-
-  int bnrFormat = (flags & 3);
+    return std::nullopt;
 
-  u32 DataOffset = GetActiveDirectory().m_dir_entries[index].m_image_offset;
-  u32 DataBlock = GetActiveDirectory().m_dir_entries[index].m_first_block - MC_FST_BLOCKS;
+  u32 image_offset = GetActiveDirectory().m_dir_entries[index].m_image_offset;
+  if (image_offset == 0xFFFFFFFF)
+    return std::nullopt;
 
-  if ((DataBlock > m_size_blocks) || (DataOffset == 0xFFFFFFFF))
+  // Data at m_image_offset stores first the banner, if any, and then the icon data.
+  // Skip over the banner if there is one.
+  // See ReadBannerRGBA8() for details on how the banner is stored.
+  const u8 flags = GetActiveDirectory().m_dir_entries[index].m_banner_and_icon_flags;
+  const u8 banner_format = (flags & 0b0000'0011);
+  const u32 banner_pixels = MEMORY_CARD_BANNER_WIDTH * MEMORY_CARD_BANNER_HEIGHT;
+  if (banner_format == MEMORY_CARD_BANNER_FORMAT_CI8)
+    image_offset += banner_pixels + MEMORY_CARD_CI8_PALETTE_ENTRIES * 2;
+  else if (banner_format == MEMORY_CARD_BANNER_FORMAT_RGB5A3)
+    image_offset += banner_pixels * 2;
+
+  // decode icon formats and frame delays
+  const u16 icon_format = GetActiveDirectory().m_dir_entries[index].m_icon_format;
+  const u16 animation_speed = GetActiveDirectory().m_dir_entries[index].m_animation_speed;
+  std::array<u8, MEMORY_CARD_ICON_ANIMATION_MAX_FRAMES> frame_formats;
+  std::array<u8, MEMORY_CARD_ICON_ANIMATION_MAX_FRAMES> frame_delays;
+  for (u32 i = 0; i < MEMORY_CARD_ICON_ANIMATION_MAX_FRAMES; ++i)
   {
-    return 0;
+    frame_formats[i] = (icon_format >> (2 * i)) & 0b11;
+    frame_delays[i] = (animation_speed >> (2 * i)) & 0b11;
   }
 
-  u8* animData = (u8*)(m_data_blocks[DataBlock].m_block.data() + DataOffset);
+  // if first frame format is 0, the entire icon is skipped
+  if (frame_formats[0] == 0)
+    return std::nullopt;
 
-  switch (bnrFormat)
+  // calculate byte length of each individual icon frame and full icon data
+  constexpr u32 pixels_per_frame = MEMORY_CARD_ICON_WIDTH * MEMORY_CARD_ICON_HEIGHT;
+  u32 data_length = 0;
+  u32 frame_count = 0;
+  std::array<u32, MEMORY_CARD_ICON_ANIMATION_MAX_FRAMES> frame_offsets;
+  bool has_shared_palette = false;
+  for (u32 i = 0; i < MEMORY_CARD_ICON_ANIMATION_MAX_FRAMES; ++i)
   {
-  case 1:
-    animData += 96 * 32 + 2 * 256;  // image+palette
-    break;
-  case 2:
-    animData += 96 * 32 * 2;
-    break;
-  }
-
-  int fmts[8];
-  u8* data[8];
-  int frames = 0;
+    if (frame_delays[i] == 0)
+    {
+      // frame delay of 0 means we're out of frames
+      break;
+    }
 
-  for (int i = 0; i < 8; i++)
-  {
-    fmts[i] = (formats >> (2 * i)) & 3;
-    delays[i] = ((fdelays >> (2 * i)) & 3);
-    data[i] = animData;
+    // otherwise this counts as a frame, even if the format is none of the three valid ones
+    // (see the actual icon decoding below for how that is handled)
+    ++frame_count;
+    frame_offsets[i] = data_length;
 
-    if (!delays[i])
+    if (frame_formats[i] == MEMORY_CARD_ICON_FORMAT_CI8_SHARED_PALETTE)
     {
-      // First icon_speed = 0 indicates there aren't any more icons
-      break;
+      data_length += pixels_per_frame;
+      has_shared_palette = true;
     }
-    // If speed is set there is an icon (it can be a "blank frame")
-    frames++;
-    if (fmts[i] != 0)
+    else if (frame_formats[i] == MEMORY_CARD_ICON_FORMAT_RGB5A3)
     {
-      switch (fmts[i])
-      {
-      case CI8SHARED:  // CI8 with shared palette
-        animData += 32 * 32;
-        break;
-      case RGB5A3:  // RGB5A3
-        animData += 32 * 32 * 2;
-        break;
-      case CI8:  // CI8 with own palette
-        animData += 32 * 32 + 2 * 256;
-        break;
-      }
+      data_length += pixels_per_frame * 2;
+    }
+    else if (frame_formats[i] == MEMORY_CARD_ICON_FORMAT_CI8_UNIQUE_PALETTE)
+    {
+      data_length += pixels_per_frame + 2 * MEMORY_CARD_CI8_PALETTE_ENTRIES;
     }
   }
 
-  const u16* sharedPal = reinterpret_cast<u16*>(animData);
+  if (frame_count == 0)
+    return std::nullopt;
 
-  for (int i = 0; i < 8; i++)
+  const u32 shared_palette_offset = data_length;
+  if (has_shared_palette)
+    data_length += 2 * MEMORY_CARD_CI8_PALETTE_ENTRIES;
+
+  // now that we have determined the data length, fetch the actual data from the save file
+  // if anything is sketchy, bail so we don't access out of bounds
+  auto save_data_bytes = GetSaveDataBytes(index, image_offset, data_length);
+  if (!save_data_bytes || save_data_bytes->size() != data_length)
+    return std::nullopt;
+
+  // and finally, decode icons into RGBA8
+  std::array<u16, MEMORY_CARD_CI8_PALETTE_ENTRIES> shared_palette;
+  if (has_shared_palette)
   {
-    if (!delays[i])
-    {
-      // First icon_speed = 0 indicates there aren't any more icons
-      break;
-    }
-    if (fmts[i] != 0)
+    std::memcpy(shared_palette.data(), save_data_bytes->data() + shared_palette_offset,
+                2 * MEMORY_CARD_CI8_PALETTE_ENTRIES);
+  }
+
+  std::vector<GCMemcardAnimationFrameRGBA8> output;
+  for (u32 i = 0; i < frame_count; ++i)
+  {
+    GCMemcardAnimationFrameRGBA8& output_frame = output.emplace_back();
+    output_frame.image_data.resize(pixels_per_frame);
+    output_frame.delay = frame_delays[i];
+
+    // Note on how to interpret this inner loop here: In the general case this just degenerates into
+    // j == i for every iteration, but in some rare cases (such as Luigi's Mansion or Pikmin) some
+    // frames will not actually have an associated format. In this case we forward to the next valid
+    // frame to decode, which appears (at least visually) to match the behavior of the GC BIOS. Note
+    // that this may end up decoding the same frame multiple times.
+    // If this happens but no next valid frame exists, we instead return a fully transparent frame,
+    // again visually matching the GC BIOS. There is no extra code necessary for this as the
+    // resize() of the vector already initializes it to a fully transparent frame.
+    for (u32 j = i; j < frame_count; ++j)
     {
-      switch (fmts[i])
+      if (frame_formats[j] == MEMORY_CARD_ICON_FORMAT_CI8_SHARED_PALETTE)
       {
-      case CI8SHARED:  // CI8 with shared palette
-        Common::DecodeCI8Image(buffer, data[i], sharedPal, 32, 32);
-        buffer += 32 * 32;
+        Common::DecodeCI8Image(output_frame.image_data.data(),
+                               save_data_bytes->data() + frame_offsets[j], shared_palette.data(),
+                               MEMORY_CARD_ICON_WIDTH, MEMORY_CARD_ICON_HEIGHT);
         break;
-      case RGB5A3:  // RGB5A3
-        Common::Decode5A3Image(buffer, (u16*)(data[i]), 32, 32);
-        buffer += 32 * 32;
-        break;
-      case CI8:  // CI8 with own palette
-        const u16* paldata = reinterpret_cast<u16*>(data[i] + 32 * 32);
-        Common::DecodeCI8Image(buffer, data[i], paldata, 32, 32);
-        buffer += 32 * 32;
+      }
+
+      if (frame_formats[j] == MEMORY_CARD_ICON_FORMAT_RGB5A3)
+      {
+        std::array<u16, pixels_per_frame> pxdata;
+        std::memcpy(pxdata.data(), save_data_bytes->data() + frame_offsets[j],
+                    pixels_per_frame * 2);
+        Common::Decode5A3Image(output_frame.image_data.data(), pxdata.data(),
+                               MEMORY_CARD_ICON_WIDTH, MEMORY_CARD_ICON_HEIGHT);
         break;
       }
-    }
-    else
-    {
-      // Speed is set but there's no actual icon
-      // This is used to reduce animation speed in Pikmin and Luigi's Mansion for example
-      // These "blank frames" show the next icon
-      for (int j = i; j < 8; ++j)
+
+      if (frame_formats[j] == MEMORY_CARD_ICON_FORMAT_CI8_UNIQUE_PALETTE)
       {
-        if (fmts[j] != 0)
-        {
-          switch (fmts[j])
-          {
-          case CI8SHARED:  // CI8 with shared palette
-            Common::DecodeCI8Image(buffer, data[j], sharedPal, 32, 32);
-            break;
-          case RGB5A3:  // RGB5A3
-            Common::Decode5A3Image(buffer, (u16*)(data[j]), 32, 32);
-            buffer += 32 * 32;
-            break;
-          case CI8:  // CI8 with own palette
-            const u16* paldata = reinterpret_cast<u16*>(data[j] + 32 * 32);
-            Common::DecodeCI8Image(buffer, data[j], paldata, 32, 32);
-            buffer += 32 * 32;
-            break;
-          }
-        }
+        std::array<u16, MEMORY_CARD_CI8_PALETTE_ENTRIES> paldata;
+        std::memcpy(paldata.data(), save_data_bytes->data() + frame_offsets[j] + pixels_per_frame,
+                    MEMORY_CARD_CI8_PALETTE_ENTRIES * 2);
+        Common::DecodeCI8Image(output_frame.image_data.data(),
+                               save_data_bytes->data() + frame_offsets[j], paldata.data(),
+                               MEMORY_CARD_ICON_WIDTH, MEMORY_CARD_ICON_HEIGHT);
+        break;
       }
     }
   }
 
-  return frames;
+  return output;
 }
 
 bool GCMemcard::Format(u8* card_data, bool shift_jis, u16 SizeMb)
diff --git a/Source/Core/Core/HW/GCMemcard/GCMemcard.h b/Source/Core/Core/HW/GCMemcard/GCMemcard.h
index e676977293..f268fa658e 100644
--- a/Source/Core/Core/HW/GCMemcard/GCMemcard.h
+++ b/Source/Core/Core/HW/GCMemcard/GCMemcard.h
@@ -7,6 +7,7 @@
 #include <algorithm>
 #include <array>
 #include <bitset>
+#include <limits>
 #include <string>
 #include <vector>
 
@@ -35,10 +36,6 @@ enum
   GCI = 0,
   SAV = 0x80,
   GCS = 0x110,
-
-  CI8SHARED = 1,
-  RGB5A3,
-  CI8,
 };
 
 enum class GCMemcardGetSaveDataRetVal
@@ -105,6 +102,12 @@ private:
   std::bitset<static_cast<size_t>(GCMemcardValidityIssues::COUNT)> m_errors;
 };
 
+struct GCMemcardAnimationFrameRGBA8
+{
+  std::vector<u32> image_data;
+  u8 delay;
+};
+
 // size of a single memory card block in bytes
 constexpr u32 BLOCK_SIZE = 0x2000;
 
@@ -117,7 +120,7 @@ constexpr u32 MC_FST_BLOCKS = 0x05;
 // maximum number of saves that can be stored on a single memory card
 constexpr u8 DIRLEN = 0x7F;
 
-// maximum size of memory card file comment in bytes
+// maximum size of a single memory card file comment in bytes
 constexpr u32 DENTRY_STRLEN = 0x20;
 
 // size of a single entry in the Directory in bytes
@@ -136,6 +139,30 @@ constexpr u16 MBIT_SIZE_MEMORY_CARD_507 = 0x20;
 constexpr u16 MBIT_SIZE_MEMORY_CARD_1019 = 0x40;
 constexpr u16 MBIT_SIZE_MEMORY_CARD_2043 = 0x80;
 
+// width and height of a save file's banner in pixels
+constexpr u32 MEMORY_CARD_BANNER_WIDTH = 96;
+constexpr u32 MEMORY_CARD_BANNER_HEIGHT = 32;
+
+// color format of banner as stored in the lowest two bits of m_banner_and_icon_flags
+constexpr u8 MEMORY_CARD_BANNER_FORMAT_CI8 = 1;
+constexpr u8 MEMORY_CARD_BANNER_FORMAT_RGB5A3 = 2;
+
+// width and height of a save file's icon in pixels
+constexpr u32 MEMORY_CARD_ICON_WIDTH = 32;
+constexpr u32 MEMORY_CARD_ICON_HEIGHT = 32;
+
+// maximum number of frames a save file's icon animation can have
+constexpr u32 MEMORY_CARD_ICON_ANIMATION_MAX_FRAMES = 8;
+
+// color format of icon frame as stored in m_icon_format (two bits per frame)
+constexpr u8 MEMORY_CARD_ICON_FORMAT_CI8_SHARED_PALETTE = 1;
+constexpr u8 MEMORY_CARD_ICON_FORMAT_RGB5A3 = 2;
+constexpr u8 MEMORY_CARD_ICON_FORMAT_CI8_UNIQUE_PALETTE = 3;
+
+// number of palette entries in a CI8 palette of a banner or icon
+// each palette entry is 16 bits in RGB5A3 format
+constexpr u32 MEMORY_CARD_CI8_PALETTE_ENTRIES = 256;
+
 class MemoryCardBase
 {
 public:
@@ -243,15 +270,13 @@ struct DEntry
   u8 m_unused_1;
 
   // 1 byte at 0x07: banner gfx format and icon animation (Image Key)
-  //      Bit(s)  Description
-  //      2       Icon Animation 0: forward 1: ping-pong
-  //      1       [--0: No Banner 1: Banner present--] WRONG! YAGCD LIES!
-  //      0       [--Banner Color 0: RGB5A3 1: CI8--]  WRONG! YAGCD LIES!
-  //      bits 0 and 1: image format
-  //      00 no banner
-  //      01 CI8 banner
-  //      10 RGB5A3 banner
-  //      11 ? maybe ==00? Time Splitters 2 and 3 have it and don't have banner
+  // First two bits are used for the banner format.
+  // YAGCD is wrong about the meaning of these.
+  // '0' and '3' both mean no banner.
+  // '1' means paletted (8 bits per pixel palette entry + 16 bit color palette in RGB5A3)
+  // '2' means direct color (16 bits per pixel in RGB5A3)
+  // Third bit is icon animation frame order, 0 for loop (abcabcabc), 1 for ping-pong (abcbabcba).
+  // Remaining bits seem unused.
   u8 m_banner_and_icon_flags;
 
   // 0x20 bytes at 0x08: Filename
@@ -450,9 +475,15 @@ public:
   u16 DEntry_FirstBlock(u8 index) const;
   // get file length in blocks
   u16 DEntry_BlockCount(u8 index) const;
-  u32 DEntry_CommentsAddress(u8 index) const;
-  std::string GetSaveComment1(u8 index) const;
-  std::string GetSaveComment2(u8 index) const;
+
+  std::optional<std::vector<u8>>
+  GetSaveDataBytes(u8 save_index, size_t offset = 0,
+                   size_t length = std::numeric_limits<size_t>::max()) const;
+
+  // Returns, if available, the two strings shown on the save file in the GC BIOS, in UTF8.
+  // The first is the big line on top, usually the game title, and the second is the smaller line
+  // next to the block size, often a progress indicator or subtitle.
+  std::optional<std::pair<std::string, std::string>> GetSaveComments(u8 index) const;
 
   // Fetches a DEntry from the given file index.
   std::optional<DEntry> GetDEntry(u8 index) const;
@@ -480,8 +511,8 @@ public:
   static void Gcs_SavConvert(DEntry& tempDEntry, int saveType, u64 length = BLOCK_SIZE);
 
   // reads the banner image
-  bool ReadBannerRGBA8(u8 index, u32* buffer) const;
+  std::optional<std::vector<u32>> ReadBannerRGBA8(u8 index) const;
 
   // reads the animation frames
-  u32 ReadAnimRGBA8(u8 index, u32* buffer, u8* delays) const;
+  std::optional<std::vector<GCMemcardAnimationFrameRGBA8>> ReadAnimRGBA8(u8 index) const;
 };
diff --git a/Source/Core/DolphinQt/GCMemcardManager.cpp b/Source/Core/DolphinQt/GCMemcardManager.cpp
index 6ff97650e8..49f0f37f2c 100644
--- a/Source/Core/DolphinQt/GCMemcardManager.cpp
+++ b/Source/Core/DolphinQt/GCMemcardManager.cpp
@@ -32,11 +32,6 @@
 
 #include "DolphinQt/QtUtils/ModalMessageBox.h"
 
-constexpr u32 BANNER_WIDTH = 96;
-constexpr u32 BANNER_HEIGHT = 32;
-constexpr u32 ICON_WIDTH = 32;
-constexpr u32 ICON_HEIGHT = 32;
-constexpr u32 ANIM_MAX_FRAMES = 8;
 constexpr float ROW_HEIGHT = 28;
 
 struct GCMemcardManager::IconAnimationData
@@ -206,14 +201,6 @@ void GCMemcardManager::UpdateSlotTable(int slot)
     return item;
   };
 
-  const auto strip_garbage = [](const std::string& s) {
-    auto offset = s.find('\0');
-    if (offset == std::string::npos)
-      offset = s.length();
-
-    return s.substr(0, offset);
-  };
-
   const u8 num_files = memcard->GetNumFiles();
   m_slot_active_icons[slot].reserve(num_files);
   for (int i = 0; i < num_files; i++)
@@ -221,12 +208,16 @@ void GCMemcardManager::UpdateSlotTable(int slot)
     int file_index = memcard->GetFileIndex(i);
     table->setRowCount(i + 1);
 
-    auto const string_decoder = memcard->IsShiftJIS() ? SHIFTJISToUTF8 : CP1252ToUTF8;
+    const auto file_comments = memcard->GetSaveComments(file_index);
+
+    QString title;
+    QString comment;
+    if (file_comments)
+    {
+      title = QString::fromStdString(file_comments->first);
+      comment = QString::fromStdString(file_comments->second);
+    }
 
-    QString title =
-        QString::fromStdString(strip_garbage(string_decoder(memcard->GetSaveComment1(file_index))));
-    QString comment =
-        QString::fromStdString(strip_garbage(string_decoder(memcard->GetSaveComment2(file_index))));
     QString blocks = QStringLiteral("%1").arg(memcard->DEntry_BlockCount(file_index));
     QString block_count = QStringLiteral("%1").arg(memcard->DEntry_FirstBlock(file_index));
 
@@ -496,13 +487,13 @@ QPixmap GCMemcardManager::GetBannerFromSaveFile(int file_index, int slot)
 {
   auto& memcard = m_slot_memcard[slot];
 
-  std::vector<u32> pxdata(BANNER_WIDTH * BANNER_HEIGHT);
+  auto pxdata = memcard->ReadBannerRGBA8(file_index);
 
   QImage image;
-  if (memcard->ReadBannerRGBA8(file_index, pxdata.data()))
+  if (pxdata)
   {
-    image = QImage(reinterpret_cast<u8*>(pxdata.data()), BANNER_WIDTH, BANNER_HEIGHT,
-                   QImage::Format_ARGB32);
+    image = QImage(reinterpret_cast<u8*>(pxdata->data()), MEMORY_CARD_BANNER_WIDTH,
+                   MEMORY_CARD_BANNER_HEIGHT, QImage::Format_ARGB32);
   }
 
   return QPixmap::fromImage(image);
@@ -512,39 +503,36 @@ GCMemcardManager::IconAnimationData GCMemcardManager::GetIconFromSaveFile(int fi
 {
   auto& memcard = m_slot_memcard[slot];
 
-  std::vector<u8> anim_delay(ANIM_MAX_FRAMES);
-  std::vector<u32> anim_data(ICON_WIDTH * ICON_HEIGHT * ANIM_MAX_FRAMES);
-
   IconAnimationData frame_data;
 
-  const u32 num_frames = memcard->ReadAnimRGBA8(file_index, anim_data.data(), anim_delay.data());
+  const auto decoded_data = memcard->ReadAnimRGBA8(file_index);
 
   // Decode Save File Animation
-  if (num_frames > 0)
+  if (decoded_data && !decoded_data->empty())
   {
-    frame_data.m_frames.reserve(num_frames);
-    const u32 per_frame_offset = ICON_WIDTH * ICON_HEIGHT;
-    for (u32 f = 0; f < num_frames; ++f)
+    frame_data.m_frames.reserve(decoded_data->size());
+    const u32 per_frame_offset = MEMORY_CARD_ICON_WIDTH * MEMORY_CARD_ICON_HEIGHT;
+    for (size_t f = 0; f < decoded_data->size(); ++f)
     {
-      QImage img(reinterpret_cast<u8*>(&anim_data[f * per_frame_offset]), ICON_WIDTH, ICON_HEIGHT,
-                 QImage::Format_ARGB32);
+      QImage img(reinterpret_cast<const u8*>((*decoded_data)[f].image_data.data()),
+                 MEMORY_CARD_ICON_WIDTH, MEMORY_CARD_ICON_HEIGHT, QImage::Format_ARGB32);
       frame_data.m_frames.push_back(QPixmap::fromImage(img));
-      for (int i = 0; i < anim_delay[f]; ++i)
+      for (int i = 0; i < (*decoded_data)[f].delay; ++i)
       {
         frame_data.m_frame_timing.push_back(static_cast<u8>(f));
       }
     }
 
     const bool is_pingpong = memcard->DEntry_IsPingPong(file_index);
-    if (is_pingpong && num_frames >= 3)
+    if (is_pingpong && decoded_data->size() >= 3)
     {
       // if the animation 'ping-pongs' between start and end then the animation frame order is
       // something like 'abcdcbabcdcba' instead of the usual 'abcdabcdabcd'
       // to display that correctly just append all except the first and last frame in reverse order
       // at the end of the animation
-      for (u32 f = num_frames - 2; f > 0; --f)
+      for (size_t f = decoded_data->size() - 2; f > 0; --f)
       {
-        for (int i = 0; i < anim_delay[f]; ++i)
+        for (int i = 0; i < (*decoded_data)[f].delay; ++i)
         {
           frame_data.m_frame_timing.push_back(static_cast<u8>(f));
         }