[HLSTree] Fix and cleanup to encryption types/states

[CastagnaIT]

trafficstars

Description

regression caused by fix pr #1537 i think better not only fix but also do a bit of cleanup to enum states/types also added new test cases to try avoid possible future breakages

fix 1:

D+ make use of multiple EXT-X-KEY's

#EXT-X-KEY:METHOD=SAMPLE-AES-CTR,KEYFORMAT="urn:uuid:edef8ba9-79d6-4ace-a3c8-27dcd51d21ed",KEYFORMATVERSIONS="1",URI="data:text/plain;base64,AAAAMnBzc2gAAAAA7e+LqXnWSs6jyCfc1R0h7QAAABISEDMaOPkkyUtflN5hPeeZLjg="
#EXT-X-KEY:METHOD=SAMPLE-AES-CTR,KEYFORMAT="com.microsoft.playready",KEYFORMATVERSIONS="1",URI="data:text/plain;charset=UTF-8;base64,xAEAAAEAAQC6ATwAVwBSAE0ASABFAEEARABFAFIAIAB4AG0AbABuAHMAPQAiAGgAdAB0AHAAOgAvAC8AcwBjAGgAZQBtAGEAcwAuAG0AaQBjAHIAbwBzAG8AZgB0AC4AYwBvAG0ALwBEAFIATQAvADIAMAAwADcALwAwADMALwBQAGwAYQB5AFIAZQBhAGQAeQBIAGUAYQBkAGUAcgAiACAAdgBlAHIAcwBpAG8AbgA9ACIANAAuADAALgAwAC4AMAAiAD4APABEAEEAVABBAD4APABQAFIATwBUAEUAQwBUAEkATgBGAE8APgA8AEsARQBZAEwARQBOAD4AMQA2ADwALwBLAEUAWQBMAEUATgA+ADwAQQBMAEcASQBEAD4AQQBFAFMAQwBUAFIAPAAvAEEATABHAEkARAA+ADwALwBQAFIATwBUAEUAQwBUAEkATgBGAE8APgA8AEsASQBEAD4AKwBUAGcAYQBNADgAawBrAFgAMAB1AFUAMwBtAEUAOQA1ADUAawB1AE8AQQA9AD0APAAvAEsASQBEAD4APAAvAEQAQQBUAEEAPgA8AC8AVwBSAE0ASABFAEEARABFAFIAPgA="
#EXT-X-KEY:METHOD=SAMPLE-AES-CTR,KEYFORMAT="PRMNAGRA",KEYFORMATVERSIONS="1",URI="data:text/plain;base64,eyJrZXktaWQiOiIzMzFhMzhmOS0yNGM5LTRiNWYtOTRkZS02MTNkZTc5OTJlMzgiLCJlbWkiOiJjdHIiLCJwcm0iOiJleUpqYjI1MFpXNTBTV1FpT2lKamRISWlMQ0pyWlhsSlpDSTZJak16TVdFek9HWTVMVEkwWXprdE5HSTFaaTA1TkdSbExUWXhNMlJsTnprNU1tVXpPQ0o5In0="

the PR #1537 has broken the parsing because always set currentEncryptionType for each case, then the first EXT-X-KEY is set correctly, the second/third EXT-X-KEY broken the previously set currentEncryptionType value

fix 2:

the master manifest can have multiple EXT-X-SESSION-KEY's but the master manifest parsing stopped at the first unsupported EXT-X-SESSION-KEY, so if the supported EXT-X-SESSION-KEY is placed last it will never be reached (this is not a problem for D+, bacause "by luck?" set widevine as first)

fix 3:

the CHLSTree::ProcessEncryption check for widevine keysystem, but dont check if it is supported by the system so i add the m_supportedKeySystem check, somewhat similar to dash parser PS. this in future should be changed again the KS check should be not a task of the parser

Motivation and context

fix #1545

How has this been tested?

plutotv kid, for transitions from unencrypted->encrypted->unencrypted http://stitcher-ipv4.pluto.tv/v1/stitch/embed/hls/channel/6452c814939a590008567a3blivestitch/master.m3u8?deviceType=samsung-tvplus&deviceMake=samsung&deviceModel=samsung&deviceVersion=unknown&appVersion=unknown&deviceLat=0&deviceLon=0&deviceDNT=%7BTARGETOPT%7D&deviceId=%7BPSID%7D&advertisingId=%7BPSID%7D&us_privacy=1YNY&samsung_app_domain=%7BAPP_DOMAIN%7D&samsung_app_name=%7BAPP_NAME%7D&profileLimit=&profileFloor=&embedPartner=samsung-tvplus&profilesFromStream=true

D+ for the issue case

Screenshots (if appropriate):

Types of change

• [x] Bug fix (non-breaking change which fixes an issue)
• [x] Clean up (non-breaking change which removes non-working, unmaintained functionality)
• [ ] Improvement (non-breaking change which improves existing functionality)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that will cause existing functionality to change)
• [ ] Cosmetic change (non-breaking change that doesn't touch code)
• [ ] None of the above (please explain below)

Checklist:

• [ ] I have read the Contributing document
• [ ] My code follows the Code Guidelines of this project
• [ ] My change requires a change to the Wiki documentation
• [ ] I have updated the documentation accordingly

[Ronny-nerd]

Thanks for the quick fix ...CastagnaIT...with this you can start the stream of Disney+ Addon again without any problems.

https://jenkins.kodi.tv/blue/organizations/jenkins/xbmc%2Finputstream.adaptive/detail/PR-1547/2/artifacts

[zuzia-dev]

@CastagnaIT Thank you very much! With Linux steram now plays properly.

[CastagnaIT]

@glennguy there is one question that i haven't found a satisfactory answer to and so i don't know if it is currently right the parser

for example on this https://github.com/xbmc/inputstream.adaptive/blob/a002757f6c7a01ac0771f7bf1269168be0b5c71c/src/test/manifests/hls/encrypt_seq_stream_drm.m3u8 (its a shrinked D+ manifest) the first period have the EXT-X-KEY encryptions but after a EXT-X-DISCONTINUITY there are no EXT-X-KEY, this imo should be assumed as unencrypted segments or not? (at least on D+ are not encrypted)

by reading on specs it just says that EXT-X-KEY is applied to all segments but what should happens after a EXT-X-DISCONTINUITY? doesn't seem well explained to me on specs

I thought that a new period should be set as "unencrypted" and if there is a EXT-X-KEY do as usual, but currently, when parse a EXT-X-DISCONTINUITY and create a new period the m_encryptionState is copied, but the m_psshSets is not copied, so this should lead to a not working drm? to me seem inconsistent

if so, I was thinking of trying to remove these lines https://github.com/xbmc/inputstream.adaptive/blob/0e0820d5a91936a7ca5d953c0f7ad2bd0dce8c0f/src/common/Period.cpp#L39 https://github.com/xbmc/inputstream.adaptive/blob/0e0820d5a91936a7ca5d953c0f7ad2bd0dce8c0f/src/parser/HLSTree.cpp#L807-L812

another example https://storage.googleapis.com/shaka-demo-assets/angel-one-widevine-hls/playlist_v-0576p-1400k-libx264.mp4.m3u8 of: https://storage.googleapis.com/shaka-demo-assets/angel-one-widevine-hls/hls.m3u8

[glennguy]

I thought that a new period should be set as "unencrypted" and if there is a EXT-X-KEY do as usual,

The Angel One Shaka test stream is what I used to develop against back in 2020 where the first 2 segments are in the clear.

RFC doesn't say that MAP and KEY are reset after a discontinuity and so the implementation followed that I thought:

if (segmentInitialization)
{
  std::swap(rep->initialization_, newInitialization);
  // EXT-X-MAP init url must persist to next period until overrided by new tag
  newInitialization.url = new char[map_url.size() + 1];
  memcpy((char*)newInitialization.url, map_url.c_str(), map_url.size() + 1);
}

........

if (!current_pssh_.empty())
{
  if (currentPsshType == ENCRYPTIONTYPE_WIDEVINE)
  {
    rep->pssh_set_ = insert_psshset(NOTYPE);
    current_period_->encryptionState_ |= ENCRYTIONSTATE_SUPPORTED;
  }
  else if (currentPsshType == ENCRYPTIONTYPE_AES128)
    segment.pssh_set_ = insert_psshset(NOTYPE);
}

but currently, when parse a EXT-X-DISCONTINUITY and create a new period the m_encryptionState is copied, but the m_psshSets is not copied

Above is where psshset is copied in the original implementation. Maybe that has been lost at some point? I haven't checked/can't remember if there was a future update to this behaviour though.

It would be nice if everything was explicit but I guess EXT-X-DISCONTINUITY can even be triggered just because of an actual discontinuity in PTS between 1 segment and the next.

IMO we should prioritise:

1. Try to follow the spec
2. Follow where the majority have ignored spec and there is a de facto standard instead
3. Do workarounds if needed.

Hope this all makes sense!

[CastagnaIT]

ah ok i think i got it so this means that on the D+ manifest since is not specified the EXT-X-KEY after the discontinuity is intended that way, so continue with same encryption data (despite can works also not encrypted)

however if you try look at this again

https://github.com/xbmc/inputstream.adaptive/blob/0e0820d5a91936a7ca5d953c0f7ad2bd0dce8c0f/src/parser/HLSTree.cpp#L807-L812

seem there is an oversight also on old implementation, that line check for WIDEVINE on currentEncryptionType to add the psshset to the period this is done when parse the EXT-X-DISCONTINUITY tag but the EXT-X-KEY, if exists, is parsed after the EXT-X-DISCONTINUITY tag so this means that initially it add to the period a widevine psshset, but in the case the encryption change due to new EXT-X-KEY, the psshset added to the period, is not removed, nor the rep->m_psshSetPos is not cleanup

[glennguy]

Correct, it was not quite right. So maybe on disco instead of acting on the currentencryptiontype it's best to set a flag - this flag can be unset if we see ext-x-key before the next media segment, and if not then set the enc type/psshset?

[CastagnaIT]

@glennguy i added a new commit with some cleanups to disc code just to make a bit more explicit what discussed above

i have preferred make no changes on this PR about psshset "oversight" since i noticed that to improve things in better way is required more time and more changes

ready to review

[matthuisman]

thanks guys :) Much appreciated