Fall back to a video frame when the server thumbnail is not ready#6524
Open
andremion wants to merge 6 commits into
Open
Fall back to a video frame when the server thumbnail is not ready#6524andremion wants to merge 6 commits into
andremion wants to merge 6 commits into
Conversation
A sent video showed a blank preview in the message list and the shared-media grids until the chat was reopened. The upload-intent response returns a thumbnail URL before server-side thumbnail generation has finished, so the client requests it and gets a 404, leaving the preview blank. The existing `?: upload` fallback never helped because the thumbnail URL is present; only the load fails. Add a `VideoThumbnailFallbackInterceptor` in ui-common that loads the server thumbnail first and, when it is missing or fails to load, extracts a frame from the video asset itself via Coil's `VideoFrameDecoder`. Both UI kits now pass `VideoThumbnailImageData(thumbUrl, assetUrl)` for videos. The interceptor rewrites the request to a plain URL before proceeding, so CDN signing, network fetch, disk cache, and decoding are reused for both the thumbnail and the fallback frame. It is registered as the outermost interceptor so the CDN interceptor still signs the fallback video URL. This matches the iOS behaviour in StreamMediaLoader.
loadAttachmentThumb is used by the XML file rows (FileAttachmentsView) and quoted previews (DefaultQuotedAttachmentView). It loaded only the server thumbnail, so a video showed a blank thumbnail when the thumbnail was not yet generated. The Compose equivalents already recover through imagePreviewData. Pass VideoThumbnailImageData here too so both kits behave the same.
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
Contributor
SDK Size Comparison 📏
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThis PR adds video thumbnail fallback loading for attachments. It introduces a thumbnail-first video preview payload, a Coil interceptor and frame fetcher, registers them in the image loader, updates attachment preview call sites to supply the new payload, and adds tests. ChangesVideo thumbnail fallback flow
Sequence Diagram(s)sequenceDiagram
participant StreamImageLoaderFactory
participant VideoThumbnailFallbackInterceptor
participant VideoFrameFetcher
participant MediaMetadataRetriever
StreamImageLoaderFactory->>VideoThumbnailFallbackInterceptor: register interceptor
StreamImageLoaderFactory->>VideoFrameFetcher: register factory
VideoThumbnailFallbackInterceptor->>VideoThumbnailFallbackInterceptor: load thumbnailUrl first
VideoThumbnailFallbackInterceptor->>VideoFrameFetcher: retry with videoUrl and videoFramePreview()
VideoFrameFetcher->>MediaMetadataRetriever: setDataSource(url, headers)
MediaMetadataRetriever-->>VideoFrameFetcher: preview frame bitmap
VideoFrameFetcher-->>VideoThumbnailFallbackInterceptor: ImageFetchResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🧹 Nitpick comments (2)
stream-chat-android-ui-common/src/test/kotlin/io/getstream/chat/android/ui/common/images/internal/VideoThumbnailFallbackInterceptorTest.kt (1)
57-66: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAssert the fallback request is marked as a video-frame preview.
This test only proves the URL rewrite. If
VideoThumbnailFallbackInterceptorstops callingvideoFramePreview(), it would still pass while breaking theVideoFrameFetcher.Factoryhandoff and potentially sending the fallback through the default video fetch path. Capture the rewrittenImageRequestinFakeCoilChainand assertvideoFramePreviewKeyis set on the video fallback request.Also applies to: 107-129
🤖 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-ui-common/src/test/kotlin/io/getstream/chat/android/ui/common/images/internal/VideoThumbnailFallbackInterceptorTest.kt` around lines 57 - 66, The fallback test in VideoThumbnailFallbackInterceptorTest only verifies the URL switch, so it can miss regressions where the video fallback is no longer marked as a video-frame preview. Update the FakeCoilChain used by VideoThumbnailFallbackInterceptorTest to capture the rewritten ImageRequest for the VIDEO_URL request, then assert that the fallback request has videoFramePreviewKey set, alongside the existing URL/proceed assertions.stream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/images/internal/VideoFrameFetcher.kt (1)
101-109: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSet
isSampled = truewhen returning a scaled frame.When
getScaledFrameAtTimedownsizes to the requested dimensions the returned bitmap is effectively sampled, butfetch()reportsisSampled = false. This can mislead Coil's memory-cache/exact-size bookkeeping for the scaled branch.🤖 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-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/images/internal/VideoFrameFetcher.kt` around lines 101 - 109, The scaled-frame branch in MediaMetadataRetriever.extractFrame() currently returns a downsized bitmap but fetch() still treats it as not sampled. Update the VideoFrameFetcher.fetch flow so that when getScaledFrameAtTime is used for the requested size, the resulting ImageSource/result is marked with isSampled = true, while keeping the unscaled getFrameAtTime path unchanged.
🤖 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-compose/src/main/java/io/getstream/chat/android/compose/ui/util/extensions/internal/AttachmentExtensions.kt`:
- Around line 45-53: The video thumbnail branch in AttachmentExtensionsKt is
forcing failures by replacing the real thumbUrl with an invalid hardcoded URL,
so remove the TEMP TEST override and pass through the actual thumbnail value.
Update the isVideo() && ChatTheme.videoThumbnailsEnabled path to use the same
CDN-resizing transform as other call sites in this Compose scope (for example
via ChatTheme.streamCdnImageResizing or the equivalent accessor) before
constructing VideoThumbnailImageData, while still falling back to upload only
when both thumbnail and asset URLs are absent.
---
Nitpick comments:
In
`@stream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/images/internal/VideoFrameFetcher.kt`:
- Around line 101-109: The scaled-frame branch in
MediaMetadataRetriever.extractFrame() currently returns a downsized bitmap but
fetch() still treats it as not sampled. Update the VideoFrameFetcher.fetch flow
so that when getScaledFrameAtTime is used for the requested size, the resulting
ImageSource/result is marked with isSampled = true, while keeping the unscaled
getFrameAtTime path unchanged.
In
`@stream-chat-android-ui-common/src/test/kotlin/io/getstream/chat/android/ui/common/images/internal/VideoThumbnailFallbackInterceptorTest.kt`:
- Around line 57-66: The fallback test in VideoThumbnailFallbackInterceptorTest
only verifies the URL switch, so it can miss regressions where the video
fallback is no longer marked as a video-frame preview. Update the FakeCoilChain
used by VideoThumbnailFallbackInterceptorTest to capture the rewritten
ImageRequest for the VIDEO_URL request, then assert that the fallback request
has videoFramePreviewKey set, alongside the existing URL/proceed assertions.
🪄 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: 44d6975d-31ca-4697-ab02-afea89397054
📒 Files selected for processing (10)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/channel/attachments/ChannelMediaAttachmentsScreen.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/util/extensions/internal/AttachmentExtensions.ktstream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/images/StreamImageLoaderFactory.ktstream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/images/internal/VideoFrameFetcher.ktstream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/images/internal/VideoThumbnailFallbackInterceptor.ktstream-chat-android-ui-common/src/test/kotlin/io/getstream/chat/android/ui/common/images/internal/VideoFrameFetcherTest.ktstream-chat-android-ui-common/src/test/kotlin/io/getstream/chat/android/ui/common/images/internal/VideoThumbnailFallbackInterceptorTest.ktstream-chat-android-ui-components/src/main/kotlin/io/getstream/chat/android/ui/feature/gallery/overview/internal/MediaAttachmentAdapter.ktstream-chat-android-ui-components/src/main/kotlin/io/getstream/chat/android/ui/feature/messages/list/adapter/view/internal/MediaAttachmentView.ktstream-chat-android-ui-components/src/main/kotlin/io/getstream/chat/android/ui/utils/AttachmentUtils.kt
The fallback previously routed the video URL through VideoFrameDecoder, which needs the whole file downloaded first and writes it to the image disk cache. Replace it with a VideoFrameFetcher backed by MediaMetadataRetriever, which seeks with HTTP range requests and reads only the bytes needed for the frame, and never stores the full video. This matches the cost profile of iOS, which uses AVAssetImageGenerator on a remote asset. The interceptor marks the fallback request and still runs before the CDN interceptor, so the CDN-signed URL and headers reach the fetcher. Requests that are not marked fall through to the default network fetcher.
andremion
force-pushed
the
andrerego/and-1267-video-preview-shows-blank-until-reload-when-server-thumbnail
branch
from
8de58e7 to
bc99d1b
Compare
Contributor
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
…table - VideoThumbnailFallbackInterceptor is only used inside ui-common, so it is internal rather than @InternalStreamChatApi public. - The MediaAttachmentQuotedContent preview handler matched the raw "video" string, but a video preview's data is now VideoThumbnailImageData. Match that type so the video cells keep their preview color and the snapshot is unchanged.
Add VideoFrameFetcher.fetch() tests using Robolectric's MediaMetadataRetriever shadow for the scaled, unscaled, and no-frame paths, and a VideoThumbnailFallbackInterceptor test for the case with neither a thumbnail nor a video URL.
Add a Compose test that captures the value imagePreviewData returns for image, video (with thumbnail, asset-only, and upload fallback), thumbnails-disabled, and non-media attachments.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goal
A sent video shows a blank preview in the message list (and in the shared-media grids and quoted previews) until the chat is reopened. Photos work fine.
The cause is a race with server-side thumbnail generation. The upload-intent response returns a
thumbUrlbefore the server has finished generating the thumbnail. The client requests that URL right away and gets a 404, so the preview is blank. A few seconds later the thumbnail is ready, and the next time the item is rendered (scroll away and back, or reopen) it loads. The existing?: uploadfallback does not help, becausethumbUrlis present; only the load fails.iOS already handles this: when the thumbnail load fails, it extracts a frame from the video itself. This change brings Android to the same behavior.
Resolves AND-1267
Implementation
Core, in
stream-chat-android-ui-common:VideoThumbnailImageData(thumbnailUrl, videoUrl)is a small request model used as the Coil data for a video preview.VideoThumbnailFallbackInterceptorloads the server thumbnail first and, when it is missing or fails, falls back to a frame from the video. It rewrites the request to a plain URL before proceeding, so CDN signing, headers, and caching are reused. It is registered as the outermost interceptor, so the CDN interceptor still signs the fallback video URL.VideoFrameFetcherextracts the fallback frame withMediaMetadataRetriever, which seeks with HTTP range requests and reads only the bytes needed for the frame. The full video is never written to the image disk cache. This matches the cost profile of iOS (AVAssetImageGeneratoron a remote asset). Local files keep using Coil'sVideoFrameDecoder.Call sites now pass
VideoThumbnailImageData(thumbUrl, assetUrl)for videos, on both kits and on the same surfaces iOS covers:MediaAttachmentContent(Compose),MediaAttachmentView(XML).ChannelMediaAttachmentsScreen(Compose),MediaAttachmentAdapter(XML).imagePreviewData(Compose, shared by quoted and file content),loadAttachmentThumb(XML).The new types are
@InternalStreamChatApi, so there is no public API change.🎨 UI Changes
Message list with a video whose thumbnail is not yet available.
Testing
Manual steps (Compose sample):
Patch to force the thumbnail to fail
Verified on the emulator: the frame renders through the fallback, and the full video is not written to the image disk cache (after clearing the cache and reloading, the largest cache entry stays in the hundreds of KB).
Summary by CodeRabbit
New Features
Bug Fixes
Tests