Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[video_player] Fixes a bug where the aspect ratio of some HLS videos are incorrectly inverted #6507

Merged

Conversation

hellohuanlin
Copy link
Contributor

@hellohuanlin hellohuanlin commented Sep 27, 2022

The issue is that item.presentationSize returns the swapped width and height for some video streams (likely due to transform).

I did lots of research and wasn't able to find any workaround online. Then I thought that each segment of the HLS may have different resolution (e.g. different internet speed), so AVFoundation must be adjusting the presentationSize continuously. Fixing aspect ratio could also be part of that adjustment. So I used the same trick as the previous PR to leverage AVPlayerLayer to do the heavy lifting work for us.

Just like the previous PR, we can also leverage AVPlayerLayer to do the heavy lifting work for us. And it worked :)

(If it weren't for the previous iOS 16 bug, I probably wouldn't be able to come up with this solution).

About the deprecated keyWindow.rootViewController

I simply copied the code from another plugin. Fixing it is not the scope of this PR (tho I'm happy to help if needed).

List which issues are fixed by this PR. You must list at least one issue.
Fixes flutter/flutter#109116

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

// TODO: (hellohuanlin) Provide a non-deprecated codepath. See
// https://github.com/flutter/flutter/issues/104117
return UIApplication.sharedApplication.keyWindow.rootViewController;
#pragma clang diagnostic pop
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simply copied the code from another plugin. Fixing it is not the scope of this PR (tho I'm happy to help if needed).

@hellohuanlin hellohuanlin changed the title [video_player] fix the aspect ratio of some HLS videos being inverted [video_player] ixes a bug where the aspect ratio of some HLS videos are incorrectly inverted Sep 27, 2022
@hellohuanlin hellohuanlin changed the title [video_player] ixes a bug where the aspect ratio of some HLS videos are incorrectly inverted [video_player] Fixes a bug where the aspect ratio of some HLS videos are incorrectly inverted Sep 27, 2022
@hellohuanlin hellohuanlin force-pushed the video_player_fix_hls_aspect_ratio branch from 679b2ee to 8422c89 Compare September 27, 2022 23:20
@hellohuanlin hellohuanlin marked this pull request as ready for review September 27, 2022 23:20
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs a test to validate the presentationSize is correct, if that's possible to check.

@@ -66,12 +68,11 @@ @interface VideoPlayerTests : XCTestCase
@implementation VideoPlayerTests

- (void)testIOS16BugWithEncryptedVideoStream {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename test.

Comment on lines 14 to 18
// This is to fix 2 bugs: 1. blank video for encrypted video streams on iOS 16
// (https://github.com/flutter/flutter/issues/111457) and 2. swapped width and height for some video
// streams (not just iOS 16). (https://github.com/flutter/flutter/issues/109116).
// An invisible AVPlayerLayer is used to overwrite the protection of pixel buffers in those streams
// for issue #1, and restore the correct width and height for issue #2.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this comment twice.

Suggested change
// This is to fix 2 bugs: 1. blank video for encrypted video streams on iOS 16
// (https://github.com/flutter/flutter/issues/111457) and 2. swapped width and height for some video
// streams (not just iOS 16). (https://github.com/flutter/flutter/issues/109116).
// An invisible AVPlayerLayer is used to overwrite the protection of pixel buffers in those streams
// for issue #1, and restore the correct width and height for issue #2.

@hellohuanlin
Copy link
Contributor Author

It turned out that validating presentationSize is trickier than i thought - it takes setting up pretty much the whole plugin, rather than just a few units. On the other hand, integration tests can probably provide good confidence too, but also stuck with the flakiness in previous PR.

I plan to dig deeper, but given the P2 priority, let's land it for now and revisit afterwards? WDYT? @jmagman

@jmagman
Copy link
Member

jmagman commented Sep 30, 2022

On the other hand, integration tests can probably provide good confidence too, but also stuck with the flakiness in previous PR.

Checking the frames seemed flaky. I wouldn't have thought checking the width and height of the view would be flaky, since our tests that check the closed captioning isn't flaky. How were you testing it?

@hellohuanlin
Copy link
Contributor Author

hellohuanlin commented Sep 30, 2022

@jmagman I was referring to the flakiness of the integration test for the iOS 16 bug (previous PR), which is tracked here: flutter/flutter#112562

For this particular one, I am thinking about taking a screenshot and then scanning through pixels to determine the frame of the video area. (if i understand it correctly, since we are not using platform view, the video frames are simply drawn on the full-screen UIView just like any other widgets)

@hellohuanlin
Copy link
Contributor Author

@jmagman Oh to clarify, I haven't tried out integration tests for this one. I was worrying it's taking longer time since it's P2. But if time permits, I can try integration test before Landing it.

@jmagman
Copy link
Member

jmagman commented Sep 30, 2022

it's taking longer time since it's P2

We shouldn't land anything without proper tests, even P2s 🙂

@jmagman I was referring to the flakiness of the integration test for the iOS 16 bug (previous PR), which is tracked here: flutter/flutter#112562

That's what I'm referring to as well.

For this particular one, I am thinking about taking a screenshot and then scanning through pixels to determine the frame of the video area. (if i understand it correctly, since we are not using platform view, the video frames are simply drawn on the full-screen UIView just like any other widgets)

Scanning through pixels and taking screenshots was what was extremely flaky. I'm suggesting something more simple to just check the width and height of the video.

For example, using the video from the issue (note we shouldn't use that video, we should have our own that reproduces, though I wasn't able to get any of our test videos to repro), this fails on master on an iOS 14 simulator, but passes on this PR:

    testWidgets('m3u8 has correct aspect ratio', (WidgetTester tester) async {
      final MiniController livestreamController = MiniController.network(
        'https://videodelivery.net/11a75f7cf7314f5a880d49fb8bebaaed/manifest/video.m3u8',
      );
      await livestreamController.initialize();

      expect(livestreamController.value.isInitialized, true);
      expect(livestreamController.value.size.width, lessThan(livestreamController.value.size.height));
    });

to be added around here:

testWidgets('live stream duration != 0', (WidgetTester tester) async {

@hellohuanlin
Copy link
Contributor Author

hellohuanlin commented Sep 30, 2022

@jmagman Ah I didn't realize we have tests on the dart side! Was thinking about xcuitests. Let me try tomorrow.

@hellohuanlin hellohuanlin force-pushed the video_player_fix_hls_aspect_ratio branch from 8422c89 to f30a7b1 Compare October 3, 2022 23:16
@hellohuanlin
Copy link
Contributor Author

For the integration test, I wasn't able to create a video that reproduces the bug (more details here).

I created an issue to track it.

@hellohuanlin hellohuanlin requested a review from jmagman October 3, 2022 23:21
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hellohuanlin hellohuanlin added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 3, 2022
@auto-submit auto-submit bot merged commit 25e10b2 into flutter:main Oct 3, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 4, 2022
adam-harwood pushed a commit to adam-harwood/flutter_plugins that referenced this pull request Nov 3, 2022
mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: video_player platform-ios
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[video_player] m3u8 videos are distorted when played on iOS
2 participants