[video_player] Fixed HLS Streams on iOS

[nathanaelneveux]

Description

Many HLS streams on iOS were being treated as incomplete files because their duration has a CMTime of CMTIME_IS_INDEFINITE which was being simplified to '0'. This PR replaces durations of CMTIME_IS_INDEFINITE with a constant borrowed from Exoplayer2 on Android - TIME_UNSET - which is similarly used for an unknown duration time.

Future iterations may want to deal with TIME_UNSET to provide better UI behavior.

Related Issues

Fixes #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.

• 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.

[mvanbeusekom]

LGTM

[ditman]

The pubspec.yaml file needs to be updated to the version: 2.0.0-nullsafety.9, otherwise this won't be publishable.

[cyanglaz]

Thank you so much for the PR and fix. I think the idea is generally ok but I left some comments and questions :)

[cyanglaz]

What's the significants to make it matches android? It seems like a high maintenance code as we don't have a test to cover it.

[nathanaelneveux]

The problem here to me really stems from:

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

Line 14 in 07cf89a

int64_t FLTCMTimeToMillis(CMTime time) {

The conversion to Milliseconds from CMTime removes some of the functionality that CMTime provides. As far as I can tell FLTCMTimeToMillis is already an attempt to mimic the timebase found in Exoplayer2 to make it easier to digest on the flutter side. But it is an incomplete implementation since the timebase used in Exoplayer2 uses constants to identify special cases. On iOS these special cases (CMTIME_IS_INDEFINITE being one of them) are part of the CMTime class.

I think there has to be a way to bubble up the complexity of CMTime to the flutter side and matching Exoplayer2's timebase for now is the best way to handle that. This way at least within the flutter code everything matches.

This PR heads in that direction but I tried to limit the scope as much as possible to fixing the issue - since it is a problem I have in a production app and I'd like to stop using dependency_overrides as soon as possible.

[cyanglaz]

I see, in this case, can we refactor the if ([self isDurationIndefinite]) into FLTCMTimeToMillis?

int64_t FLTCMTimeToMillis(CMTime time) {
  if (CMTIME_IS_INDEFINITE(time)) { return TIME_UNSET; }
  if (time.timescale == 0) { return 0; }
  return time.value * 1000 / time.timescale;
}

And I think FLTCMTimeToMillis should really be a static function as it is only accessed in this file.

[nathanaelneveux]

I've done the refactor but I'm a bit at a loss at how to create a test for this. Should I create a XCTest to test passing variations of CMTime to FLTCMTimeToMillis and checking their output? Or should I make an integration test - in which case wouldn't I need an example HLS video hosted somewhere like https://flutter.github.io/assets-for-api-docs/assets/videos/?

[nathanaelneveux]

added a test

[cyanglaz]

Thanks for the explanation. I have left a comment. Can we also add tests for this?

[cyanglaz]

I see, in this case, can we refactor the if ([self isDurationIndefinite]) into FLTCMTimeToMillis?

int64_t FLTCMTimeToMillis(CMTime time) {
  if (CMTIME_IS_INDEFINITE(time)) { return TIME_UNSET; }
  if (time.timescale == 0) { return 0; }
  return time.value * 1000 / time.timescale;
}

And I think FLTCMTimeToMillis should really be a static function as it is only accessed in this file.

[marcusrohden]

hey guys, any news about this?
it would be amazing to have the capacity to fetch streams on iOS again...

[andreaslans]

Any news on this? We are currently investigating Flutter as a framework for a new streaming app, but we are getting more and more unsure as to if this is feasible at all since a lot of the packages that are published as working actually have critial bugs like this that are not fixed even though they seem to have working PRs ready to merge...

[ditman]

I'm landing this ASAP.

[ditman]

@nathanaelneveux with the URL that you added (link), the 'live stream duration != 0' test passes, but with the one we found (link), it fails.

I've used the Shaka demo to add both m3u8s, and they both seem to represent "live" video.

Can you please take a look to find an explanation of why one live stream works but the other doesn't? Is this a quirk of m3u8s? a bug in the iOS implementation that needs to be patched? a little bit of both?

If so, I'd keep both stream URLs in the tests, to ensure this doesn't break in the future.

Thanks for your patience!

[nathanaelneveux]

@nathanaelneveux with the URL that you added (link), the 'live stream duration != 0' test passes, but with the one we found (link), it fails.

I've used the Shaka demo to add both m3u8s, and they both seem to represent "live" video.

Can you please take a look to find an explanation of why one live stream works but the other doesn't? Is this a quirk of m3u8s? a bug in the iOS implementation that needs to be patched? a little bit of both?

If so, I'd keep both stream URLs in the tests, to ensure this doesn't break in the future.

Thanks for your patience!

@ditman I'm having trouble with consistent playback of the https://storage.googleapis.com/shaka-live-assets/player-source.m3u8 in safari on MacOS using both the built in safari video player and the Shaka demo. I'm getting a lot of 404 errors for the video and audio segments - I wonder if this is causing the test to not pass?

[nathanaelneveux]

@ditman I've made a minimal test project to compare playing back https://storage.googleapis.com/shaka-live-assets/player-source.m3u8 with this PR vs without it.

With this minimal project I am able to confirm that without the changes in this PR https://storage.googleapis.com/shaka-live-assets/player-source.m3u8 does not playback. Overriding video_player with this PR (as outlined in a previous comment) does cause the live video to playback.

Minimal Test Project
main.dart

import 'package:flutter/material.dart';
import 'package:chewie/chewie.dart';
import 'package:video_player/video_player.dart';

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  MyHomePage({Key? key, required this.title}) : super(key: key);

  final String title;

  @override
  _MyHomePageState createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  late VideoPlayerController _videoPlayerController1;
  ChewieController? _chewieController;

  @override
  void initState() {
    super.initState();
    initializePlayer();
  }

  @override
  void dispose() {
    _videoPlayerController1.dispose();
    _chewieController?.dispose();
    super.dispose();
  }

  Future<void> initializePlayer() async {
    _videoPlayerController1 = VideoPlayerController.network(
        'https://storage.googleapis.com/shaka-live-assets/player-source.m3u8');
    await Future.wait([
      _videoPlayerController1.initialize(),
    ]);

    _chewieController = ChewieController(
      videoPlayerController: _videoPlayerController1,
      autoPlay: true,
      looping: true,
    );

    setState(() {});
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(widget.title),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            Expanded(
              child: Center(
                child: _chewieController != null &&
                        _chewieController!
                            .videoPlayerController.value.isInitialized
                    ? Chewie(
                        controller: _chewieController!,
                      )
                    : Column(
                        mainAxisAlignment: MainAxisAlignment.center,
                        children: const [
                          CircularProgressIndicator(),
                          SizedBox(height: 20),
                          Text('Loading'),
                        ],
                      ),
              ),
            ),
          ],
        ),
      ),
    );
  }
}

pubspec.yaml - Toggle the "dependency_overrides" block to test stable video_player vs. this PR

name: video_player_tester
description: A new Flutter project.
publish_to: 'none' # Remove this line if you wish to publish to pub.dev

version: 1.0.0+1

environment:
  sdk: ">=2.12.0 <3.0.0"

dependencies:
  flutter:
    sdk: flutter

  cupertino_icons: ^1.0.2
  chewie: ^1.0.0

#dependency_overrides:
  #TODO switch to newer version once https://github.com/flutter/plugins/pull/3360 lands
  #video_player:
      #git:
        #url: https://github.com/nathanaelneveux/flutter-plugins.git
        #path: packages/video_player/video_player
        #ref: issue-48670

dev_dependencies:
  flutter_test:
    sdk: flutter

flutter:
  uses-material-design: true

**All other files are Flutter default template

[nathanaelneveux]

There is something not quite right with https://storage.googleapis.com/shaka-live-assets/player-source.m3u8 - running the playlist through Apple's mediastreamvalidator (guide) produces quite a few errors. In contrast running https://cph-p2p-msl.akamaized.net/hls/live/2000341/test/master.m3u8 does not seem to produce errors.

I'm not familiar enough with the HLS specification to provide much more info than that.

[ditman]

@nathanaelneveux thanks for diving deeper. I'll rebase the branch and revert the URL so this is mergeable again.

[ditman]

I think this 87da9ab was the stuff that was breaking our tests. @nathanaelneveux, can you confirm that my change is valid?

(I tried to use the google-hosted m3u8 and I started getting errors consistent with what you mention here, lots of stream not found errors which were in fact breaking tests too. See this run.)