Skip to content

fix: frame skip due to native viewer rebuilds #26539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shenlong-tanwen wants to merge 2 commits into from
Closed

fix: frame skip due to native viewer rebuilds #26539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
39ef77a
fix: frame skip due to native viewer rebuilds
shenlong-tanwen Feb 26, 2026
2d1a47b
fix lint
shenlong-tanwen Feb 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions mobile/lib/presentation/widgets/asset_viewer/asset_page.widget.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ import 'package:immich_mobile/presentation/widgets/images/thumbnail.widget.dart'
import 'package:immich_mobile/providers/app_settings.provider.dart';
import 'package:immich_mobile/providers/asset_viewer/is_motion_video_playing.provider.dart';
import 'package:immich_mobile/providers/asset_viewer/video_player_controls_provider.dart';
import 'package:immich_mobile/services/app_settings.service.dart';
import 'package:immich_mobile/providers/infrastructure/asset.provider.dart';
import 'package:immich_mobile/providers/infrastructure/asset_viewer/asset.provider.dart';
import 'package:immich_mobile/providers/infrastructure/timeline.provider.dart';
import 'package:immich_mobile/services/app_settings.service.dart';
import 'package:immich_mobile/widgets/common/immich_loading_indicator.dart';
import 'package:immich_mobile/widgets/photo_view/photo_view.dart';

Expand All @@ -31,9 +31,16 @@ enum _DragIntent { none, scroll, dismiss }
class AssetPage extends ConsumerStatefulWidget {
final int index;
final int heroOffset;
final Map<String, GlobalKey> videoPlayerKeys;
final void Function(int direction)? onTapNavigate;

const AssetPage({super.key, required this.index, required this.heroOffset, this.onTapNavigate});
const AssetPage({
super.key,
required this.index,
required this.heroOffset,
required this.videoPlayerKeys,
this.onTapNavigate,
});

@override
ConsumerState createState() => _AssetPageState();
Expand Down Expand Up @@ -293,6 +300,11 @@ class _AssetPageState extends ConsumerState<AssetPage> {
_listenForScaleBoundaries(controller);
}

GlobalKey _getVideoPlayerKey(String id) {
Copy link
Member

@mertalev mertalev Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s add a comment about why this is necessary.

widget.videoPlayerKeys.putIfAbsent(id, () => GlobalKey());
return widget.videoPlayerKeys[id]!;
}

Widget _buildPhotoView(
BaseAsset displayAsset,
BaseAsset asset, {
Expand Down Expand Up @@ -350,7 +362,7 @@ class _AssetPageState extends ConsumerState<AssetPage> {
enablePanAlways: true,
backgroundDecoration: backgroundDecoration,
child: NativeVideoViewer(
key: ValueKey(displayAsset),
key: _getVideoPlayerKey(displayAsset.heroTag),
Copy link
Collaborator

@uhthomas uhthomas Feb 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also I wonder if this is just my mistake. Can we try with just:

Suggested change
key: _getVideoPlayerKey(displayAsset.heroTag),
key: Key(displayAsset.heroTag),

Copy link
Member

@mertalev mertalev Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent a lot of time on this and landed on global keys to make the behavior correct. We can test how alternatives behave in terms of rebuilding etc., but I feel this is already a solved problem.

Copy link
Collaborator

@uhthomas uhthomas Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be keen to try plain keys with a stable id (hero tag in this case). It should work the same?

Copy link
Collaborator

@uhthomas uhthomas Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's also a lot less code and a lot less confusing

Copy link
Member

@mertalev mertalev Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not the same because the hero animation tries to snapshot the destination, which creates two separate widget states for the video. A global key was the only way I found to avoid that.

Copy link
Collaborator

@uhthomas uhthomas Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds weird - let's try with the Key on hero tag and see if it fixes it. All the research I've done into global keys suggests that it doesn't really make sense here.

Copy link
Collaborator

@uhthomas uhthomas Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think I have a better understanding now. So although a ValueKey does help when siblings are moved around, it doesn't help if the parent is recreated. It may create the exact same tree again, but the widget with those keys are disregarded. A global key will persist it, even if the parent is rebuilt. I see why a global key is needed, though I do think we should just keep the global key on the state of the asset page rather than having a map. I imagine the map was needed when both the asset page and asset viewer were one big widget. Now that it's its own separate stateful widget, it's not needed.

Copy link
Collaborator

@uhthomas uhthomas Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the video is the right level for it though. Surely painting images can be kind of expensive too. Should the global key be on the photo view widget itself?

Copy link
Member

@mertalev mertalev Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of the image provider / image cache system, it's not as bad for images since the actual image loading "should" persist and this is the expensive part (painting an already loaded bitmap is relatively cheap). But it can be problematic if it results in the image request being cancelled and then re-requested. I haven't really looked into it though tbh, could be fine to use global keys for either and not just for videos.

Copy link
Collaborator

@uhthomas uhthomas Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I am concerned that this won't work properly for asset stacks, with videos.

Is there any reason we can't just use GlobalObjectKey(displayAsset.heroTag)? That means we don't need to store the global key anywhere, and it will work properly with asset stacks.

asset: displayAsset,
scaleStateNotifier: _videoScaleStateNotifier,
disableScaleGestures: showingDetails,
Expand Down
12 changes: 9 additions & 3 deletions mobile/lib/presentation/widgets/asset_viewer/asset_viewer.page.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ import 'package:immich_mobile/presentation/widgets/asset_viewer/asset_page.widge
import 'package:immich_mobile/presentation/widgets/asset_viewer/asset_preloader.dart';
import 'package:immich_mobile/presentation/widgets/asset_viewer/asset_stack.provider.dart';
import 'package:immich_mobile/presentation/widgets/asset_viewer/asset_viewer.state.dart';
import 'package:immich_mobile/presentation/widgets/asset_viewer/viewer_top_app_bar.widget.dart';
import 'package:immich_mobile/presentation/widgets/asset_viewer/viewer_bottom_app_bar.widget.dart';
import 'package:immich_mobile/presentation/widgets/asset_viewer/viewer_top_app_bar.widget.dart';
import 'package:immich_mobile/providers/asset_viewer/video_player_controls_provider.dart';
import 'package:immich_mobile/providers/asset_viewer/video_player_value_provider.dart';
import 'package:immich_mobile/providers/cast.provider.dart';
Expand Down Expand Up @@ -90,6 +90,7 @@ class _AssetViewerState extends ConsumerState<AssetViewer> {
late final _heroOffset = widget.heroOffset ?? TabsRouterScope.of(context)?.controller.activeIndex ?? 0;
late final _pageController = PageController(initialPage: widget.initialIndex);
late final _preloader = AssetPreloader(timelineService: ref.read(timelineServiceProvider), mounted: () => mounted);
final Map<String, GlobalKey> _videoPlayerKeys = {};
Copy link
Collaborator

@uhthomas uhthomas Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to keep track of video player keys for all asset pages? Can't each page have its own global key? That should be enough to prevent the video from being rebuilt when it matters?

Copy link
Member

@mertalev mertalev Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should never be two video widgets for the same asset.

Copy link
Collaborator

@uhthomas uhthomas Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe what I said wasn't clear - this map will grow unbounded. So if a user swipes between lots of videos, we'll end up with lots of global keys for no real reason.

Copy link
Collaborator

@uhthomas uhthomas Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and my suggestion was that we could have a field on the asset page state like

GlobalKey _videoKey;

Copy link
Member

@mertalev mertalev Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that seems like it can work.


StreamSubscription? _reloadSubscription;
KeepAliveLink? _stackChildrenKeepAlive;
Expand Down Expand Up @@ -124,6 +125,7 @@ class _AssetViewerState extends ConsumerState<AssetViewer> {

@override
void dispose() {
_videoPlayerKeys.clear();
_pageController.dispose();
_preloader.dispose();
_reloadSubscription?.cancel();
Expand Down Expand Up @@ -287,8 +289,12 @@ class _AssetViewerState extends ConsumerState<AssetViewer> {
: const FastClampingScrollPhysics(),
itemCount: ref.read(timelineServiceProvider).totalAssets,
onPageChanged: (index) => _onAssetChanged(index),
itemBuilder: (context, index) =>
AssetPage(index: index, heroOffset: _heroOffset, onTapNavigate: _onTapNavigate),
itemBuilder: (context, index) => AssetPage(
index: index,
heroOffset: _heroOffset,
videoPlayerKeys: _videoPlayerKeys,
onTapNavigate: _onTapNavigate,
),
),
),
if (!CurrentPlatform.isIOS)
Expand Down
Loading