[video_player] Make `initialize` not depend on duration, fix the live stream #1743

ianko · 2019-06-13T18:17:42Z

Description

The 0.10.1 version introduced the condition to have the duration in order to consider the player initialized. That change made the live stream videos not able to play anymore, since it does not have a duration.

Related Issues

flutter/flutter#35402
flutter/flutter#48670

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.

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.

…ontroller_dispose # Conflicts: # packages/video_player/CHANGELOG.md # packages/video_player/pubspec.yaml

…ontroller_dispose # Conflicts: # packages/video_player/CHANGELOG.md

…ller_dispose

collinjackson · 2019-06-17T02:53:20Z

Is there a better way to prevent the division by zero error that #1554 was trying to fix?

@iskakaushik @ened

ianko · 2019-06-17T13:59:26Z

@collinjackson Didn't know that the division by zero was the issue there. I will definitely work in a solution that solves both cases.

collinjackson · 2019-06-17T15:06:21Z

@collinjackson Didn't know that the division by zero was the issue there. I will definitely work in a solution that solves both cases.

Great, thanks! Let me know when this PR is ready for re-review.

…t_0.10.1

handle livestream better

googlebot · 2019-07-01T19:08:55Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

collinjackson · 2019-07-03T00:14:50Z

Checking in, it looks like you're still working on this PR since the tests don't pass so I'm marking it WIP. Just let us know when it's ready for re-review. Thanks!

ianko · 2019-07-03T03:09:32Z

@collinjackson We have a working solution already.

Taking some time to fix the errors from the example app. Will be finished by tomorrow.

googlebot · 2019-07-03T21:50:23Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

bduyng · 2019-07-05T15:16:32Z

@googlebot confirmed!

ianko · 2019-07-05T18:18:30Z

@collinjackson I am done with the PR.

Here are some things to notice:

We have added a better validation for the duration in the VideoPlayerPlugin.m file;
Better validation and checks in the video_player.dart;
Added a Live tab in the example app;

When testing the example app, I've noticed that, everytime we switch to a tab that plays a video from the assets, an exception is thrown and the other videos start to play as well. This was not introduced by this PR. I went back and tested previous versions and it does the same.

I refactored the whole example app trying to find the source of the problem but it's not in it. It's something from the player itself and I couldn't find the reason. That's probably why there was a controller.setVolume(0.0) in the deactive() method. To hide the fact that the video was playing.

I am sending the PR with the refactored example app (all the features remain the same). I believe it's cleaner this way. Let me know what you think.

About the failing CLA: @bduyng is the co-author of this PR. He already signed the CLA and also sent a confirmation message here.

cyanglaz · 2019-07-09T23:43:19Z

@ianko Thank you for the contribution. The PR seems pretty long and it includes different things. Do you mind to separate the refactor and the race condition fixing to different PRs?

ianko · 2019-07-10T15:49:49Z

@ianko Thank you for the contribution. The PR seems pretty long and it includes different things. Do you mind to separate the refactor and the race condition fixing to different PRs?

@cyanglaz,

It was already a different PR (#1562). I had to bring to try to fix the issue with the video from assets that I mentioned, but it does not solve this case.

Added a reference to a related issue flutter/flutter#35402 opened recently.

cyanglaz · 2019-07-10T17:03:49Z

@ianko Thanks, this PR seems still contains multiple stuff: the duration method addition and a refactoring of the example app. Do you mind separate them so one PR only contains a single feature/fix?

ianko · 2019-07-10T18:38:29Z

@cyanglaz Done.

#1562 - Fix race condition
#1743 - (this) Fix the live stream (initialize does not depend on duration)
#1829 - Example app refactor

marianoarga · 2019-07-16T19:11:15Z

I can confirm that 0.10.1 causes this issue
flutter/flutter#35402
Using the previous version (0.10.0+8) works as spected.

Thank you @ianko

cyanglaz

Thank you for the contribution. I have left some comments. One of them is a suggestion to an alternative solution for the problem. See if you like it.

cyanglaz · 2019-07-18T19:50:06Z

packages/video_player/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.10.2
+
+* iOS: Revert the changes made in version `0.10.1` that made live streams not able to initialize on the absence of `duration`.


Please also add what is added in this patch as well.

cyanglaz · 2019-07-18T19:55:51Z

packages/video_player/ios/Classes/VideoPlayerPlugin.m

+    for (NSValue* rangeValue in [_player currentItem].loadedTimeRanges) {
+      CMTimeRange range = [rangeValue CMTimeRangeValue];
+      int64_t start = FLTCMTimeToMillis(range.start);
+      if (start > maxBuffering) {


why did we need this check? Can start somehow be negative? If the start is 0, do we not add the range.duration to maxBuffering?

cyanglaz · 2019-07-18T20:00:11Z

packages/video_player/ios/Classes/VideoPlayerPlugin.m

 }

 - (int64_t)duration {
+  if (CMTIME_IS_INDEFINITE([[_player currentItem] duration])) {


I assume this is computing the length of loaded times in livestream. I personally would think the duration is the length of the whole video. Maybe create a different method for getting the "loadedTimes" for livestream videos?

cyanglaz · 2019-07-18T20:01:23Z

packages/video_player/lib/video_player.dart

  }

+  /// The duration in the current video.
+  Future<Duration> get duration async {


It would seem to fail if called on Android.

cyanglaz · 2019-07-18T20:14:31Z

packages/video_player/ios/Classes/VideoPlayerPlugin.m

    if (height == CGSizeZero.height && width == CGSizeZero.width) {
      return;
    }
-    // The player may be initialized but still needs to determine the duration.


I think the quicker fix can be something like

if ([self duration] == 0 && ![self isDurationIndefinate])

And create a method to check if the duration isDurationIndefinate as simple as

return CMTIME_IS_INDEFINITE([[_player currentItem] duration]);

This way we can avoid the breakage also we won't need to introduce the new api for duration anymore.

aletorrado · 2019-10-13T21:49:35Z

Hi, could you please review the CLA task that is pending to move forward? This is pretty much needed. Thanks

ianko · 2019-10-15T20:04:35Z

Hi, could you please review the CLA task that is pending to move forward? This is pretty much needed. Thanks

I have signed already.

I believe that @bduyng has also signed.

aletorrado · 2019-10-15T20:28:44Z

Ok, I think a project admin is needed to change the cla:no tag to cla:yes.

cyanglaz · 2019-11-07T20:01:57Z

@ianko Are you still planning to work on this? It seems that I left some review comments that haven't been addressed.

ianko · 2019-11-07T21:58:47Z

@ianko Are you still planning to work on this? It seems that I left some review comments that haven't been addressed.

@cyanglaz Unfortunately not in a short term.

HenglyEver · 2020-12-12T17:41:21Z

This bug seems to be very old and needed for everyone as I was reading through a lot of threads, so can you please fix it?

VictorLekweuwa · 2021-07-02T16:10:27Z

Please, is there a fix for this??

ianko added 5 commits May 2, 2019 16:20

Fix race condition on disposing the VideoController.

6f85e06

Merge branch 'master' of github.com:flutter/plugins into videoplayerc…

5883b42

…ontroller_dispose # Conflicts: # packages/video_player/CHANGELOG.md # packages/video_player/pubspec.yaml

Merge branch 'master' of github.com:flutter/plugins into videoplayerc…

d15419f

…ontroller_dispose # Conflicts: # packages/video_player/CHANGELOG.md

Merge remote-tracking branch 'upstream/master' into videoplayercontro…

93debc9

…ller_dispose

revert videoplayer 0.10.1 changes

ffedc7e

ianko requested review from cyanglaz and iskakaushik as code owners June 13, 2019 18:17

googlebot added the cla: yes label Jun 13, 2019

bduyng and others added 3 commits July 1, 2019 14:35

handle livestream better

a20c8d2

Merge remote-tracking branch 'upstream/master' into videoplayer_rever…

5f73640

…t_0.10.1

Merge pull request #1 from bduyng/master

4e74ed4

handle livestream better

googlebot added cla: no and removed cla: yes labels Jul 1, 2019

collinjackson added the WIP label Jul 3, 2019

(wip) refactor the example app

2af2529

ianko added 2 commits July 5, 2019 10:39

add progress indicator while initializing

bd6a6ad

set volume on init and dispose

f9a531d

ianko added 3 commits July 5, 2019 13:20

format

f243d3a

update CHANGELOG

2093139

format

138bfd3

separated changes to a single feaure

c5fdba6

ianko mentioned this pull request Jul 10, 2019

[video_player] Example App refactor #1829

Closed

13 tasks

cyanglaz changed the title ~~[video_player] Revert changes from 0.10.1~~ [video_player] Make initialize not depend on duration, fix the live stream Jul 18, 2019

cyanglaz suggested changes Jul 18, 2019

View reviewed changes

cyanglaz self-assigned this Jul 18, 2019

cyanglaz added the in review label Jul 31, 2019

ianko closed this Dec 23, 2019

aidandavis mentioned this pull request Aug 11, 2020

Flutter video_player unable to play HLS live video on iOS device flutter/flutter#48670

Closed

This was referenced Sep 18, 2020

[video_player] Bug/issue 48670 #3047

Closed

[video_player] Fixed HLS streams on iOS (#48760) #3048

Closed

danielroek mentioned this pull request Oct 1, 2020

Fixed HLS Stream on iOS #3096

Closed

5 tasks

ianko mentioned this pull request Jul 1, 2021

m3u8 wouldn't load on iOS when using live stream flutter/flutter#35402

Closed

		@@ -1,3 +1,7 @@
		## 0.10.2

		* iOS: Revert the changes made in version `0.10.1` that made live streams not able to initialize on the absence of `duration`.

[video_player] Make initialize not depend on duration, fix the live stream #1743

[video_player] Make initialize not depend on duration, fix the live stream #1743

Uh oh!

Conversation

ianko commented Jun 13, 2019 • edited by VladyslavBondarenko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

collinjackson commented Jun 17, 2019

Uh oh!

ianko commented Jun 17, 2019

Uh oh!

collinjackson commented Jun 17, 2019

Uh oh!

googlebot commented Jul 1, 2019

Uh oh!

collinjackson commented Jul 3, 2019

Uh oh!

ianko commented Jul 3, 2019

Uh oh!

googlebot commented Jul 3, 2019

Uh oh!

bduyng commented Jul 5, 2019

Uh oh!

ianko commented Jul 5, 2019

Uh oh!

cyanglaz commented Jul 9, 2019

Uh oh!

ianko commented Jul 10, 2019

Uh oh!

cyanglaz commented Jul 10, 2019

Uh oh!

ianko commented Jul 10, 2019

Uh oh!

marianoarga commented Jul 16, 2019

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

cyanglaz Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

cyanglaz Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

cyanglaz Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

cyanglaz Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

cyanglaz Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

aletorrado commented Oct 13, 2019

Uh oh!

ianko commented Oct 15, 2019

Uh oh!

aletorrado commented Oct 15, 2019

Uh oh!

cyanglaz commented Nov 7, 2019

Uh oh!

ianko commented Nov 7, 2019

Uh oh!

HenglyEver commented Dec 12, 2020

Uh oh!

VictorLekweuwa commented Jul 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

[video_player] Make `initialize` not depend on duration, fix the live stream #1743

[video_player] Make `initialize` not depend on duration, fix the live stream #1743

ianko commented Jun 13, 2019 •

edited by VladyslavBondarenko

Loading