[ASVideoNode] Use of URL in construction

[gazreese]

I'm not sure whether this is a bug, but thought I'd raise it anyway for discussion with @appleguy.

- (AVPlayerItem *)constructPlayerItem
{
  ASDN::MutexLocker l(_videoLock);

  AVPlayerItem *playerItem = nil;

  if (_asset != nil) {
    if (_asset.tracks.count > 0) {
      playerItem = [[AVPlayerItem alloc] initWithAsset:_asset];
    } else if ([_asset isKindOfClass:[AVURLAsset class]]) {
      playerItem = [[AVPlayerItem alloc] initWithURL:((AVURLAsset *)_asset).URL];
    }
  }

  return playerItem;
}

In my original changes to add URL / HLS Video support to the ASVideoNode I had explicit construction from either a URL or an AVAsset. This was because during my testing I had problems with the AVPlayerItem if I tried to construct from an HLS AVURLAsset rather than constructing directly from the URL. I can't remember exactly what the problem was if I'm honest, but it seemed prudent to mirror the construction of the AVPlayerItem in the ASVideoNode interface as this was the underlying tech.

Since the original merge the API for ASVideoNode has been simplified to only take an AVAsset. I assume someone has run into similar problems I had however as looking at the code above the internals have been reverted to creating the AVPlayerItem directly from the URL, except this time the URL is extracted from the AVAsset and the player item is constructed without it.

What this does however is drop the use of the AVURLAsset that the user uses for construction without any warning. If the user constructed the video node with a normal AVAsset then after construction videoNode.playerItem.asset would be the same as videoNode.asset. If the user observed anything in the AVAsset before construction or accessed anything inside videoNode.asset everything would work.

Looking at it now however if the user constructs with an AVURLAsset the asset is dropped. I assume the AVPlayerItem makes a new asset if you construct with the raw URL, so videoNode.playerItem.asset would no longer be the same object as videoNode.asset.

The .asset property is also used throughout the class, i.e.

- (void)generatePlaceholderImage
{
  ASVideoNode * __weak weakSelf = self;
  AVAsset * __weak asset = self.asset;

  [self imageAtTime:kCMTimeZero completionHandler:^(UIImage *image) {
    ASPerformBlockOnMainThread(^{
      // Ensure the asset hasn't changed since the image request was made
      if (ASAssetIsEqual(weakSelf.asset, asset)) {
        [weakSelf setVideoPlaceholderImage:image];
      }
    });
  }];
}

Looking at where this is called, you can see we're probably generating the placeholder image from the AVAsset that we're no longer using:

- (void)fetchData
{
  [super fetchData];

  {
    ASDN::MutexLocker l(_videoLock);

    AVPlayerItem *playerItem = [self constructPlayerItem];
    self.currentItem = playerItem;

    if (_player != nil) {
      [_player replaceCurrentItemWithPlayerItem:playerItem];
    } else {
      self.player = [AVPlayer playerWithPlayerItem:playerItem];
    }

    if (_placeholderImageNode.image == nil) {
      [self generatePlaceholderImage];
    }
  }
}

Like I say, I'm not sure this is even a bug but it just didn't look ideal so thought I'd raise it for discussion.

[chourobin]

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.

[ejensen]

@chourobin That's great news! The workaround of extracting the URL is troublesome, as detailed by @gazreese. Let me know If you'd like any help with your PR 😃

[gazreese]

This sounds great to me guys, well spotted!

On 28 Apr 2016, at 19:48, Robin Chou notifications@github.com wrote:

hey @gazreese https://github.com/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 https://developer.apple.com/library/ios/releasenotes/AudioVideo/RN-AVFoundation-Old/ -- 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.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #1600 (comment)

[appleguy]

@chourobin @gazreese @ejensen: awesome collaboration here. Keep up the great work!