[video_player] VTT Support #2878

ferrazrx · 2020-07-15T17:03:27Z

Description

Class WebVTTCaptionFile created.

Related Issues

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.

googlebot · 2020-07-15T17:04:39Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

johnsonmh

This PR looks awesome! Thank you so much for the contribution.

I just left a few comments for suggestions/nits here and there.

packages/video_player/video_player/lib/src/web_vtt.dart

packages/video_player/video_player/example/ios/Flutter/Flutter.podspec

johnsonmh

Thanks for adding thorough tests!

packages/video_player/video_player/example/pubspec.yaml

packages/video_player/video_player/test/web_vtt.test.dart

cyanglaz

Thank you for this PR, I have left some comments. Also we need to update the CHANGELOG and the version number in the pubspec.yaml

packages/video_player/video_player/lib/src/web_vtt.dart

packages/video_player/video_player/test/web_vtt.test.dart

stuartmorgan-g · 2021-09-20T18:42:41Z

packages/video_player/video_player/lib/src/web_vtt.dart

@@ -0,0 +1,200 @@
+// Copyright 2020 The Chromium Authors. All rights reserved.


stuartmorgan-g · 2021-09-20T18:47:54Z

packages/video_player/video_player/lib/src/closed_caption_file.dart

-  final Duration start;
+  ///
+  /// When the value is null, the caption object is invalid.
+  final Duration? start;


This is a breaking change, because this whole file is exported.

Why do we need the ability to make invalid Caption objects in the first place? That seems like a strange thing to allow. If this is just for:

final Caption newCaption = Caption( number: captionNumber, start: startAndEnd.start, end: startAndEnd.end, text: textWithoutFormat, ); if (newCaption.start != null && newCaption.end != null) { captions.add(newCaption); captionNumber++; }

the caption is being thrown away anyway, so we can just not make it in the first place.

Good call.
Updated to make start and end non-null

stuartmorgan-g · 2021-09-20T18:48:45Z

packages/video_player/video_player/lib/src/web_vtt.dart

+      : _captions = _parseCaptionsFromWebVttString(fileContents);
+
+  /// The entire body of the Vtt file.
+  final String fileContents;


Why is this public?

It was probably a copy paste from SubRipCaptionFile in sub_rip.dart. Removed fileContents completely. Also added TODO in sub_rip.dart . It's going to be a breaking change to update it in sub_rip.dart and I think it would be better to update it when there's a meaningful breaking change for this plugin.

I'm not sure how to keep track of this tho. We might forgot about this when doing breaking changes in the plugin.

stuartmorgan-g · 2021-09-20T18:49:11Z

packages/video_player/video_player/lib/src/web_vtt.dart

+  final List<Caption> captions = <Caption>[];
+
+  // Ignore metadata
+  List<String> metadata = ['HEADER', 'NOTE', 'REGION', 'WEBVTT'];


stuartmorgan-g · 2021-09-20T18:49:45Z

packages/video_player/video_player/lib/src/web_vtt.dart

+    //  [00:00.000 --> 01:24.000 align:center]
+    //  ['Introduction']
+    // ]
+    // if caption has just header or time, but no text, captionLines.length will be 1


Please fix the comments to be capitalized and end with a period, per normal style.

stuartmorgan-g · 2021-09-20T18:57:02Z

packages/video_player/video_player/lib/src/web_vtt.dart

+  }
+
+  final List<String> dotSections = timestampString.split('.');
+  final List<String> hoursMinutesSeconds = dotSections[0].split(':');


Nit: timeComponents. There's no guarantee that this has all of these.

stuartmorgan-g · 2021-09-20T18:58:48Z

packages/video_player/video_player/lib/src/web_vtt.dart

+import 'closed_caption_file.dart';
+import 'package:html/parser.dart' as html_parser;
+
+/// Represents a [ClosedCaptionFile], parsed from the WebVtt file format.


Updated so all the appearance of WebVtt are WebVTT now.

stuartmorgan-g · 2021-09-20T18:58:56Z

packages/video_player/video_player/lib/src/web_vtt.dart

+import 'package:html/parser.dart' as html_parser;
+
+/// Represents a [ClosedCaptionFile], parsed from the WebVtt file format.
+/// See: https://en.wikipedia.org/wiki/WebVtt


This link is broken; it's https://en.wikipedia.org/wiki/WebVTT

stuartmorgan-g · 2021-09-20T19:04:12Z

packages/video_player/video_player/lib/src/web_vtt.dart

+    minutes = int.parse(hoursMinutesSeconds[1]);
+    seconds = int.parse(hoursMinutesSeconds[2]);
+  } else if (int.parse(hoursMinutesSeconds[0]) > 59) {
+    // Timestamp takes the form of [hours]:[minutes].[milliseconds]


This seemed deeply strange to me so I checked the spec, and this isn't a valid timestamp. We shouldn't handle invalid values unless there's substantial real-world evidence that lots of people have written this (which seems unlikely, because even if other parsers allow this it is incapable of expressing any time within the first 59 hours of a video)

stuartmorgan-g · 2021-09-20T19:13:40Z

packages/video_player/video_player/lib/src/web_vtt.dart

+    // Timestamp takes the form of [minutes]:[seconds].[milliseconds]
+    minutes = int.parse(hoursMinutesSeconds[0]);
+    seconds = int.parse(hoursMinutesSeconds[1]);
+  }


Once the above is removed, this can all condense to:

int hours = 0; if (hoursMinutesSeconds.length == 3) { hours = int.parse(timeComponents.removeAt(0)); } final int minutes = int.parse(timeComponents.removeAt(0)); final int seconds = int.parse(timeComponents.removeAt(0));

It would be good to add a safety check that the length is either 2 or 3 and returns null first, as well.

…errazrx/web_vtt

stuartmorgan-g

LGTM with various nits.

(If for whatever reason you feel like doing the bigger change, I can re-review that part, otherwise it's good to land without another review pass.)

packages/video_player/video_player/lib/src/closed_caption_file.dart

packages/video_player/video_player/lib/src/web_vtt.dart

stuartmorgan-g · 2021-09-21T20:08:23Z

packages/video_player/video_player/lib/src/web_vtt.dart

+
+  List<String> milisecondsStyles = dotSections[1].split(" ");
+
+  // TODO(cyanglaz): Handle caption styles.


As I'm reading this again: it's weird that _parseWebVTTTimestamp is where captions would be handled. I think the caller—which is already doing a regex match for timestamps anyway—should just use match groups (the regex would need to be adjusted slightly since right now it's making groups for all the sub-pieces of the timestamp, which isn't actually used) to extract just the timestamps, and then pass those here, removing the need for the milisecondsStyles = dotSections[1].split(" "). Then the caption-handling TODO would go in the calling function, not here.

But I'm fine with that being something that's cleaned up if/when this functionality is added since I don't want to keep piling changes onto a PR you adopted in the first place :)

Sounds good, I'll put a note in the code and link this comment.

stuartmorgan-g · 2021-09-21T20:08:57Z

packages/video_player/video_player/lib/video_player.dart

@@ -581,7 +581,12 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {

    // TODO: This would be more efficient as a binary search.
    for (final caption in _closedCaptionFile!.captions) {
-      if (caption.start <= position && caption.end >= position) {
+      final Duration? start = caption.start;
+      final Duration? end = caption.end;


This change is no longer needed.

fluttergithubbot · 2021-09-22T05:28:04Z

This pull request is not suitable for automatic merging in its current state.

This pull request has changes requested by @cyanglaz. Please resolve those before re-applying the label.

cyanglaz

Approving to fix CI

* master: (51 commits) [webview_flutter] Update version number app_facing package (flutter#4375) [webview_flutter] Adjust integration test domains (flutter#4383) Remove some trivial custom analysis options files (flutter#4379) [google_maps_flutter_platfomr_interface] Add Marker drag events (flutter#2653) [flutter_plugin_tools] Improve version check error handling (flutter#4376) [flutter_plugin_tools] Allow overriding breaking change check (flutter#4369) [url_launcher] Error handling when URL cannot be parsed with Uri.parse (flutter#4365) [webview_flutter] Migrate main package to fully federated architecture. (flutter#4366) [google_sign_in] Bump minimum Flutter version and iOS deployment target (flutter#4334) Add false secret lists, and enforce ordering (flutter#4372) [camera_web] Update usage documentation (flutter#4371) [video_player] VTT Support (flutter#2878) Require authors file (flutter#4367) [flutter_plugin_tools] Fix federated safety check (flutter#4368) [webview_flutter] Extract WKWebView implementation into a separate package (flutter#4345) [webview_flutter] Extract Android implementation into a separate package (flutter#4343) [in_app_purchase] Ensure the `introductoryPriceMicros` field is populated correctly. (flutter#4364) [flutter_plugin_tools] Add a federated PR safety check (flutter#4329) [camera] Add web support (flutter#4240) [webview_flutter] Bump minimum Flutter version and iOS deployment target (flutter#4361) ... # Conflicts: # packages/webview_flutter/webview_flutter/lib/platform_interface.dart # packages/webview_flutter/webview_flutter/lib/src/webview_method_channel.dart # packages/webview_flutter/webview_flutter/lib/webview_flutter.dart

)

Web VTT Supported Added

9788b63

ferrazrx requested review from amirh, bkonyi, bparrishMines, cyanglaz, digiter, gaaclarke, LHLL, matthew-carroll and mklim as code owners July 15, 2020 17:03

googlebot added the cla: no label Jul 15, 2020

ferrazrx changed the base branch from videoplayerUpdate to master July 15, 2020 17:06

ferrazrx mentioned this pull request Jul 15, 2020

Video player plugin should support WebVTT closed captions file format. flutter/flutter#50595

Open

Fixes

a66f5d7

ferrazrx changed the title ~~Ferrazrx/web vtt~~ [video_player] VTT Support Jul 15, 2020

Rafael Ferraz added 3 commits July 15, 2020 15:24

VTT with header added

6481331

VTT with header added

8f8b095

VTT with header added

da1c6e4

johnsonmh reviewed Jul 15, 2020

View reviewed changes

Rafael Ferraz added 2 commits July 15, 2020 16:14

VTT with header added

1a21621

New tests added

0d3549e

matthew-carroll removed their request for review July 15, 2020 20:46

johnsonmh reviewed Jul 15, 2020

View reviewed changes

packages/video_player/video_player/example/pubspec.yaml Outdated Show resolved Hide resolved

packages/video_player/video_player/test/web_vtt.test.dart Outdated Show resolved Hide resolved

cyanglaz reviewed Jul 15, 2020

View reviewed changes

packages/video_player/video_player/lib/src/web_vtt.dart Show resolved Hide resolved

packages/video_player/video_player/lib/src/web_vtt.dart Outdated Show resolved Hide resolved

packages/video_player/video_player/test/web_vtt.test.dart Outdated Show resolved Hide resolved

Rafael Ferraz added 5 commits July 15, 2020 19:13

tests organized by group

b018ea9

Changelog updated

b92f305

Sample vtt renamed

1573670

Sample vtt renamed

1e53fc5

Todo Styles commented

9034263

cyanglaz requested review from stuartmorgan-g and cyanglaz and removed request for amirh, bkonyi, bparrishMines, gaaclarke, LHLL, mklim and cyanglaz September 16, 2021 17:55

stuartmorgan-g suggested changes Sep 20, 2021

View reviewed changes

Chris Yang added 3 commits September 21, 2021 11:41

review

0d68872

Merge branch 'ferrazrx/web_vtt' of github.com:ferrazrx/plugins into f…

c97876c

…errazrx/web_vtt

Merge branch 'master' into ferrazrx/web_vtt

6c457a5

stuartmorgan-g approved these changes Sep 21, 2021

View reviewed changes

Chris Yang added 2 commits September 21, 2021 15:10

review 2

5ffff31

fix license header

c8f7c2c

cyanglaz added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Sep 22, 2021

fluttergithubbot removed the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Sep 22, 2021

cyanglaz approved these changes Sep 22, 2021

View reviewed changes

cyanglaz added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Sep 22, 2021

fluttergithubbot merged commit ed2e7a7 into flutter:master Sep 22, 2021

engine-flutter-autoroll mentioned this pull request Sep 22, 2021

Roll Plugins from 42a17dc4de1c to ed2e7a78aee8 (1 revision) flutter/flutter#90497

Merged

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

ed2e7a7 [video_player] VTT Support (flutter/plugins#2878)

cabd55e

fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Sep 22, 2021

ed2e7a7 [video_player] VTT Support (flutter/plugins#2878) (#90497)

321c872

amantoux pushed a commit to amantoux/plugins that referenced this pull request Sep 27, 2021

[video_player] VTT Support (flutter#2878)

4ceda45

clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Oct 29, 2021

ed2e7a7 [video_player] VTT Support (flutter/plugins#2878) (flutter#90497

c8f864e

)

KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021

[video_player] VTT Support (flutter#2878)

f821148

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[video_player] VTT Support #2878

[video_player] VTT Support #2878

ferrazrx commented Jul 15, 2020 •

edited

Loading

googlebot commented Jul 15, 2020

johnsonmh left a comment

johnsonmh left a comment

cyanglaz left a comment

stuartmorgan-g Sep 20, 2021

cyanglaz Sep 21, 2021

stuartmorgan-g Sep 20, 2021

cyanglaz Sep 21, 2021

stuartmorgan-g Sep 20, 2021

cyanglaz Sep 21, 2021

stuartmorgan-g Sep 20, 2021

stuartmorgan-g Sep 20, 2021

cyanglaz Sep 21, 2021

stuartmorgan-g Sep 20, 2021

cyanglaz Sep 21, 2021

stuartmorgan-g Sep 20, 2021

cyanglaz Sep 21, 2021

stuartmorgan-g Sep 20, 2021

cyanglaz Sep 21, 2021

stuartmorgan-g Sep 20, 2021

stuartmorgan-g Sep 20, 2021

cyanglaz Sep 21, 2021

stuartmorgan-g left a comment

stuartmorgan-g Sep 21, 2021 •

edited

Loading

cyanglaz Sep 21, 2021

stuartmorgan-g Sep 21, 2021

cyanglaz Sep 21, 2021

fluttergithubbot commented Sep 22, 2021

cyanglaz left a comment

		@@ -0,0 +1,200 @@
		// Copyright 2020 The Chromium Authors. All rights reserved.


		List<String> milisecondsStyles = dotSections[1].split(" ");

		// TODO(cyanglaz): Handle caption styles.

[video_player] VTT Support #2878

[video_player] VTT Support #2878

Conversation

ferrazrx commented Jul 15, 2020 • edited Loading

Description

Related Issues

Checklist

Breaking Change

googlebot commented Jul 15, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

johnsonmh left a comment

Choose a reason for hiding this comment

johnsonmh left a comment

Choose a reason for hiding this comment

cyanglaz 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan-g left a comment

Choose a reason for hiding this comment

stuartmorgan-g Sep 21, 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

fluttergithubbot commented Sep 22, 2021

cyanglaz left a comment

Choose a reason for hiding this comment

ferrazrx commented Jul 15, 2020 •

edited

Loading

stuartmorgan-g Sep 21, 2021 •

edited

Loading