fix: Missing Source Resolution (and therefore WAB and VQ) in some cases #265
Conversation
|
|
||
| if (accessLog && accessLog.events) { | ||
| NSArray<AVPlayerItemAccessLogEvent *> *events = [accessLog.events copy]; | ||
|
|
There was a problem hiding this comment.
Something else we can do is try to scan this log for valid indicatedBitrates, though this doesn't always work
Maybe the calculation strategy below could be a fallback instead of the default behavior.. Curious what we think here, I'm pretty comfy with either option
| // TODO: what even throws here anyway? Is this catch-block really needed? | ||
| // @try { | ||
| for (int t = 0; t < item.tracks.count; t++) { | ||
| AVPlayerItemTrack *playerItemTrack = [ | ||
| [item tracks] objectAtIndex:t | ||
| ]; | ||
| if (playerItemTrack) { | ||
| AVAssetTrack *assetTrack = playerItemTrack.assetTrack; | ||
| if (assetTrack) { | ||
| AVMediaType mediaType = assetTrack.mediaType; | ||
| if (mediaType && [mediaType isEqualToString:AVMediaTypeVideo]) { | ||
| return playerItemTrack; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| // } @catch (NSException *exception) { | ||
| // NSLog(@"%@", exception.reason); | ||
| // } |
There was a problem hiding this comment.
Added a note to the other try/catch, but here it's still possible (if extremely unlikely) that the tracks property could change out from under you. In any case, for in loops are easier to write:
| // TODO: what even throws here anyway? Is this catch-block really needed? | |
| // @try { | |
| for (int t = 0; t < item.tracks.count; t++) { | |
| AVPlayerItemTrack *playerItemTrack = [ | |
| [item tracks] objectAtIndex:t | |
| ]; | |
| if (playerItemTrack) { | |
| AVAssetTrack *assetTrack = playerItemTrack.assetTrack; | |
| if (assetTrack) { | |
| AVMediaType mediaType = assetTrack.mediaType; | |
| if (mediaType && [mediaType isEqualToString:AVMediaTypeVideo]) { | |
| return playerItemTrack; | |
| } | |
| } | |
| } | |
| } | |
| // } @catch (NSException *exception) { | |
| // NSLog(@"%@", exception.reason); | |
| // } | |
| for (AVPlayerItemTrack *playerItemTrack in item.tracks) { | |
| if ([playerItemTrack.assetTrack.mediaType isEqual:AVMediaTypeVideo]) { | |
| return playerItemTrack; | |
| } | |
| } |
I also suggested here to use isEqual: instead of isEqualToString:. The only "benefit" of isEqualToString: is that it throws an exception if the argument or receiver is not a string. So it's almost always a good idea to just use isEqual:.
| @@ -518,6 +571,7 @@ - (CGRect)getViewBounds { | |||
| } | |||
|
|
|||
| - (CGSize)getSourceDimensions { | |||
| // TODO: What throws here? Is this try-catch even necessary | |||
There was a problem hiding this comment.
Probably since it manually iterates over _player.currentItem.tracks which means any of _player, currentItem, or tracks could all change mid-iteration. Using "fast iteration" takes care of this - behind the scenes it grabs an objectEnumerator property off the collection one time and uses that to implement the for in loop. enumerateObjectsUsingBlock: has the same benefit if you still need the index for some reason.
In any case I've flagged this for replacement in another ticket.
| NSError *error = nil; | ||
| AVKeyValueStatus status = [_playerItem.asset statusOfValueForKey:@"duration" error: &error]; | ||
| if (!error && status == AVKeyValueStatusLoaded) { | ||
| CMTime duration = [_playerItem.asset duration]; | ||
| float floatingSec = CMTimeGetSeconds(duration); | ||
| if (!isnan(floatingSec) && floatingSec > 0) { | ||
| float ms = floatingSec * 1000; | ||
| NSNumber *timeMs = [NSNumber numberWithFloat:ms]; | ||
| [videoData setVideoSourceDuration:[NSNumber numberWithLongLong:[timeMs longLongValue]]]; | ||
| } | ||
| _videoDuration = duration; | ||
| } | ||
|
|
||
| long alreadyAdvertisedBitrate = [self findMostRecentAdvertisedBitrateForPlayerItem:_playerItem]; | ||
| if (alreadyAdvertisedBitrate > 0) { | ||
| _lastAdvertisedBitrate = alreadyAdvertisedBitrate; | ||
| _lastDispatchedAdvertisedBitrate = alreadyAdvertisedBitrate; | ||
| videoData.videoSourceAdvertisedBitrate = [ | ||
| NSNumber numberWithFloat:alreadyAdvertisedBitrate | ||
| ]; | ||
| } | ||
| [MUXSDKCore dispatchEvent:dataEvent forPlayer:[self name]]; |
There was a problem hiding this comment.
It seems like this (and possibly the surrounding code) duplicates what's already in checkVideoData, maybe there's a way to avoid that?
Co-authored-by: Kevin Cassidy Jr <[email protected]>
When we can't get a source resolution or source bitrate, we can't calculate WAB, Scaling, or other VQ-related metrics.
Cases where Src Res/Bitrate was not picked up
AVPlayerItemorAVPlayerhad been used before (as when preloading or when pooling/reusing players), we won't see the events that report things like Source URL, Source Resolution, and Advertised BitrateEXT-X-BITRATEtags (they are optional), people playing progressive mp4 files (possibly; I didn't test this, could depend on what is in themdat)Future-facing stuff
AVPlayerItem.tracks(including the.initialflag), we might be able to do some of these calculations there instead ofmonitorAVPlayerItem, and there'd be fewer paths that lead to this logic. Unsure if this is a real improvement or not, since the way this is written is a little more clear: "When I monitor an AVPlayerItem, I need to check for these specific things".Draft Until
BasicPlaybackExampleViewControllerrenditionchange, but this PR is about what happens when we can't see a rendition being selected and/or when there are no extra renditions to select.