[video_player] Added setMaxVideoResolution to disambiguate over different video stream resolutions by AngeloAvv · Pull Request #6230 · flutter/plugins

AngeloAvv · 2022-08-11T20:47:57Z

Since HLS streams can deliver multiple tracks with a single URL, it could be particularly useful to set a maximum video resolution to spare network bandwidth instead of picking the best stream that could also be calculated according to the device network speed.

The main issue:

[video_player] Select different video stream resolutions flutter#109414

Related issues:

[video_player]: Support mutable dataSource (for different resolutions) or provide copyWith flutter#84170

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Fixed analysis for master channel Fixed unit tests due do mismatching import

stuartmorgan-g

Thanks for the contribution!

It looks like this only has tests that the new parameters are passed all the way through to the native code, but no tests that the native code actually does the correct things with them. We'll need test coverage of that in order to proceed with this PR, since that's the core of the functionality.

packages/video_player/video_player/lib/video_player.dart

stuartmorgan-g · 2022-08-16T15:51:30Z

packages/video_player/video_player/lib/video_player.dart

    this.playbackSpeed = 1.0,
    this.rotationCorrection = 0,
    this.errorDescription,
+    this.maxVideoResolution,


Do we actually need this to be part of the state stream?

If I remove it, we should only rely on setMaxVideoResolution method to set the value. Plus, we won't be able to access to this information since we will need to remove it from the toString() method.

WDYT?

packages/video_player/video_player/pubspec.yaml

packages/video_player/video_player_web/pubspec.yaml

packages/video_player/video_player_android/android/gradle/wrapper/gradle-wrapper.properties

# Conflicts: # packages/video_player/video_player_android/CHANGELOG.md # packages/video_player/video_player_android/pubspec.yaml

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m

# Conflicts: # packages/video_player/video_player/CHANGELOG.md # packages/video_player/video_player_android/CHANGELOG.md # packages/video_player/video_player_avfoundation/CHANGELOG.md # packages/video_player/video_player_platform_interface/CHANGELOG.md

AngeloAvv · 2022-08-18T15:11:51Z

Hey @stuartmorgan, I added tests on both sides: Android and iOS native! Let me know if it's ok

# Conflicts: # packages/video_player/video_player/CHANGELOG.md # packages/video_player/video_player_android/CHANGELOG.md # packages/video_player/video_player_avfoundation/CHANGELOG.md # packages/video_player/video_player_platform_interface/CHANGELOG.md # packages/video_player/video_player_platform_interface/lib/messages.g.dart # packages/video_player/video_player_platform_interface/lib/method_channel_video_player.dart # packages/video_player/video_player_platform_interface/lib/video_player_platform_interface.dart # packages/video_player/video_player_platform_interface/pigeons/messages.dart # packages/video_player/video_player_platform_interface/test/method_channel_video_player_test.dart # packages/video_player/video_player_platform_interface/test/test.dart

AngeloAvv · 2022-09-04T15:32:37Z

Hey @stuartmorgan , do we have an ETA for this PR?

I need to understand if this feature will ever land in production.

jmagman · 2022-09-07T20:53:18Z

cc @hellohuanlin

packages/video_player/video_player_avfoundation/CHANGELOG.md

hellohuanlin

The iOS part looks good. But android folks (maybe @camsim99?) may want to take a look as well.

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m

hellohuanlin · 2022-09-07T21:00:49Z

packages/video_player/video_player_avfoundation/ios/Classes/messages.g.h

+- (instancetype)init NS_UNAVAILABLE;
+ (instancetype)makeWithTextureId:(NSNumber *)textureId
+                            width:(NSNumber *)width
+                           height:(NSNumber *)height;


it's recommended to do alloc/init explicitly instead of combining them.

This is Pigeon-generated code; I filed a bug a while ago about it being non-idiomatic.

camsim99

Left some comments on the Android side :)

packages/video_player/video_player_android/CHANGELOG.md

camsim99 · 2022-09-08T20:21:12Z

packages/video_player/video_player_android/example/pubspec.yaml

    path: ../
-  video_player_platform_interface: ">=4.2.0 <6.0.0"
+  video_player_platform_interface:
+    path: ../../video_player_platform_interface


Was this just for testing?

camsim99 · 2022-09-08T20:22:37Z

packages/video_player/video_player_android/pubspec.yaml

    sdk: flutter
-  video_player_platform_interface: ^5.1.1
+  video_player_platform_interface:
+    path: ../video_player_platform_interface


Just for testing?

…deo_player_resolution # Conflicts: # packages/video_player/video_player_android/CHANGELOG.md

# Conflicts: # packages/webview_flutter/webview_flutter_android/test/webview_android_widget_test.mocks.dart

GaryQian · 2022-10-25T17:18:42Z

@stuartmorgan Could you take a look at this to see if this API is reasonable?

camsim99

The Android part looks good to me! @stuartmorgan @bparrishMines can one of you take a look at the overall structure?

bparrishMines · 2022-11-02T16:38:16Z

packages/video_player/video_player_platform_interface/lib/video_player_platform_interface.dart

+  /// Sets the max video resolution to a [resolution] value allowing the player
+  /// to reproduce lwoer resolution streams.


nit: I think the wording on this could be improved. Something like

Suggested change

/// Sets the max video resolution to a [resolution] value allowing the player

/// to reproduce lwoer resolution streams.

/// Sets the maximum allowed video resolution to [resolution], allowing the

/// player to produce streams below this resolution.

bparrishMines · 2022-11-02T16:42:44Z

packages/video_player/video_player_platform_interface/lib/video_player_platform_interface.dart

+/// The video resolution, the number of distinct pixels in each dimension that
+/// can be displayed


I think this can be improved as well:

Suggested change

/// The video resolution, the number of distinct pixels in each dimension that

/// can be displayed

/// The number of distinct pixels in each dimension that can be displayed for

/// a video.

bparrishMines · 2022-11-02T16:43:11Z

packages/video_player/video_player_platform_interface/pubspec.yaml

 # NOTE: We strongly prefer non-breaking changes, even at the expense of a
 # less-clean API. See https://flutter.dev/go/platform-interface-breaking-changes
-version: 6.0.0
+version: 6.0.1


This should be bumped to 6.1.0 since it is adding a new feature.

bparrishMines · 2022-11-02T16:46:10Z

packages/video_player/video_player/lib/video_player.dart

+    // Setting the playback speed on iOS will trigger the video to play. We
+    // prevent this from happening by not applying the playback speed until
+    // the video is manually played from Flutter.
+    if (!value.isPlaying) {


Is this check necessary for this method? It looks like it was copied from _applyPlaybackSpeed().

bparrishMines · 2022-11-02T16:49:56Z

packages/video_player/video_player/lib/video_player.dart

+      return;
+    }
+
+    // If we haven't set a max video resolution we won't do anything


Why is setting a max video resolution ignored if one is not already set? The public method, setMaxVideoResolution, should also contain this information.

bparrishMines · 2022-11-02T16:50:11Z

packages/video_player/video_player/lib/video_player.dart

+  /// Sets the max video resolution of [this].
+  ///
+  /// The [resolution] will be used to set a maximum resolution value which will
+  /// allow the player to pick a different stream to play


nit:

Suggested change

/// allow the player to pick a different stream to play

/// allow the player to pick a different stream to play.

bparrishMines · 2022-11-02T16:51:16Z

packages/video_player/video_player/pubspec.yaml

 repository: https://github.com/flutter/plugins/tree/main/packages/video_player/video_player
 issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+video_player%22
-version: 2.4.7
+version: 2.4.8


This should be a major version bump since it is adding a feature. (e.g. 2.5.0). Although the exact value may change by the time the PR is ready to land.

ditman

(Please, do revert changes to files that are not relevant to the PR; thanks!)

ditman · 2022-11-03T02:25:06Z

packages/image_picker/image_picker/test/image_picker_test.mocks.dart

-class _FakeLostDataResponse_1 extends _i1.Fake
-    implements _i2.LostDataResponse {}
+class _FakeLostDataResponse_1 extends _i1.Fake implements _i2.LostDataResponse {
+}


Please, revert this file, this pr has nothing to do with the image_picker package.

ditman · 2022-11-03T02:25:26Z

packages/webview_flutter/webview_flutter_android/test/android_webview_test.mocks.dart

-class _FakeDownloadListener_0 extends _i1.Fake
-    implements _i2.DownloadListener {}
+class _FakeDownloadListener_0 extends _i1.Fake implements _i2.DownloadListener {
+}


Please, revert this file, this pr has nothing to do with the webview_flutter package.

stuartmorgan-g · 2023-01-04T01:30:20Z

@AngeloAvv Are you still planning on updating this based on the comments above?

reidbaker · 2023-02-02T19:31:47Z

@AngeloAvv Are you still planning on updating this based on the comments above?

AngeloAvv · 2023-02-10T11:05:17Z

@stuartmorgan @reidbaker sure! I just need to find a spot in the next weeks to address the changes.

stuartmorgan-g · 2023-02-22T15:56:43Z

We've just completed the migration of the plugin code to the flutter/packages repository, as described in https://flutter.dev/go/flutter-plugins-repo-migration, and this repository is now being archived. Unfortunately that means that all in-progress PRs here must be moved to flutter/packages.

Please see our instructions for an explanation of how to move your PR, and if you have any issues moving your PR please don't hesitate to reach out in the #hackers-ecosystem channel in Discord.

Our apologies that your PR was caught in this one-time transition. We're aware that it's disruptive in the short term, and appreciate your help in getting us to a better long-term state!

AngeloAvv and others added 5 commits August 11, 2022 19:24

Added setMaxVideoResolution message to change video quality over streams

ba98a9f

Regenerated pigeons support classes

5216942

Fixed typos in FLTVideoPlayerPlugin.m

f75d749

Fixed tests, reformatted files and updated changelogs and authors

278f037

Merge branch 'flutter:main' into video_player_resolution

3c59c31

AngeloAvv requested review from camsim99, ditman, gaaclarke and hellohuanlin as code owners August 11, 2022 20:47

github-actions bot added p: video_player platform-android platform-ios platform-web labels Aug 11, 2022

AngeloAvv and others added 3 commits August 11, 2022 23:54

Fixed some tests and reformatted files

8805bc3

Fixed style for iOS classes

03858ec

Fixed analysis for master channel Fixed unit tests due do mismatching import

Fixed typos and reformatted files

9ae3891

stuartmorgan-g suggested changes Aug 16, 2022

View reviewed changes

AngeloAvv and others added 2 commits August 16, 2022 19:24

Merge branch 'main' into video_player_resolution

85a8656

# Conflicts: # packages/video_player/video_player_android/CHANGELOG.md # packages/video_player/video_player_android/pubspec.yaml

Merge branch 'main' into video_player_resolution

49e29fd

# Conflicts: # packages/video_player/video_player_android/CHANGELOG.md # packages/video_player/video_player_android/pubspec.yaml

hellohuanlin reviewed Aug 17, 2022

View reviewed changes

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m Outdated Show resolved Hide resolved

Added native tests for both Android and iOS. Fixed PR requests

9400bbe

github-actions bot removed the platform-web label Aug 18, 2022

AngeloAvv requested review from hellohuanlin and stuartmorgan-g and removed request for camsim99, ditman, gaaclarke and stuartmorgan-g August 18, 2022 15:11

stuartmorgan-g requested review from camsim99 and stuartmorgan-g August 18, 2022 20:18

hellohuanlin reviewed Sep 7, 2022

View reviewed changes

packages/video_player/video_player_avfoundation/CHANGELOG.md Outdated Show resolved Hide resolved

hellohuanlin approved these changes Sep 7, 2022

View reviewed changes

camsim99 reviewed Sep 8, 2022

View reviewed changes

AngeloAvv added 2 commits September 11, 2022 09:51

Merge remote-tracking branch 'origin/video_player_resolution' into vi…

682813f

…deo_player_resolution # Conflicts: # packages/video_player/video_player_android/CHANGELOG.md

Added width and height checks on iOS with tests. Updated changelogs

2f97743

AngeloAvv requested a review from bparrishMines as a code owner September 11, 2022 08:07

github-actions bot added p: image_picker p: webview_flutter Edits files for a webview_flutter plugin labels Sep 11, 2022

AngeloAvv and others added 2 commits September 11, 2022 10:09

Updated changelogs

fff2e93

Merge branch 'main' into video_player_resolution

887a2b9

# Conflicts: # packages/webview_flutter/webview_flutter_android/test/webview_android_widget_test.mocks.dart

camsim99 self-requested a review October 25, 2022 17:21

camsim99 reviewed Oct 31, 2022

View reviewed changes

bparrishMines suggested changes Nov 2, 2022

View reviewed changes

ditman reviewed Nov 3, 2022

View reviewed changes

stuartmorgan-g closed this Feb 22, 2023

		/// Sets the max video resolution to a [resolution] value allowing the player
		/// to reproduce lwoer resolution streams.

		/// The video resolution, the number of distinct pixels in each dimension that
		/// can be displayed

	/// allow the player to pick a different stream to play
	/// allow the player to pick a different stream to play.

Conversation

AngeloAvv commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AngeloAvv commented Aug 18, 2022

Uh oh!

AngeloAvv commented Sep 4, 2022

Uh oh!

jmagman commented Sep 7, 2022

Uh oh!

Uh oh!

hellohuanlin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camsim99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaryQian commented Oct 25, 2022

Uh oh!

camsim99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Jan 4, 2023

Uh oh!

reidbaker commented Feb 2, 2023

Uh oh!

AngeloAvv commented Feb 10, 2023

Uh oh!

stuartmorgan-g commented Feb 22, 2023

Uh oh!

Reviewers

Assignees

AngeloAvv commented Aug 11, 2022 •

edited

Loading