[ASVideoNode, ASVideoPlayerNode] Add video composition and audio mix capabilities

[flovouin]

Hi guys,

After some discussion on Slack, we decided to add two new properties to the ASVideoNode: videoComposition and audioMix. For those who are not familiar with these properties, they are defined in the AVPlayerItem, and give instruction to the player as how the tracks in the asset should be combined during playback (e.g. cross-fade between video or audio tracks).
With the current implementation of the ASVideoNode, it is hard to access the currentItem as soon as possible, as the user can't be notified when the AVPlayerItem is first constructed. This is solved by commit 16c6698, which introduces a new delegate method in the ASVideoNodeDelegate. This way the user can access the currentItem as soon as possible, and set the videoComposition and audioMix properties if need be.
However this first solution is not the most convenient, as the user needs to keep a reference to the AVVideoComposition and AVAudioMix for later use (when the new delegate method is called), whereas the asset has already been "long ago". Hence the second commit fd6b877, which introduces the corresponding videoComposition and audioMix properties directly into the ASVideoNode. This way the user can simply set these properties at the same time he sets the asset. Of course these properties are entirely optional, and simply ignoring them keeps the default behaviour of the ASVideoNode.
These new properties make the new delegate method somewhat obsolete, but accessing the AVPlayerItem as soon as possible might still be of interest in other cases we haven't thought of.
Commit fc37235 simply propagates the changes to the ASVideoPlayerNode, for which the composition and audio mix can be set using a new initialiser.
Finally, commit 9b49453 forwards some missing delegate method from the ASVideoPlayer to the ASVideoPlayerNodeDelegate.

A (very basic) example of use for these properties is available here: https://github.com/flovouin/ASDK-AVVideoCompositionTest

Happy to discuss these changes more in depth, let me know what you think :-)
Cheers,

Flo

[appleguy]

@flovouin thank you, both for the awesome new features and your detailed write up! Having the test project is very helpful.

These features are fairly advanced, in the sense that most users will not need them. I think it is a good idea to support them, but we might want to look at any small ways that we can help stratify the API of these classes to help ensure that new readers are not overwhelmed by the length of the file. This could be as simple as grouping more advanced features in part of the header file, using a category, or perhaps we don't need to worry about this for right now.

@yunnanwu, @maicki, @binl,

[flovouin]

Thanks for your reply @appleguy !
Yes I agree, these properties won't be used by most users, and they shouldn't confuse someone who just wants to play a simple video. Good thing is, the default behaviour of the ASVideoNode stays the same if you simply ignore these properties.
As these properties are present in AVPlayerItem, I think simply moving them down the header file should be enough, as someone who explored the extended capabilities of the AVPlayerItem (i.e. scrolled down the header file haha) should have come across them anyway. However I don't have any strong opinion on the subject, so whatever you think is best is totally fine by me.

[appleguy]

We should get this in soon. Can anyone review?

Sent from my iPhone

On Jun 23, 2016, at 9:04 AM, Flo notifications@github.com wrote:

Thanks for your reply @appleguy !
Yes I agree, these properties won't be used by most users, and they shouldn't confuse someone who just wants to play a simple video. Good thing is, the default behaviour of the ASVideoNode stays the same if you simply ignore these properties.
As these properties are present in AVPlayerItem, I think simply moving them down the header file should be enough, as someone who explored the extended capabilities of the AVPlayerItem (i.e. scrolled down the header file haha) should have come across them anyway. However I don't have any strong opinion on the subject, so whatever you think is best is totally fine by me.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[binl]

@flovouin thanks for using and helping improve ASVideoNode! The test project looks great!

I agree with @appleguy here that these advanced features aren't very applicable to most of the users of this module. However, the fact that you needed to add properties to ASVideoNode to achieve what you are trying to do here is also concerning, I think reconsider how ASVideoNode should serve people or provide other Media Player modules that are less coupled with AVPlayer and AVPlayerItem could be helpful to you.

[appleguy]

@binl it would be really great if we could make a few changes that allow for flexibility in the feature set and customizability of the video components… perhaps we could introduce a protocol type of system, similar to how ASNetworkImageNode and ASMultiplexImageNode are able to support completely customized Image downloading and caching, but also offer a great out-of-the-box experience if you don't provide these.

@steveram @Xinchi - could you each think about this with @binl and if a proposal arises, we can figure out who in the community can implement? We are open to breaking API changes as 2.0 is coming up.

[flovouin]

This looks like it's a discussion for the core ASDK team, but happy to help if needed!

[appleguy]

@flovouin thanks for this! I've decided to take the diff; your implementation is quite clean, and we can look at simplifying / refactoring the API in the future when there is more focus on that. It may result in some minor thrash for clients that use these options, but I think it would be very easy to adapt if we added a helper object for advanced controls etc.

Could you update / rebase this to account for using the _propertyLock as is done by this diff? #1877

[flovouin]

@appleguy Thanks for taking the time to review it. I've rebased on master and updated the new properties to use the _propertyLock. It should be good to go!