Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Conversation

@cbenhagen
Copy link
Contributor

@cbenhagen cbenhagen commented Dec 11, 2019

Description

Explicitly mark texture frame available on seekTo. This is necessairy since flutter/engine@6a90b7f has changed the external texture data flow from a producer-consumer model. Now ios_external_texture_gl caches previous opengl texture, if no new frame are available, it does not execute the copyPixelBuffer method, just uses cached opengl texture to draw.

Related Issues

Fixes flutter/flutter#43567

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@cbenhagen
Copy link
Contributor Author

@ened PTAL

@cbenhagen
Copy link
Contributor Author

@cyanglaz do you have time for a quick review? This is a very simple change that would fix a bug present since the last flutter stable release.

@amirh
Copy link
Contributor

amirh commented Jan 10, 2020

Can you please add more details on the nature of the change and the reasoning behind it to the PR description?

@cbenhagen
Copy link
Contributor Author

Done. Hopefully it does make more sense now.

@amirh
Copy link
Contributor

amirh commented Jan 10, 2020

I'm not sure I fully understand flutter/engine@6a90b7f does it mean we need to call textureFrameAvailable every time there's a new frame? if so who's calling it for each video frame?

cc @cbracken who landed flutter/engine#11524 and probably better understands this.

@cbenhagen
Copy link
Contributor Author

CADisplayLink here:

@implementation FLTFrameUpdater
- (FLTFrameUpdater*)initWithRegistry:(NSObject<FlutterTextureRegistry>*)registry {
NSAssert(self, @"super init cannot be nil");
if (self == nil) return nil;
_registry = registry;
return self;
}
- (void)onDisplayLink:(CADisplayLink*)link {
[_registry textureFrameAvailable:_textureId];
}
@end

@amirh
Copy link
Contributor

amirh commented Jan 10, 2020

I see, thanks.

@amirh
Copy link
Contributor

amirh commented Jan 10, 2020

I don't see an obvious way to test this, @collinjackson do you have any suggestion?

@cbenhagen
Copy link
Contributor Author

@amirh can we release this without a test? I mean I'd really love to have one but I really don't know how as the simulator would not even show an image.

@amirh
Copy link
Contributor

amirh commented Jan 16, 2020

We need some test, how about an XCTest that mocks the texture registry, invokes handleMethodCall with a "seekTo" and asserts that textureFrameAvailable was invoked?

(we'll have to mock a few other objects to be able to deliver a mock texture registry, I may be missing something but it seems reasonably doable)

@cyanglaz
Copy link
Contributor

cyanglaz commented Jan 16, 2020

@cbenhagen I recently set up a similar test (#2445) mentioned by @amirh above in the webview plugin. You can use it as a reference.

@wterrill
Copy link

wterrill commented Feb 2, 2020

I'm seeing that the ios version of video_player does not perform an update (and therefore a triggered build) on "seekTo()" when the video is paused. (but does update when playing)

Android works for both paused videos and running videos.

Is that bug caused by this issue?

@jpiabrantes
Copy link

I really need this merged. The newest version of video_player still doesn't fix this issue.

@dthuering
Copy link

Hi @jpiabrantes I think some changes in the repo make this PR breaking. The FLTVideoPlayerPlugin.m file changed a lot.
:(

@cbenhagen
Copy link
Contributor Author

Closing in favor of #2758

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[video_player] [iOS] texture not updated while scrubbing

8 participants