Fix transcoding status update for ContentResolver requests
When a transcoding job completes successfully, we update a database
row for transcode_status to 'completed'. This update was failing
because extra SQL 'WHERE' clauses were attached to the update SQL to
match the calling apps owner_package_name.
This was a problem only for ContentResolver updates because in those
cases we were running as the calling identity of the caller.
-Now we force all transcode status updates to run as the MediaProvider
calling identity.
-Added @Ignore regression test currently blocked by b/174655855
-Rewrote the assertTranscode util to only check read(2) duration. This
should be more reliable across file path and ContentResolver
especially as open latency can be very different in either case.
Bug: 174521420
Test: atest TranscodeTest
Change-Id: Ibb71876b5c92a730442398bff4ba950a96b5b3f4
diff --git a/src/com/android/providers/media/MediaProvider.java b/src/com/android/providers/media/MediaProvider.java
index d92e17f..43083b4 100644
--- a/src/com/android/providers/media/MediaProvider.java
+++ b/src/com/android/providers/media/MediaProvider.java
@@ -4042,7 +4042,15 @@
*/
@NonNull SQLiteQueryBuilder getQueryBuilderForTranscoding(int type, int match,
@NonNull Uri uri, @NonNull Bundle extras, @Nullable Consumer<String> honored) {
- return getQueryBuilder(type, match, uri, extras, honored);
+ // Force MediaProvider calling identity when accessing the db from transcoding to avoid
+ // generating 'strict' SQL e.g forcing owner_package_name matches
+ // We already handle the required permission checks for the app before we get here
+ final LocalCallingIdentity token = clearLocalCallingIdentity();
+ try {
+ return getQueryBuilder(type, match, uri, extras, honored);
+ } finally {
+ restoreLocalCallingIdentity(token);
+ }
}
/**
@@ -6515,6 +6523,7 @@
Log.d(TAG, "Using FUSE with transcode cache for " + filePath + " Uid: " + uid);
return FileUtils.openSafely(getFuseFile(transcodeFile), modeBits);
} else if (mTranscodeHelper.transcode(filePath, transcodePath, uid)) {
+ // TODO(b/174655855): We should transcode lazily and just return the opened fd here
Log.d(TAG, "Using FUSE with transcode for " + filePath + " Uid: " + uid);
return FileUtils.openSafely(getFuseFile(transcodeFile), modeBits);
} else {
diff --git a/src/com/android/providers/media/TranscodeHelper.java b/src/com/android/providers/media/TranscodeHelper.java
index c8b9a32..dcd40f4 100644
--- a/src/com/android/providers/media/TranscodeHelper.java
+++ b/src/com/android/providers/media/TranscodeHelper.java
@@ -598,8 +598,12 @@
ContentValues values = new ContentValues();
values.put(FileColumns._TRANSCODE_STATUS, transcodeStatus);
- return qb.update(getDatabaseHelperForUri(uri), values, TRANSCODE_WHERE_CLAUSE,
- selectionArgs) == 1;
+ final boolean success = qb.update(getDatabaseHelperForUri(uri), values,
+ TRANSCODE_WHERE_CLAUSE, selectionArgs) == 1;
+ if (!success) {
+ Log.w(TAG, "Transcoding status update to: " + transcodeStatus + " failed for " + path);
+ }
+ return success;
}
public boolean deleteCachedTranscodeFile(long rowId) {
diff --git a/tests/transcode/src/com/android/providers/media/transcode/TranscodeTest.java b/tests/transcode/src/com/android/providers/media/transcode/TranscodeTest.java
index 1eec3bc..236f626 100644
--- a/tests/transcode/src/com/android/providers/media/transcode/TranscodeTest.java
+++ b/tests/transcode/src/com/android/providers/media/transcode/TranscodeTest.java
@@ -51,6 +51,7 @@
import org.junit.After;
import org.junit.Before;
+import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -325,6 +326,82 @@
}
/**
+ * Tests that transcode cache is reused after file path transcode
+ * @throws Exception
+ */
+ @Test
+ public void testTranscodedCacheReuse_FilePath() throws Exception {
+ File modernFile = new File(DIR_CAMERA, HEVC_FILE_NAME);
+ try {
+ TranscodeTestUtils.stageHEVCVideoFile(modernFile);
+ TranscodeTestUtils.enableTranscodingForUid(Os.getuid());
+
+ assertTranscode(modernFile, true);
+ assertTranscode(modernFile, false);
+ } finally {
+ modernFile.delete();
+ }
+ }
+
+ /**
+ * Tests that transcode cache is reused after ContentResolver transcode
+ * @throws Exception
+ */
+ @Ignore("b/174655855")
+ @Test
+ public void testTranscodedCacheReuse_ContentResolver() throws Exception {
+ File modernFile = new File(DIR_CAMERA, HEVC_FILE_NAME);
+ try {
+ Uri uri = TranscodeTestUtils.stageHEVCVideoFile(modernFile);
+ TranscodeTestUtils.enableTranscodingForUid(Os.getuid());
+
+ assertTranscode(uri, true);
+ assertTranscode(uri, false);
+ } finally {
+ modernFile.delete();
+ }
+ }
+
+ /**
+ * Tests that transcode cache is reused after ContentResolver transcode
+ * and file path opens
+ * @throws Exception
+ */
+ @Ignore("b/174655855")
+ @Test
+ public void testTranscodedCacheReuse_ContentResolverFilePath() throws Exception {
+ File modernFile = new File(DIR_CAMERA, HEVC_FILE_NAME);
+ try {
+ Uri uri = TranscodeTestUtils.stageHEVCVideoFile(modernFile);
+ TranscodeTestUtils.enableTranscodingForUid(Os.getuid());
+
+ assertTranscode(uri, true);
+ assertTranscode(modernFile, false);
+ } finally {
+ modernFile.delete();
+ }
+ }
+
+ /**
+ * Tests that transcode cache is reused after file path transcode
+ * and ContentResolver opens
+ * @throws Exception
+ */
+ @Test
+ public void testTranscodedCacheReuse_FilePathContentResolver() throws Exception {
+ File modernFile = new File(DIR_CAMERA, HEVC_FILE_NAME);
+ try {
+ Uri uri = TranscodeTestUtils.stageHEVCVideoFile(modernFile);
+ TranscodeTestUtils.enableTranscodingForUid(Os.getuid());
+
+ assertTranscode(modernFile, true);
+ assertTranscode(uri, false);
+ } finally {
+ modernFile.delete();
+ }
+ }
+
+ /**
* Tests that transcode cache is reused after rename
* @throws Exception
*/
diff --git a/tests/transcode/src/com/android/providers/media/transcode/TranscodeTestUtils.java b/tests/transcode/src/com/android/providers/media/transcode/TranscodeTestUtils.java
index 5bddab2..63fc57a 100644
--- a/tests/transcode/src/com/android/providers/media/transcode/TranscodeTestUtils.java
+++ b/tests/transcode/src/com/android/providers/media/transcode/TranscodeTestUtils.java
@@ -390,24 +390,29 @@
assertEquals(message, isSame, assertSame);
}
- public static void assertTranscode(File file, boolean transcode) throws Exception {
+ public static void assertTranscode(Uri uri, boolean transcode) throws Exception {
long start = SystemClock.elapsedRealtimeNanos();
- ParcelFileDescriptor pfd = open(file, false);
- long end = SystemClock.elapsedRealtimeNanos();
- long openDuration = end - start;
+ assertTranscode(open(uri, true, null /* bundle */), transcode);
+ }
- start = SystemClock.elapsedRealtimeNanos();
+ public static void assertTranscode(File file, boolean transcode) throws Exception {
+ assertTranscode(open(file, false), transcode);
+ }
+
+ public static void assertTranscode(ParcelFileDescriptor pfd, boolean transcode)
+ throws Exception {
+ long start = SystemClock.elapsedRealtimeNanos();
assertEquals(10, Os.pread(pfd.getFileDescriptor(), new byte[10], 0, 10, 0));
- end = SystemClock.elapsedRealtimeNanos();
+ long end = SystemClock.elapsedRealtimeNanos();
long readDuration = end - start;
- // With transcoding read(2) dominates open(2)
- // Without transcoding open(2) dominates IO
- String message = "readDuration=" + readDuration + "ns. openDuration=" + openDuration + "ns";
+ // With transcoding read(2) > 100ms (usually > 1s)
+ // Without transcoding read(2) < 10ms (usually < 1ms)
+ String message = "readDuration=" + readDuration + "ns";
if (transcode) {
- assertTrue(message, readDuration > openDuration);
+ assertTrue(message, readDuration > TimeUnit.MILLISECONDS.toNanos(100));
} else {
- assertTrue(message, openDuration > readDuration);
+ assertTrue(message, readDuration < TimeUnit.MILLISECONDS.toNanos(10));
}
}
}