[ASVideoNode] Initializing Streaming-based Video PlayerItems using URL instead of AVAsset #1954

Xinchi · 2016-07-19T23:14:49Z

Closing #1748 and opening this Pull Request, as the base in #1748 was still the old commit and after rebasing, it leads to the longest PR.

@gazreese @ejensen @hannahmbanana @levi @maicki

Apple claims in the AVFoundationProgramming Guide that HLS videos can be constructed only through URL, but later with iOS 4.3 release notes it claimed to bring updates to how the HLS videos should be initialized, which works with asset too. I’ve tested with both, and it looks like initializing with asset is buggy.

…nto patch-4

… it doesn't block the main thread

Xinchi · 2016-07-20T03:31:04Z

@chourobin Regarding your comment in #1600,

hey @gazreese, I'd just like to add that after further investigation, I believe it's no longer required to build the AVPlayerItem from the URL (for HLS). Hence, the check for tracks should no longer be needed.

Apparently the AVFoundation guide was out of date. In this release note -- under Enhancements for HTTP Live Streaming -- it states that all assets (HLS and non-HLS) may be constructed from an AVURLAsset. We just have to call -loadValuesAsynchronouslyForKeys:completionHandler: first before assigning it to a player item. I've been doing this outside of ASVideoNode for now but I would be happy to submit a PR to bring this async asset loading into ASVideoNode by default. I wanted to check in and see what you guys think.

Seems it's not entirely true that this issue has gone away in the AVFoundation, as @gazreese and I have observed the issue with HLS loading. So this is what this PR is for.

Xinchi · 2016-07-20T03:31:27Z

@gazreese Mind taking a look at this PR to see if it addressed the internal / public asset inconsistency issue you raised in #1600 ?

Xinchi · 2016-07-20T03:32:47Z

@ejensen Mind taking a look at this PR to see if it addressed the asynchronous loading of track key that you mentioned in #1748 ?

Xinchi · 2016-07-20T03:40:35Z

@gazreese @chourobin @ejensen @Eke
Do you think it's better for ASDK to tell if the AVUrlAsset specified by the client a HLS vs non-HLS asset by checking the track property, or is it better to provide a separate API for the client to specify which to construct the playeritem with, asset or url?

appleguy · 2016-07-20T06:33:00Z

AsyncDisplayKit/ASVideoNode.mm

+- (BOOL)hasURLAsset
+{
+  // The array of AVAssetTrack objects available via the tracks property of an URL asset is typically empty for streaming-based media
+  return _asset.tracks.count == 0;


@Xinchi Below, it looks like there is use of the safe / "correct" API to asynchronously load this value. Can we instead use that form of request / completion handler to restructure how we construct the player item, so the player item is created "as quickly as possible" even if it first goes through a call to loadValues...completionBlock: and the bulk of the work is in the completion block?

The constructPlayerItem method is called once the requestedKeys are loaded in the prepareToPlayAsset: withKeys: method.. not sure how we can make it faster than that?

gazreese · 2016-07-20T07:05:51Z

Hi @Xinchi. Regarding construction I'm keener on having the explicit construction using either the asset or the URL if I'm honest. This is reflected in ASVideoPlayerNode as well as the standard iOS APIs.

As for giving this a try, I've raised it up on my internal tracker and will have a look at it when I've finished what I'm working on.

…truction with URL

maicki · 2016-07-20T23:11:20Z

AsyncDisplayKit/ASVideoNode.h

@@ -41,6 +41,7 @@ NS_ASSUME_NONNULL_BEGIN
 - (BOOL)isPlaying;

 @property (nullable, nonatomic, strong, readwrite) AVAsset *asset;
+@property (nullable, atomic, strong, readwrite) NSURL *assetURL;


@Xinchi Any reason why this property is atomic?

Good catch! fixing..

levi · 2016-07-21T00:07:08Z

@Xinchi strong direction. Looks good to me!

maicki · 2016-07-21T00:13:58Z

@Xinchi Looks good to me!!

Adlai-Holler · 2016-07-21T00:22:04Z

AsyncDisplayKit/ASVideoNode.mm

+  _asset = [AVURLAsset assetWithURL:assetURL];
+
+  [self setNeedsDataFetch];
+}


It seems that -assetURL and -setAssetURL: are just convenience accessors for setting and retrieving the asset BY url. So couldn't we simplify with e.g.:

- (void)setAssetURL:(NSURL *)assetURL { if (!ASObjectIsEqual(assetURL, self.assetURL) { self.asset = [AVURLAsset assetWithURL:assetURL]; } }

etc?

Hls issue

appleguy · 2016-07-21T01:17:03Z

AsyncDisplayKit/ASVideoNode.mm

-    playerItem.audioMix = _audioMix;
-    return playerItem;
+  AVPlayerItem *playerItem = nil;
+  if (self.assetURL != nil) {


@Xinchi I'm not sure this is correct - what if we already have an _asset, which was created from this assetURL?

This should probably still be if _asset == nil, create the asset from the assetURL (if available). Then as a separate if condition, if we have a non-nil _asset, create the playerItem?

In this case you described, the method won't be invoked because this method is invoked in 3 cases:

fetch data is called from parent node

fetch data is called from setting the AssetURL

fetch data is called from setting the asset.

In #1, It shouldn't load the video asset more than once.
In #2, if we already have an _asset, which was created from this assetURL, the setter will be returned without setting the Asset and invoking the player item construction.
In #3, It's very unlikely that the application will set the assetURL first and then set the Asset. @maicki and I have discussed how we can prevent users from setting both the asset and url, and @maicki suggested we should add clear documentation for our users.

appleguy · 2016-07-21T01:18:58Z

AsyncDisplayKit/ASVideoNode.mm

+- (void)setAssetURL:(NSURL *)assetURL
+{
+  if (!ASObjectIsEqual(assetURL, self.assetURL)) {
+     self.asset = [AVURLAsset assetWithURL:assetURL];


What if the player item has been created already? Does the clearFetchedData / fetch data pass cause this to be updated correctly?

Yes - setting the asset will invoke [self clearFetchedData] and [self setNeedsDataFetch]; cc @Adlai-Holler who proposed this approach.

Xinchi and others added 3 commits June 14, 2016 10:15

Merge branch 'patch-4' of https://github.com/Xinchi/AsyncDisplayKit i…

7237149

…nto patch-4

ghost added the CLA Signed label Jul 19, 2016

Xinchi mentioned this pull request Jul 19, 2016

[ASVideoNode] Re-enabling HLS video constructed from URL #1748

Closed

Adding track to the requestedKeys to be loaded asynchronously so that…

44a8067

… it doesn't block the main thread

ghost added the CLA Signed label Jul 20, 2016

appleguy reviewed Jul 20, 2016
View reviewed changes

ghost added the CLA Signed label Jul 20, 2016

Max Gu added 2 commits July 20, 2016 16:01

Adding the new ASVideoNode API that allows for video player item cons…

f6eaa43

…truction with URL

Removing some unused method

b04aa2e

maicki reviewed Jul 20, 2016
View reviewed changes

ghost added the CLA Signed label Jul 20, 2016

Setting the internal asset to the player item's asest

76303f3

ghost added the CLA Signed label Jul 21, 2016

Combine 2 if statements into 1

b8ad8d6

ghost added the CLA Signed label Jul 21, 2016

Adlai-Holler reviewed Jul 21, 2016
View reviewed changes

Addressing Adlai's comment

6bd2870

Xinchi mentioned this pull request Jul 21, 2016

Update ASVideoNode.mm #1950

Closed

Adlai-Holler merged commit 5723f60 into facebookarchive:master Jul 21, 2016

Adlai-Holler pushed a commit that referenced this pull request Jul 21, 2016

Merge pull request #1954 from Xinchi/HLS_issue

b071714

Hls issue

appleguy reviewed Jul 21, 2016
View reviewed changes

Xinchi changed the title ~~Hls issue~~ [ASVideoNode] Initializing Streaming-based Video PlayerItems using URL instead of AVAsset Jul 21, 2016

appleguy reviewed Jul 21, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ASVideoNode] Initializing Streaming-based Video PlayerItems using URL instead of AVAsset #1954

[ASVideoNode] Initializing Streaming-based Video PlayerItems using URL instead of AVAsset #1954

Xinchi commented Jul 19, 2016

Xinchi commented Jul 20, 2016

Xinchi commented Jul 20, 2016

Xinchi commented Jul 20, 2016

Xinchi commented Jul 20, 2016

appleguy Jul 20, 2016

Xinchi Jul 20, 2016

gazreese commented Jul 20, 2016

maicki Jul 20, 2016

Xinchi Jul 20, 2016

levi commented Jul 21, 2016

maicki commented Jul 21, 2016

Adlai-Holler Jul 21, 2016 •

edited

Loading

appleguy Jul 21, 2016 •

edited

Loading

Xinchi Jul 21, 2016

appleguy Jul 21, 2016

Xinchi Jul 21, 2016

[ASVideoNode] Initializing Streaming-based Video PlayerItems using URL instead of AVAsset #1954

[ASVideoNode] Initializing Streaming-based Video PlayerItems using URL instead of AVAsset #1954

Conversation

Xinchi commented Jul 19, 2016

Xinchi commented Jul 20, 2016

Xinchi commented Jul 20, 2016

Xinchi commented Jul 20, 2016

Xinchi commented Jul 20, 2016

appleguy Jul 20, 2016

Choose a reason for hiding this comment

Xinchi Jul 20, 2016

Choose a reason for hiding this comment

gazreese commented Jul 20, 2016

maicki Jul 20, 2016

Choose a reason for hiding this comment

Xinchi Jul 20, 2016

Choose a reason for hiding this comment

levi commented Jul 21, 2016

maicki commented Jul 21, 2016

Adlai-Holler Jul 21, 2016 • edited Loading

Choose a reason for hiding this comment

appleguy Jul 21, 2016 • edited Loading

Choose a reason for hiding this comment

Xinchi Jul 21, 2016

Choose a reason for hiding this comment

appleguy Jul 21, 2016

Choose a reason for hiding this comment

Xinchi Jul 21, 2016

Choose a reason for hiding this comment

Adlai-Holler Jul 21, 2016 •

edited

Loading

appleguy Jul 21, 2016 •

edited

Loading