[video_player] add http headers #3671

PiN73 · 2021-03-03T11:14:27Z

This enables to pass HTTP headers to VideoPlayerController.network.

Actually this is rebasing @Yarikk26's implementation #897 on top of video_player 2.0.0.

Pre-launch Checklist

@Yarikk26

based on https://github.com/tvip/flutter-plugins/tree/videoplayer_http_headers/packages/video_player @Yarikk26 tvip@1bc99e9

… video_player_update_pigeon

PiN73 · 2021-03-03T11:43:31Z

As described in CONTRIBUTING.md, video_player in the PR is temporary using overriden video_player_platform_interface_dependency because it isn't published yet

stuartmorgan · 2021-03-03T21:13:44Z

Thanks for picking up this PR! I'll try to take a look in the next week or so.

BartusZak · 2021-03-04T09:43:33Z

Looking forward to see that.

stuartmorgan

Overall this looks really good. Two things it'll need:

The platform interface change moved to its own PR
Dart tests in the video_player package that ensure that headers passed into the API reach the platform interface (via a mock/fake platform interface implementation)

stuartmorgan · 2021-03-10T19:33:06Z

packages/video_player/video_player/example/pubspec.yaml

+# the parent directory to use the current plugin's version.
+dependency_overrides:
+  video_player_platform_interface:
+    path: ../../video_player_platform_interface


This will need to be removed, and the PR split into two parts, before landing.

Doesn't it make sense to keep the override to make anyone cloning the source to work with local copy?

Just like video_player: dependency in the example is declared as path: ../ to point to local copy of video_player package, this would do the same for video_player_platform_interface package

Doesn't it make sense to keep the override to make anyone cloning the source to work with local copy?

There are several issues with that, most notably that it causes the integration tests to run against a configuration that is different from what actual clients of this package would run, making it easy to have a PR pass CI and be landed and published, but break everyone because it doesn't work with the published interface package.

Just like video_player: dependency in the example is declared as path: ../ to point to local copy of video_player package

The example is part of that package. It's an internal reference.

this would do the same for video_player_platform_interface package

Overriding dependencies in a multi-package graph is definitely not the same thing.

OK, thanks, I understand the problem now. May it be commented out or should be completely deleted?

We don't check in commented out code, generally speaking. It's very easy for someone who wants this behavior for temporary testing to re-add it.

OK, I'll delete it. But the #3702 need to be landed first (if there aren't any other issues in the current PR) because the example won't built otherwise

Deleted it now

stuartmorgan · 2021-03-10T19:33:39Z

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

+               frameUpdater:(FLTFrameUpdater*)frameUpdater
+                httpHeaders:(NSDictionary<NSString*, NSString*>*)headers {
+  NSDictionary<NSString*, id>* options = nil;
+  if (headers != NULL) {


nil, not NULL

stuartmorgan · 2021-03-10T19:36:14Z

packages/video_player/video_player/lib/video_player.dart

+      {this.formatHint,
+      this.closedCaptionFile,
+      this.videoPlayerOptions,
+      this.httpHeaders})


This will format better if you add a comma after httpHeaders

packages/video_player/video_player_platform_interface/lib/messages.dart

PiN73 · 2021-03-11T15:51:33Z

@stuartmorgan I fixed all problems you mentioned except removing dependency_overrides in example app. If I remove it now, the example won't compile. Should #3702 be merged and I remove the override then, right?

PiN73 · 2021-03-23T12:03:23Z

Any additional edits needed from me?

stuartmorgan

Sorry for the review delay; doing both now.

stuartmorgan · 2021-03-23T15:36:45Z

packages/video_player/video_player/lib/video_player.dart

        super(VideoPlayerValue(duration: Duration.zero));

  /// The URI to the video file. This will be in different formats depending on
  /// the [DataSourceType] of the original video.
  final String dataSource;

+  /// HTTP headers used for the request to the [dataSource].
+  final Map<String, String>? httpHeaders;


We generally prefer to make collections non-nullable and defaulting to empty, so that we don't have both null and empty as codepaths that mean the same thing (except in cases where null and empty actually mean different things, but that's not true here). It avoids bugs where, e.g., one platform implementation handles them the same, but one fails with one or the other (or treats them differently on accident).

As pigeon doesn't currently support non-nullable fields, generated fields will be still nullable. If we make headers non-nullable in VideoPlayerController class, should we check for nulls in platform code then or rely that non-nullable value will be passed from Dart?

Well I made it non-nullable and empty by default. Also I kept check for null and added check for empty in platform code. So the default behavior is untouched (don't call function that sets headers in native code if no headers were passed). And also, if null will get into platform side somehow, it will be also ignored

stuartmorgan · 2021-03-23T15:37:26Z

packages/video_player/video_player/test/video_player_test.dart

@@ -30,6 +30,9 @@ class FakeController extends ValueNotifier<VideoPlayerValue>
  @override
  String get dataSource => '';

+  @override
+  Map<String, String>? get httpHeaders => null;


This reverts commit ff013d6

platform_interface portion of #3671

PiN73 · 2021-03-25T16:44:50Z

Now, as video_player_platform_interface 4.1.0 is published, I removed dependency overrides. Any additional changes needed, @stuartmorgan?

stuartmorgan

Looks good, thanks!

BartusZak · 2021-03-25T20:50:09Z

Thank you very much.

I'm going to test it right away :)

PiN73 added 7 commits March 1, 2021 16:34

[video_player] update pigeon generated files

f82e55a

[video_player] update pigeon generated files

4b90e51

add httpHeaders to pigeon

1006b93

headers implementation

b6bb419

based on https://github.com/tvip/flutter-plugins/tree/videoplayer_http_headers/packages/video_player @Yarikk26 tvip@1bc99e9

tests

45bda59

Merge remote-tracking branch 'origin/video_player_update_pigeon' into…

54975b6

… video_player_update_pigeon

version and changelog

0761edf

github-actions bot added p: video_player platform-android platform-ios labels Mar 3, 2021

google-cla bot added the cla: yes label Mar 3, 2021

stuartmorgan self-requested a review March 3, 2021 21:13

nt4f04uNd added a commit to nt4f04uNd/plugins that referenced this pull request Mar 9, 2021

merge flutter#3671

8eafed6

nt4f04uNd added a commit to nt4f04uNd/plugins that referenced this pull request Mar 9, 2021

merge flutter#3671

9dad8a7

nt4f04uNd added a commit to nt4f04uNd/plugins that referenced this pull request Mar 9, 2021

merge flutter#3671

5c1e3e8

stuartmorgan suggested changes Mar 10, 2021

View reviewed changes

PiN73 added 3 commits March 11, 2021 03:20

minor fixes

af53886

Merge branch 'master' into video_player_http_headers

8790077

tests

083ac9f

PiN73 mentioned this pull request Mar 11, 2021

[video_player_platform_interface] add http headers #3702

Merged

PiN73 requested a review from stuartmorgan March 15, 2021 08:59

PiN73 added 2 commits March 17, 2021 19:27

Merge branch 'master' into video_player_http_headers

d129eb8

Merge pull request #1 from flutter/master

5209ec9

stuartmorgan reviewed Mar 23, 2021

View reviewed changes

Merge branch 'master' into video_player_http_headers

33ecdb7

PiN73 added 3 commits March 23, 2021 19:37

update pigeon

ff013d6

Revert "update pigeon"

3d7d737

This reverts commit ff013d6

make headers non-nullable in VideoPlayerController and DataSource

706456d

PiN73 requested a review from stuartmorgan March 23, 2021 19:28

stuartmorgan pushed a commit that referenced this pull request Mar 24, 2021

[video_player_platform_interface] add http headers (#3702)

e9ed3cd

platform_interface portion of #3671

PiN73 added 4 commits March 24, 2021 19:35

Merge branch 'flutter/master' into video_player_http_headers

e13943d

remove dependency override from example

7c32f0a

remove dependency override from main package

d40f0bc

_

2a84a97

stuartmorgan approved these changes Mar 25, 2021

View reviewed changes

stuartmorgan merged commit 7c87a8c into flutter:master Mar 25, 2021

github-actions bot added the needs-publishing label Mar 25, 2021

engine-flutter-autoroll mentioned this pull request Mar 25, 2021

Roll Plugins from 809d0de989d3 to 7c87a8c52ce2 (1 revision) flutter/flutter#79083

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 25, 2021

7c87a8c [video_player] add http headers (flutter/plugins#3671)

ab1d27d

stuartmorgan removed the needs-publishing label Mar 25, 2021

fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Mar 25, 2021

7c87a8c [video_player] add http headers (flutter/plugins#3671) (#79083)

fd1ccd5

BartusZak mentioned this pull request Mar 30, 2021

[WEB] HLS with secured http headers balvinderz/video_player_web_hls#8

Closed

This was referenced Apr 8, 2021

Request headers for web ryanheise/just_audio#304

Open

[WEB] add http headers for audio player flutter/flutter#80039

Closed

Saifallak mentioned this pull request May 28, 2021

[video_player] Add Headers to Video player ex: (cookies and auth headers). flutter/flutter#83570

Closed

This was referenced Aug 2, 2021

[video_player_platform_interface] Add cookie support (1/3) #3136

Closed

[video_player] Add cookie support (3/3) #3138

Closed

[video_player_web] Add cookie support (2/3) #3137

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[video_player] add http headers #3671

[video_player] add http headers #3671

PiN73 commented Mar 3, 2021 •

edited

Loading

PiN73 commented Mar 3, 2021

stuartmorgan commented Mar 3, 2021

BartusZak commented Mar 4, 2021

stuartmorgan left a comment

stuartmorgan Mar 10, 2021

PiN73 Mar 11, 2021

stuartmorgan Mar 11, 2021

PiN73 Mar 11, 2021 •

edited

Loading

stuartmorgan Mar 11, 2021

PiN73 Mar 11, 2021 •

edited

Loading

PiN73 Mar 24, 2021

stuartmorgan Mar 10, 2021

stuartmorgan Mar 10, 2021

PiN73 commented Mar 11, 2021

PiN73 commented Mar 23, 2021

stuartmorgan left a comment

stuartmorgan Mar 23, 2021

PiN73 Mar 23, 2021 •

edited

Loading

PiN73 Mar 23, 2021 •

edited

Loading

stuartmorgan Mar 23, 2021

PiN73 commented Mar 25, 2021

stuartmorgan left a comment

BartusZak commented Mar 25, 2021

[video_player] add http headers #3671

[video_player] add http headers #3671

Conversation

PiN73 commented Mar 3, 2021 • edited Loading

Pre-launch Checklist

PiN73 commented Mar 3, 2021

stuartmorgan commented Mar 3, 2021

BartusZak commented Mar 4, 2021

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PiN73 Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PiN73 Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PiN73 commented Mar 11, 2021

PiN73 commented Mar 23, 2021

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PiN73 Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

PiN73 Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PiN73 commented Mar 25, 2021

stuartmorgan left a comment

Choose a reason for hiding this comment

BartusZak commented Mar 25, 2021

PiN73 commented Mar 3, 2021 •

edited

Loading

PiN73 Mar 11, 2021 •

edited

Loading

PiN73 Mar 11, 2021 •

edited

Loading

PiN73 Mar 23, 2021 •

edited

Loading

PiN73 Mar 23, 2021 •

edited

Loading