From 49be09e5b20a01d0b528f458ee88b4959a08b0e1 Mon Sep 17 00:00:00 2001
From: Zaggy1024 <zaggy1024@gmail.com>
Date: Fri, 25 Aug 2023 18:54:06 -0500
Subject: [PATCH] LibAudio: Skip ID3 tags before synchronizing to MP3 frames

Previously, we would just start from byte 0 and check individual bytes
of the file until we find two bytes starting with `FF F`, and then
assume that that was the MP3 frame sync code. However, some ID3v2 tags
do not have to be what is referred to as "unsynchronized", meaning that
they can contain that `FF F` sequence and cause our decoder to think it
has found a frame.

To avoid this happening, we can read a minimal amount of the ID3 header
to determine how many bytes to skip before attempting to find the MP3
frames.

This allows the recent podcast with Andreas to play here:

https://changelog.com/podcast/554
---
 Userland/Libraries/LibAudio/MP3Loader.cpp | 33 ++++++++++++++++++++---
 Userland/Libraries/LibAudio/MP3Loader.h   |  1 +
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/Userland/Libraries/LibAudio/MP3Loader.cpp b/Userland/Libraries/LibAudio/MP3Loader.cpp
index 5c029a0112..af749c46bb 100644
--- a/Userland/Libraries/LibAudio/MP3Loader.cpp
+++ b/Userland/Libraries/LibAudio/MP3Loader.cpp
@@ -22,8 +22,36 @@ MP3LoaderPlugin::MP3LoaderPlugin(NonnullOwnPtr<SeekableStream> stream)
 {
 }
 
+MaybeLoaderError MP3LoaderPlugin::skip_id3(SeekableStream& stream)
+{
+    // FIXME: This is a bit of a hack until we have a proper ID3 reader and MP3 demuxer.
+    // Based on https://mutagen-specs.readthedocs.io/en/latest/id3/id3v2.2.html
+    char identifier_buffer[3] = { 0, 0, 0 };
+    auto read_identifier = StringView(TRY(stream.read_some({ &identifier_buffer[0], sizeof(identifier_buffer) })));
+    if (read_identifier == "ID3"sv) {
+        [[maybe_unused]] auto version = TRY(stream.read_value<u8>());
+        [[maybe_unused]] auto revision = TRY(stream.read_value<u8>());
+        [[maybe_unused]] auto flags = TRY(stream.read_value<u8>());
+        auto size = 0;
+        for (auto i = 0; i < 4; i++) {
+            // Each byte has a zeroed most significant bit to prevent it from looking like a sync code.
+            auto byte = TRY(stream.read_value<u8>());
+            size <<= 7;
+            size |= byte & 0x7F;
+        }
+        TRY(stream.seek(size, SeekMode::FromCurrentPosition));
+    } else if (read_identifier != "TAG"sv) {
+        MUST(stream.seek(-static_cast<int>(read_identifier.length()), SeekMode::FromCurrentPosition));
+    }
+    return {};
+}
+
 bool MP3LoaderPlugin::sniff(SeekableStream& stream)
 {
+    auto skip_id3_result = skip_id3(stream);
+    if (skip_id3_result.is_error())
+        return false;
+
     auto maybe_bit_stream = try_make<BigEndianInputBitStream>(MaybeOwned<Stream>(stream));
     if (maybe_bit_stream.is_error())
         return false;
@@ -55,8 +83,8 @@ ErrorOr<NonnullOwnPtr<LoaderPlugin>, LoaderError> MP3LoaderPlugin::create(Nonnul
 
 MaybeLoaderError MP3LoaderPlugin::initialize()
 {
+    TRY(skip_id3(*m_stream));
     m_bitstream = TRY(try_make<BigEndianInputBitStream>(MaybeOwned<Stream>(*m_stream)));
-
     TRY(synchronize());
 
     auto header = TRY(read_header());
@@ -68,8 +96,7 @@ MaybeLoaderError MP3LoaderPlugin::initialize()
     m_loaded_samples = 0;
 
     TRY(build_seek_table());
-
-    TRY(m_stream->seek(0, SeekMode::SetPosition));
+    TRY(seek(0));
 
     return {};
 }
diff --git a/Userland/Libraries/LibAudio/MP3Loader.h b/Userland/Libraries/LibAudio/MP3Loader.h
index f6e5421f45..0d0fcdf92c 100644
--- a/Userland/Libraries/LibAudio/MP3Loader.h
+++ b/Userland/Libraries/LibAudio/MP3Loader.h
@@ -41,6 +41,7 @@ public:
 
 private:
     MaybeLoaderError initialize();
+    static MaybeLoaderError skip_id3(SeekableStream& stream);
     static MaybeLoaderError synchronize(BigEndianInputBitStream& stream, size_t sample_index);
     MaybeLoaderError synchronize();
     MaybeLoaderError build_seek_table();