Add opt-in video disk cache for ExoPlayer playback#6542
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
SDK Size Comparison 📏
|
|
WalkthroughThis PR introduces an opt-in on-disk video cache for streamed media. It adds ChangesVideo Disk Cache Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
stream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientCacheAndTemporaryFilesTest.kt (1)
73-108: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid process-wide
clearAll()as the signal for deleting this directory.
VideoMediaCache.clearAll()returnstruefor any live cache in the JVM, even one owned by another test. That can skipfileManager.clearVideoCache(context)here and leavevideoCachebehind, making this assertion order-dependent. Make the delete decision based on whether this directory is actually owned, not whether some otherVideoMediaCacheexists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientCacheAndTemporaryFilesTest.kt` around lines 73 - 108, The cache cleanup test is using the global VideoMediaCache clearAll() state to decide whether this specific directory should be deleted, which makes the assertion dependent on other tests. Update the cleanup logic around chatClient.clearCacheAndTemporaryFiles and fileManager.clearVideoCache(context) so it checks ownership of the current videoCache directory instead of any live cache instance, and ensure the test always verifies deletion of the directory created in this test.
🧹 Nitpick comments (2)
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoCacheDataSourceFactory.kt (1)
40-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider interface delegation for
DataSource.Factory.Static analysis flags this class for interface delegation via
by.cacheKeyForonly touchesdataSpec(no instance state), so it could be extracted to a top-level/private function, lettingdelegatebe delegated directly in the class header.♻️ Proposed refactor
+private fun cacheKeyFor(dataSpec: DataSpec): String = + dataSpec.key ?: dataSpec.uri.buildUpon().clearQuery().build().toString() + `@OptIn`(UnstableApi::class) internal class VideoCacheDataSourceFactory( videoCache: VideoMediaCache, upstreamFactory: DataSource.Factory, -) : DataSource.Factory { - - private val delegate: DataSource.Factory = CacheDataSource.Factory() - .setCache(videoCache.cache) - .setUpstreamDataSourceFactory(upstreamFactory) - .setCacheKeyFactory(::cacheKeyFor) - .setFlags(CacheDataSource.FLAG_IGNORE_CACHE_ON_ERROR) - - override fun createDataSource(): DataSource = delegate.createDataSource() - - /** - * Returns the cache key for [dataSpec]. Strips the URI's query so rotating signature or expiry - * parameters on the same path land on the same cache entry; a caller-supplied [DataSpec.key] is - * honoured when present. - */ - `@OptIn`(UnstableApi::class) - private fun cacheKeyFor(dataSpec: DataSpec): String = - dataSpec.key ?: dataSpec.uri.buildUpon().clearQuery().build().toString() -} +) : DataSource.Factory by CacheDataSource.Factory() + .setCache(videoCache.cache) + .setUpstreamDataSourceFactory(upstreamFactory) + .setCacheKeyFactory(::cacheKeyFor) + .setFlags(CacheDataSource.FLAG_IGNORE_CACHE_ON_ERROR)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoCacheDataSourceFactory.kt` around lines 40 - 61, The VideoCacheDataSourceFactory class can be simplified by using interface delegation for DataSource.Factory instead of manually forwarding createDataSource through a delegate property. Move cacheKeyFor out of the class as a private top-level helper since it only depends on DataSpec, then delegate DataSource.Factory directly in the class declaration and keep the cache setup logic intact.Source: Linters/SAST tools
stream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientCacheAndTemporaryFilesTest.kt (1)
56-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd video cache cleanup to
tearDown().Unlike stream/image/timestamped caches,
clearAllCacheintentionally never touches the video cache directory (perStreamFileManager.clearAllCache). This test creates a video cache dir but relies solely on the in-testclearCacheAndTemporaryFilescall to remove it;tearDown()provides no fallback if that assertion ever fails, so a video cache directory could leak into subsequent test runs.♻️ Proposed defensive cleanup
`@After` fun tearDown() { // Clean up all cache and external storage after each test streamFileManager.clearAllCache(context) streamFileManager.clearExternalStorage(context) + streamFileManager.clearVideoCache(context) }Also applies to: 73-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientCacheAndTemporaryFilesTest.kt` around lines 56 - 61, The tearDown cleanup in ChatClientCacheAndTemporaryFilesTest does not remove the video cache directory because StreamFileManager.clearAllCache intentionally skips it, so add explicit video cache cleanup alongside the existing clearAllCache and clearExternalStorage calls. Update the test’s tearDown() to invoke the appropriate StreamFileManager cleanup for the video cache path so it always gets removed even if the in-test clearCacheAndTemporaryFiles assertion fails. Apply the same defensive cleanup to the other referenced test block as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt`:
- Around line 137-156: The VideoMediaCache.create(...) path currently constructs
SimpleCache directly, which can throw and abort
ChatClient.Builder.internalBuild() when the cache dir is locked or unusable.
Wrap the SimpleCache initialization in create(...) with failure handling, and on
error fall back to returning no video cache instead of propagating the
exception. Keep the guard localized to VideoMediaCache.create and preserve the
existing reuse behavior in the instances map when cache creation succeeds.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt`:
- Around line 1517-1532: The video cache cleanup in ChatClient should not decide
between clearing live caches and deleting the directory in two separate steps.
Update the cache removal flow around VideoMediaCache.clearAll() so the “clear
live caches or delete on disk” decision is handled atomically under the registry
lock, and remove the fallback delete from ChatClient’s cache-clearing path. Use
the existing VideoMediaCache and fileManager.clearVideoCache(context) symbols to
locate the affected logic and route the fallback through a single synchronized
API that prevents a newly created cache from being deleted mid-flight.
---
Outside diff comments:
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientCacheAndTemporaryFilesTest.kt`:
- Around line 73-108: The cache cleanup test is using the global VideoMediaCache
clearAll() state to decide whether this specific directory should be deleted,
which makes the assertion dependent on other tests. Update the cleanup logic
around chatClient.clearCacheAndTemporaryFiles and
fileManager.clearVideoCache(context) so it checks ownership of the current
videoCache directory instead of any live cache instance, and ensure the test
always verifies deletion of the directory created in this test.
---
Nitpick comments:
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoCacheDataSourceFactory.kt`:
- Around line 40-61: The VideoCacheDataSourceFactory class can be simplified by
using interface delegation for DataSource.Factory instead of manually forwarding
createDataSource through a delegate property. Move cacheKeyFor out of the class
as a private top-level helper since it only depends on DataSpec, then delegate
DataSource.Factory directly in the class declaration and keep the cache setup
logic intact.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientCacheAndTemporaryFilesTest.kt`:
- Around line 56-61: The tearDown cleanup in
ChatClientCacheAndTemporaryFilesTest does not remove the video cache directory
because StreamFileManager.clearAllCache intentionally skips it, so add explicit
video cache cleanup alongside the existing clearAllCache and
clearExternalStorage calls. Update the test’s tearDown() to invoke the
appropriate StreamFileManager cleanup for the video cache path so it always gets
removed even if the in-test clearCacheAndTemporaryFiles assertion fails. Apply
the same defensive cleanup to the other referenced test block as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bbed4cb6-da81-49eb-a1d0-4ba2f86e3bd9
📒 Files selected for processing (21)
stream-chat-android-client-test/src/main/java/io/getstream/chat/android/client/test/MockedChatClientTest.ktstream-chat-android-client/api/stream-chat-android-client.apistream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/api/ChatClientConfig.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/StreamCacheConfig.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoCacheDataSourceFactory.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/cdn/internal/CDNDataSourceFactory.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/cdn/internal/StreamMediaDataSource.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/file/StreamFileManager.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientCacheAndTemporaryFilesTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/cache/VideoCacheConfigTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/cache/internal/StreamMediaDataSourceCacheIntegrationTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/cache/internal/VideoMediaCacheTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/cdn/internal/CDNDataSourceTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/file/StreamFileManagerTest.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/MediaGalleryPlayerState.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/StreamMediaPlayerContent.ktstream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/attachments/preview/internal/PrepareIfNeededTest.ktstream-chat-android-ui-components/src/main/kotlin/io/getstream/chat/android/ui/feature/gallery/AttachmentMediaActivity.ktstream-chat-android-ui-components/src/main/kotlin/io/getstream/chat/android/ui/feature/gallery/internal/AttachmentGalleryVideoPageFragment.kt
| @JvmStatic | ||
| public fun create(appContext: Context, cacheDir: File, config: VideoCacheConfig): VideoMediaCache = | ||
| synchronized(instances) { | ||
| cacheDir.mkdirs() | ||
| val key = cacheDir.absolutePath | ||
| instances[key]?.let { existing -> | ||
| logger.w { | ||
| "[create] Reusing existing VideoMediaCache for '$key'; " + | ||
| "additional VideoCacheConfig values are ignored." | ||
| } | ||
| return@synchronized existing | ||
| } | ||
| val dbProvider = StandaloneDatabaseProvider(appContext) | ||
| val simpleCache = SimpleCache( | ||
| cacheDir, | ||
| LeastRecentlyUsedCacheEvictor(config.maxSizeBytes), | ||
| dbProvider, | ||
| ) | ||
| VideoMediaCache(simpleCache, dbProvider, key).also { instances[key] = it } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Can androidx.media3 SimpleCache constructor throw an exception if the cache directory lock is stale or corrupted?
💡 Result:
Yes, the androidx.media3 SimpleCache constructor can throw an exception if the cache directory is already locked by another SimpleCache instance [1][2]. Specifically, if you attempt to instantiate SimpleCache for a directory that is already in use, the constructor will throw a java.lang.IllegalStateException with a message indicating that another SimpleCache instance is already using that folder [3]. This mechanism ensures that only one instance of SimpleCache manages a given directory at any time to prevent corruption [1][2]. Regarding stale or corrupted locks specifically: - If a previous SimpleCache instance was not properly released (e.g., due to an application crash or improper shutdown), the lock on the directory may persist, causing subsequent attempts to instantiate a new SimpleCache in that same directory to fail with the aforementioned IllegalStateException [3]. - The API provides a static method, SimpleCache.isCacheFolderLocked(File cacheFolder), which allows you to check if a directory is currently locked by a SimpleCache instance [1][2]. - To resolve issues where a folder remains locked, you must ensure that all previous SimpleCache instances associated with that directory are properly released by calling their release method [3][2]. Note that this behavior relates to the filesystem locking mechanism implemented by SimpleCache to maintain integrity; it is not typically described in terms of "corrupted" lock files, but rather as a persistent lock that requires the corresponding instance to be explicitly released [3][2].
Citations:
- 1: https://developer.android.com/reference/androidx/media3/datasource/cache/SimpleCache
- 2: https://developer.android.com/reference/kotlin/androidx/media3/datasource/cache/SimpleCache
- 3: https://stackoverflow.com/questions/52507270/java-lang-illegalstateexception-another-simplecache-instance-uses-the-folder
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '^(stream-chat-android-client/src/main/java/io/getstream/chat/android/client/(cache/internal/VideoMediaCache\.kt|ChatClient\.kt|.*VideoCacheConfig.*|.*CacheConfig.*))$'
echo "== locate create call sites =="
rg -n "VideoMediaCache\.create\(" stream-chat-android-client/src/main/java/io/getstream/chat/android/client -S
echo "== relevant outlines =="
ast-grep outline stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt --view expanded
echo "---"
ast-grep outline stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt --view expandedRepository: GetStream/stream-chat-android
Length of output: 8516
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== VideoMediaCache.create slice =="
sed -n '120,180p' stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt
echo
echo "== ChatClient internalBuild slice around video cache =="
sed -n '5260,5310p' stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt
echo
echo "== StreamCacheConfig slice =="
sed -n '1,220p' stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/StreamCacheConfig.ktRepository: GetStream/stream-chat-android
Length of output: 6310
Guard SimpleCache construction here
SimpleCache(...) can throw when the cache folder is locked or otherwise unusable, and this path is called directly from ChatClient.Builder.internalBuild(). If video caching is enabled, a stale cache dir can abort client initialization instead of just disabling cache. Catch the failure and fall back to no video cache so cache issues don't take down startup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt`
around lines 137 - 156, The VideoMediaCache.create(...) path currently
constructs SimpleCache directly, which can throw and abort
ChatClient.Builder.internalBuild() when the cache dir is locked or unusable.
Wrap the SimpleCache initialization in create(...) with failure handling, and on
error fall back to returning no video cache instead of propagating the
exception. Keep the guard localized to VideoMediaCache.create and preserve the
existing reuse behavior in the instances map when cache creation succeeds.
| // Clear video cache: in-place via any live cache in the process (keeps the SimpleCache | ||
| // alive so playback continues to work), or by deleting the directory when no live cache | ||
| // owns it. The registry is process-wide, so this covers caches from a prior ChatClient | ||
| // build even if the current client was built without a cacheConfig. | ||
| val videoCacheResult = if (VideoMediaCache.clearAll()) { | ||
| Result.Success(Unit) | ||
| } else { | ||
| fileManager.clearVideoCache(context) | ||
| } | ||
| // Clear all cache directories | ||
| val cacheResult = fileManager.clearAllCache(context) | ||
| // Clear external (temporary) storage files - always run regardless of cache result | ||
| val externalStorageResult = fileManager.clearExternalStorage(context) | ||
| // Return the first failure if any, otherwise success | ||
| when { | ||
| videoCacheResult is Result.Failure -> videoCacheResult |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant symbols before reading slices
echo "== ChatClient outline =="
ast-grep outline stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt --view expanded | sed -n '1,220p'
echo
echo "== VideoMediaCache search =="
rg -n "class VideoMediaCache|fun clearAll|fun create|instances|clearVideoCache|clearAllCache|clearExternalStorage" stream-chat-android-client/src/main/java -S
echo
echo "== FileManager outline if present =="
fd -a "FileManager.kt" stream-chat-android-client/src/main/java || trueRepository: GetStream/stream-chat-android
Length of output: 31263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ChatClient outline =="
ast-grep outline stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt --view expanded | sed -n '1,220p'
echo
echo "== VideoMediaCache search =="
rg -n "class VideoMediaCache|fun clearAll|fun create|instances|clearVideoCache|clearAllCache|clearExternalStorage" stream-chat-android-client/src/main/java -S
echo
echo "== FileManager files =="
fd -a "FileManager.kt" stream-chat-android-client/src/main/java || trueRepository: GetStream/stream-chat-android
Length of output: 31250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant slices with line numbers once the file locations are known.
sed -n '1500,1545p' stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt
echo
echo "== VideoMediaCache.kt slices =="
sed -n '1,260p' stream-chat-android-client/src/main/java/io/getstream/chat/android/client/helpers/VideoMediaCache.ktRepository: GetStream/stream-chat-android
Length of output: 3026
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the exact files and relevant symbols.
git ls-files 'stream-chat-android-client/src/main/java/**' | rg 'ChatClient\.kt|VideoMediaCache\.kt|FileManager\.kt'
echo
for f in \
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt \
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/helpers/VideoMediaCache.kt \
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/helpers/FileManager.kt
do
if [ -f "$f" ]; then
echo "== $f =="
wc -l "$f"
fi
doneRepository: GetStream/stream-chat-android
Length of output: 783
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the helper implementation and call sites.
rg -n "class VideoMediaCache|object VideoMediaCache|clearAll\(|create\(|deleteRecursively|clearVideoCache\(" stream-chat-android-client/src/main/java/io/getstream/chat/android/client -SRepository: GetStream/stream-chat-android
Length of output: 7459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant sections of the two helper files.
python3 - <<'PY'
from pathlib import Path
paths = [
Path("stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt"),
Path("stream-chat-android-client/src/main/java/io/getstream/chat/android/client/helpers/VideoMediaCache.kt"),
Path("stream-chat-android-client/src/main/java/io/getstream/chat/android/client/helpers/FileManager.kt"),
]
for p in paths:
if p.exists():
print(f"\n== {p} ==")
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if ("clearCacheAndTemporaryFiles" in line or
"clearAll(" in line or
"clearVideoCache" in line or
"deleteRecursively" in line or
"class VideoMediaCache" in line or
"object VideoMediaCache" in line or
"fun create(" in line):
start = max(1, i - 20)
end = min(len(lines), i + 40)
for j in range(start, end + 1):
print(f"{j:5d}: {lines[j-1]}")
print()
PYRepository: GetStream/stream-chat-android
Length of output: 14827
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt"
wc -l "$f"
echo
sed -n '1,240p' "$f"Repository: GetStream/stream-chat-android
Length of output: 7626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '360,390p' stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/file/StreamFileManager.ktRepository: GetStream/stream-chat-android
Length of output: 1522
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/file/StreamFileManager.kt")
lines = p.read_text().splitlines()
for start in [360]:
end = min(len(lines), start+35)
for i in range(start, end+1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: GetStream/stream-chat-android
Length of output: 1954
Close the video-cache clear/delete race.
VideoMediaCache.clearAll() only tells you whether a live cache existed at that moment; if it returns false, the fallback fileManager.clearVideoCache(context) in ChatClient.kt:1521-1524 can run after another thread has created a new VideoMediaCache for the same directory. That deleteRecursively() path can corrupt the live SimpleCache index/lock. Move the fallback delete behind a single atomic VideoMediaCache API that holds the registry lock across the “clear live caches or delete on disk” decision.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt`
around lines 1517 - 1532, The video cache cleanup in ChatClient should not
decide between clearing live caches and deleting the directory in two separate
steps. Update the cache removal flow around VideoMediaCache.clearAll() so the
“clear live caches or delete on disk” decision is handled atomically under the
registry lock, and remove the fallback delete from ChatClient’s cache-clearing
path. Use the existing VideoMediaCache and fileManager.clearVideoCache(context)
symbols to locate the affected logic and route the fallback through a single
synchronized API that prevents a newly created cache from being deleted
mid-flight.
andremion
left a comment
andremion
left a comment
There was a problem hiding this comment.
LGTM, design is solid. Two nits inline, both optional.
| * Wrap an instance in [StreamCacheConfig] and set it on | ||
| * [io.getstream.chat.android.client.api.ChatClientConfig.cacheConfig] to opt in. When the cache is | ||
| * enabled, replaying or seeking within a previously watched video reuses cached byte ranges | ||
| * instead of re-downloading from the CDN. |
There was a problem hiding this comment.
Small doc suggestion. The cache key strips the URL query, so entries are keyed by path alone, and this wraps all ExoPlayer media, not only Stream attachments. Worth one line here noting the cache assumes the path identifies the content? Sets expectations for anyone playing non-Stream URLs where the query is content-significant.
| // and falls back to alphabetical key order on ties, which would pick A over B under | ||
| // load and make the assertions non-deterministic. | ||
| readFully(factory.createDataSource(), DataSpec(Uri.parse(VIDEO_A_URL))) | ||
| Thread.sleep(SPAN_SPACING_MS) |
There was a problem hiding this comment.
This spaces writes with Thread.sleep(5L) because the evictor orders by System.currentTimeMillis(). Reasoning is clear, but it's wall-clock dependent so it could flake on a loaded runner. Just flagging in case it ever goes red.
2 participants
Goal
Resolves: AND-1275
Media attachments currently re-stream from the CDN on every replay, seek, or scroll-back — wasting customer bandwidth and forcing users to wait through buffering on content they have already watched. This PR introduces an opt-in on-disk media cache so previously watched videos (and other ExoPlayer-driven media, including audio) replay from local bytes.
Port of v6 PR #6533 (squash commit
eadde4d) todevelop. This was a manual port, not a clean cherry-pick, because:ChatClient.kthas diverged significantly betweenv6anddevelop.develop(the v6 change toMediaGalleryPreviewScreen.ktmaps toMediaGalleryPlayerState.kthere).Intentional API difference from the v6 PR: the cache is configured via a new
ChatClientConfig.cacheConfigproperty (mirroring the existingmessageLimitConfigpattern) rather than aChatClient.Builder.cacheConfig(...)builder method. The v6 PR's incidentalMediaPreviewActivity.getIntent@JvmOverloadschurn was dropped as unrelated to caching.Implementation
Public API
StreamCacheConfig(video: VideoCacheConfig? = null)— aggregate config; future cache types can be added as additional nullable properties without breaking the call site.VideoCacheConfig(maxSizeBytes: Long)— soft cap on cache size; defaults to 150 MB, LRU eviction once exceeded. Rejects non-positive sizes at construction.ChatClientConfig.cacheConfig: StreamCacheConfig— single entry point, off by default (existing apps see no behavior change).Internals
VideoMediaCacheowns a Media3SimpleCache+StandaloneDatabaseProvider. A process-wide registry keyed by the cache directory's absolute path prevents the "twoSimpleCacheinstances against the same directory" crash if a customer rebuildsChatClientin the same process.VideoCacheDataSourceFactorywraps Media3'sCacheDataSource.Factoryand composes outside any custom CDN — on a hit the bytes are served from disk and the customer'sCDN.fileRequestis not consulted; on a miss the CDN signs the URL just-in-time. The cache key strips URL query parameters, so rotating pre-signed signature/query values still resolve to the same cached entry.videoCache:StreamMediaPlayerContent(createPlayer)AttachmentMediaActivity,AttachmentGalleryVideoPageFragmentStreamAudioPlayerpipeline built inChatClient.Builder.build()clearCacheAndTemporaryFiles(context)clears every live cache in the process viaVideoMediaCache.clearAll()(which empties theSimpleCachewhile keeping it alive and its directory lock held), and only falls back to deleting the cache directory when no live cache owns it.Player.prepareIfNeeded(...)skips re-preparing an already-loaded URL, avoiding a second concurrent load that could leave a cached file's head uncached.UI Changes
No UI changes.
Testing
Automated — ran locally, all green:
StreamMediaDataSourceCacheIntegrationTest—{custom CDN, no CDN} × {hit, miss}matrix, rotated-query cache hit, LRU eviction, in-placeclear, andclearAll.VideoMediaCacheTest— directory creation,release()idempotency, per-directory instance registry.VideoCacheConfigTest— default/positive/zero/negativemaxSizeBytesvalidation.StreamFileManagerTest/ChatClientCacheAndTemporaryFilesTest— cache directory lookup and cleanup wiring.PrepareIfNeededTest(compose) — redundant-prepare skipping.Manual smoke
ChatClientConfig(cacheConfig = StreamCacheConfig(video = VideoCacheConfig())).<cacheDir>/stream_video_cache/contains files.clearCacheAndTemporaryFiles(context); cached bytes are removed and playback re-fetches, with the cache staying functional afterwards.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes