Fix handling of ID3v2.4 extended header
If an optional ID3v2.4 extended header was present, it would be
treated as a frame, causing parse errors. Fix by doing ID3v2.4
per-frame unsynchronization after skipping over the extended
header.
Bug: 154357105
Bug: 151448144
Test: CTS, manual w/ temporary additional logging.
Change-Id: Iaeb8cab866b98dcf97f81d3ade4a1c8d26add3a5
diff --git a/media/libstagefright/id3/ID3.cpp b/media/libstagefright/id3/ID3.cpp
index 425468f..e97f6eb 100644
--- a/media/libstagefright/id3/ID3.cpp
+++ b/media/libstagefright/id3/ID3.cpp
@@ -107,20 +107,6 @@
}
}
-ID3::ID3(DataSourceBase *source, bool ignoreV1, off64_t offset)
- : mIsValid(false),
- mData(NULL),
- mSize(0),
- mFirstFrameOffset(0),
- mVersion(ID3_UNKNOWN),
- mRawSize(0) {
- mIsValid = parseV2(source, offset);
-
- if (!mIsValid && !ignoreV1) {
- mIsValid = parseV1(source);
- }
-}
-
ID3::ID3(const uint8_t *data, size_t size, bool ignoreV1)
: mIsValid(false),
mData(NULL),
@@ -247,44 +233,14 @@
return false;
}
- if (header.version_major == 4) {
- void *copy = malloc(size);
- if (copy == NULL) {
- free(mData);
- mData = NULL;
- ALOGE("b/24623447, no more memory");
- return false;
- }
-
- memcpy(copy, mData, size);
-
- bool success = removeUnsynchronizationV2_4(false /* iTunesHack */);
- if (!success) {
- memcpy(mData, copy, size);
- mSize = size;
-
- success = removeUnsynchronizationV2_4(true /* iTunesHack */);
-
- if (success) {
- ALOGV("Had to apply the iTunes hack to parse this ID3 tag");
- }
- }
-
- free(copy);
- copy = NULL;
-
- if (!success) {
- free(mData);
- mData = NULL;
-
- return false;
- }
- } else if (header.flags & 0x80) {
+ // first handle global unsynchronization
+ if (header.flags & 0x80) {
ALOGV("removing unsynchronization");
removeUnsynchronization();
}
+ // handle extended header, if present
mFirstFrameOffset = 0;
if (header.version_major == 3 && (header.flags & 0x40)) {
// Version 2.3 has an optional extended header.
@@ -296,6 +252,7 @@
return false;
}
+ // v2.3 does not have syncsafe integers
size_t extendedHeaderSize = U32_AT(&mData[0]);
if (extendedHeaderSize > SIZE_MAX - 4) {
free(mData);
@@ -367,6 +324,48 @@
mFirstFrameOffset = ext_size;
}
+ // Handle any v2.4 per-frame unsynchronization
+ // The id3 spec isn't clear about what should happen if the global
+ // unsynchronization flag is combined with per-frame unsynchronization,
+ // or whether that's even allowed, so this code assumes id3 writing
+ // tools do the right thing and not apply double-unsynchronization,
+ // but will honor the flags if they are set.
+ if (header.version_major == 4) {
+ void *copy = malloc(size);
+ if (copy == NULL) {
+ free(mData);
+ mData = NULL;
+ ALOGE("b/24623447, no more memory");
+ return false;
+ }
+
+ memcpy(copy, mData, size);
+
+ bool success = removeUnsynchronizationV2_4(false /* iTunesHack */);
+ if (!success) {
+ memcpy(mData, copy, size);
+ mSize = size;
+
+ success = removeUnsynchronizationV2_4(true /* iTunesHack */);
+
+ if (success) {
+ ALOGV("Had to apply the iTunes hack to parse this ID3 tag");
+ }
+ }
+
+ free(copy);
+ copy = NULL;
+
+ if (!success) {
+ free(mData);
+ mData = NULL;
+
+ return false;
+ }
+ }
+
+
+
if (header.version_major == 2) {
mVersion = ID3_V2_2;
} else if (header.version_major == 3) {
@@ -411,7 +410,7 @@
bool ID3::removeUnsynchronizationV2_4(bool iTunesHack) {
size_t oldSize = mSize;
- size_t offset = 0;
+ size_t offset = mFirstFrameOffset;
while (mSize >= 10 && offset <= mSize - 10) {
if (!memcmp(&mData[offset], "\0\0\0\0", 4)) {
break;
@@ -445,7 +444,7 @@
}
if ((flags & 2) && (dataSize >= 2)) {
- // This file has "unsynchronization", so we have to replace occurrences
+ // This frame has "unsynchronization", so we have to replace occurrences
// of 0xff 0x00 with just 0xff in order to get the real data.
size_t readOffset = offset + 11;
diff --git a/media/libstagefright/id3/test/ID3Test.cpp b/media/libstagefright/id3/test/ID3Test.cpp
index a8f1470..cd5cd9e 100644
--- a/media/libstagefright/id3/test/ID3Test.cpp
+++ b/media/libstagefright/id3/test/ID3Test.cpp
@@ -24,6 +24,7 @@
#include <datasource/FileSource.h>
#include <media/stagefright/foundation/hexdump.h>
+#include <media/MediaExtractorPluginHelper.h>
#include <ID3.h>
#include "ID3TestEnvironment.h"
@@ -42,7 +43,8 @@
string path = gEnv->getRes() + GetParam();
sp<FileSource> file = new FileSource(path.c_str());
ASSERT_EQ(file->initCheck(), (status_t)OK) << "File initialization failed! \n";
- ID3 tag(file.get());
+ DataSourceHelper helper(file->wrap());
+ ID3 tag(&helper);
ASSERT_TRUE(tag.isValid()) << "No valid ID3 tag found for " << path.c_str() << "\n";
ID3::Iterator it(tag, nullptr);
@@ -61,7 +63,8 @@
sp<android::FileSource> file = new FileSource(path.c_str());
ASSERT_EQ(file->initCheck(), (status_t)OK) << "File initialization failed! \n";
- ID3 tag(file.get());
+ DataSourceHelper helper(file->wrap());
+ ID3 tag(&helper);
ASSERT_TRUE(tag.isValid()) << "No valid ID3 tag found for " << path.c_str() << "\n";
ASSERT_TRUE(tag.version() >= versionNumber)
<< "Expected version: " << tag.version() << " Found version: " << versionNumber;
@@ -73,7 +76,8 @@
sp<android::FileSource> file = new FileSource(path.c_str());
ASSERT_EQ(file->initCheck(), (status_t)OK) << "File initialization failed! \n";
- ID3 tag(file.get());
+ DataSourceHelper helper(file->wrap());
+ ID3 tag(&helper);
ASSERT_TRUE(tag.isValid()) << "No valid ID3 tag found for " << path.c_str() << "\n";
int countTextFrames = 0;
ID3::Iterator it(tag, nullptr);
@@ -99,7 +103,8 @@
sp<android::FileSource> file = new FileSource(path.c_str());
ASSERT_EQ(file->initCheck(), (status_t)OK) << "File initialization failed! \n";
- ID3 tag(file.get());
+ DataSourceHelper helper(file->wrap());
+ ID3 tag(&helper);
ASSERT_TRUE(tag.isValid()) << "No valid ID3 tag found for " << path.c_str() << "\n";
size_t dataSize;
String8 mime;
@@ -124,7 +129,8 @@
sp<android::FileSource> file = new FileSource(path.c_str());
ASSERT_EQ(file->initCheck(), (status_t)OK) << "File initialization failed! \n";
- ID3 tag(file.get());
+ DataSourceHelper helper(file->wrap());
+ ID3 tag(&helper);
ASSERT_TRUE(tag.isValid()) << "No valid ID3 tag found for " << path.c_str() << "\n";
int count = 0;
ID3::Iterator it(tag, nullptr);
diff --git a/media/libstagefright/id3/testid3.cpp b/media/libstagefright/id3/testid3.cpp
index 9984d85..5cd51cf 100644
--- a/media/libstagefright/id3/testid3.cpp
+++ b/media/libstagefright/id3/testid3.cpp
@@ -24,6 +24,7 @@
#include <binder/ProcessState.h>
#include <datasource/FileSource.h>
#include <media/stagefright/foundation/ADebug.h>
+#include <media/MediaExtractorPluginHelper.h>
#define MAXPATHLEN 256
@@ -72,7 +73,8 @@
sp<FileSource> file = new FileSource(path);
CHECK_EQ(file->initCheck(), (status_t)OK);
- ID3 tag(file.get());
+ DataSourceHelper helper(file->wrap());
+ ID3 tag(&helper);
if (!tag.isValid()) {
printf("FAIL %s\n", path);
} else {
diff --git a/media/libstagefright/include/ID3.h b/media/libstagefright/include/ID3.h
index 2843a7a..0be5896 100644
--- a/media/libstagefright/include/ID3.h
+++ b/media/libstagefright/include/ID3.h
@@ -37,7 +37,6 @@
};
explicit ID3(DataSourceHelper *source, bool ignoreV1 = false, off64_t offset = 0);
- explicit ID3(DataSourceBase *source, bool ignoreV1 = false, off64_t offset = 0);
ID3(const uint8_t *data, size_t size, bool ignoreV1 = false);
~ID3();