More useful info in segment-metadata cue (bandwidth/resolution/codecs/byte-length)

[tchakabam]

Description

adds effective segment bitrate (byte-length), recommended bandwidth, resolution and codecs string to cue metadata

this way we don't need to resolve this information from the master playlist using the URIs, which is very unhandy. The URI itself is usually not the data we really need.

Is there any cost involved in adding this?

Specific Changes proposed

Please list the specific changes involved in this pull request.

Requirements Checklist

• Feature implemented / Bug fixed
• If necessary, more likely in a feature request than a bug fix
  • Unit Tests updated or fixed
  • Docs/guides updated
  • Example created (starter template on JSBin)
• Reviewed by Two Core Contributors

[mjneil]

attributes may be undefined on the playlist object so you'll want to check for it before accessing the inner properties.

Also travis is failing because the linter prefers dot notation playlist.attributes.BANDWIDTH

[tchakabam]

should we not rather fix the fact that attributes may be undefined? it could be initialized to {} by default?

sure, we can go for dot notation :)

and how about we just add attributes as such property to the cue data? that way the user has all the info, no need to make this here a replication of the semantic that is already in attributes with lots of checks and so on. Because the attributes themselves may not be present in some playlists. It's just about making available all the info we have I guess :)

[mjneil]

fixing the attributes is something we want to address, see #1205 and videojs/m3u8-parser#25 so we could just wait on that before merging these changes in. Also just adding attributes as a property to value would work as well I'd think, though it may be better to individually set values so the cue isn't holding on the memory reference of the playlist object. Because of the way our PlaylistLoader works live playlist refreshes create a deep copy of the playlist and updating the copy, instead of just updating the original reference. The attributes aren't going to change on refreshes, and its possible there are other places in our code that is holding onto old playlist references, but its probably a good idea to not do so if we know about it

[mjneil]

attributes: videojs.mergeOptions(segmentInfo.playlist.attributes, {})

[squarebracket]

attributes being undefined was fixed in #1214. You do still need to change to dot notation though.

[squarebracket]

attributes being undefined was fixed in #1214. You do still need to change to dot notation though.

[squarebracket]

I feel like bitrate is a bit ambiguous here. Without seeing the code, I'd probably assume it would be the same thing as bandwidth. I think I favour dropping it for clarity's sake since it can be calculated from the other values. Is there a reason you're including it, other than convenience?

[squarebracket]

It would be nice if you could modify this test to test that the new attributes are added to the cue. You could modify this line for the test helper playlist creator to be

    attributes: conf && typeof conf.attributes !== 'undefined' ? conf.attributes : {}

and then you could simply attach some test data to the playlist creation with

        let playlist = playlistWithDuration(50, {attributes: {BANDWIDTH: 4500000, ...}});

[tchakabam]

oops, haven't looked into this since a while. yeah sure we can do that! will keep you posted about it.