fix: frame skip due to native viewer rebuilds#26539
fix: frame skip due to native viewer rebuilds#26539shenlong-tanwen wants to merge 2 commits intomainfrom
Conversation
| 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); | ||
| Map<String, GlobalKey> _videoPlayerKeys = {}; |
There was a problem hiding this comment.
Not the prettiest way to do it, but the state has to be above the asset viewer page and since it is tied to the lifecycle of the asset viewer page, added this here for now
| 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 = {}; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
There should never be two video widgets for the same asset.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
and my suggestion was that we could have a field on the asset page state like
GlobalKey _videoKey;There was a problem hiding this comment.
Yeah, that seems like it can work.
| _listenForScaleBoundaries(controller); | ||
| } | ||
|
|
||
| GlobalKey _getVideoPlayerKey(String id) { |
There was a problem hiding this comment.
Let’s add a comment about why this is necessary.
| backgroundDecoration: backgroundDecoration, | ||
| child: NativeVideoViewer( | ||
| key: ValueKey(displayAsset), | ||
| key: _getVideoPlayerKey(displayAsset.heroTag), |
There was a problem hiding this comment.
also I wonder if this is just my mistake. Can we try with just:
| key: _getVideoPlayerKey(displayAsset.heroTag), | |
| key: Key(displayAsset.heroTag), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'd be keen to try plain keys with a stable id (hero tag in this case). It should work the same?
There was a problem hiding this comment.
it's also a lot less code and a lot less confusing
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
also fixes #26448 |
| } | ||
|
|
||
| return PhotoView.customChild( | ||
| key: ValueKey(displayAsset), |
There was a problem hiding this comment.
also, we need to fix the key here. Maybe we should remove the key from the native video player and just have it here.
|
Superseded by #26553 |
3 participants
No description provided.