fix(HLS): Fix missing roles

[fredrik-telia]

Issue wasn't caught by the existing tests.

Closes #4759

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[avelad]

@fredrik-telia can you add a regression test? Thanks!

[github-actions]

Incremental code coverage: 100.00%

[fredrik-telia]

I might need some pointers, as there seems to be a test ( parses characteristics from text tags ) that passes, but shouldn't..

When using the player with a real stream, the role field in the stream object isn't filled, but the unit test seems to pass.

[fredrik-telia]

I need some help understanding why this line in the function testHlsParser in hls_parser_unit.js is truthy:

expect(actual).toEqual(manifest);
when they are, in fact, not equal at all

actual (using console.log(JSON.stringify(actual)):

'{"presentationTimeline":{"presentationStartTime_":null,"presentationDelay_":0,"duration_":null,"segmentAvailabilityDuration_":null,"maxSegmentDuration_":1,"minSegmentStartTime_":null,"maxSegmentEndTime_":null,"clockOffset_":0,"static_":true,"userSeekStart_":0,"autoCorrectDrift_":true,"availabilityTimeOffset_":0,"startTimeLocked_":false},"variants":[{"id":2,"language":"und","primary":false,"audio":null,"video":{"id":1,"originalId":null,"segmentIndex":null,"mimeType":"video/mp4","codecs":"avc1","encrypted":false,"drmInfos":[],"keyIds":{},"language":"und","label":null,"type":"video","primary":false,"trickModeVideo":null,"emsgSchemeIdUris":null,"frameRate":60,"width":960,"height":540,"roles":[],"forced":false,"channelsCount":null,"audioSamplingRate":null,"spatialAudio":false,"closedCaptions":null},"bandwidth":200,"allowedByApplication":true,"allowedByKeySystem":true,"decodingInfos":[]}],"textStreams":[{"id":3,"originalId":"English (caption)","segmentIndex":null,"mimeType":"text/vtt","codecs":"","encrypted":false,"drmInfos":[],"keyIds":{},"language":"en","label":"English (caption)","type":"text","primary":true,"trickModeVideo":null,"emsgSchemeIdUris":null,"roles":[],"forced":false,"channelsCount":null,"audioSamplingRate":null,"spatialAudio":false,"closedCaptions":null},{"id":4,"originalId":"English (caption)","segmentIndex":null,"mimeType":"text/vtt","codecs":"","encrypted":false,"drmInfos":[],"keyIds":{},"language":"en","label":"English (caption)","type":"text","primary":true,"trickModeVideo":null,"emsgSchemeIdUris":null,"roles":[],"forced":false,"channelsCount":null,"audioSamplingRate":null,"spatialAudio":false,"closedCaptions":null}],"imageStreams":[],"offlineSessionIds":[],"minBufferTime":0,"sequenceMode":true}'

vs manifest (same deal using JSON.stringify):

'{"variants":[{"sample":{"audio":null,"video":{"sample":{"type":"video"}}}}],"textStreams":[{"sample":{"type":"text","language":"en","kind":"subtitle","mimeType":"application/mp4","codecs":""}},{"sample":{"type":"text","language":"en","kind":"subtitle","mimeType":"application/mp4","codecs":"","roles":["public.accessibility.describes-spoken-dialog","public.accessibility.describes-music-and-sound"]}}],"imageStreams":[],"presentationTimeline":{},"offlineSessionIds":[],"minBufferTime":0,"sequenceMode":true}'

Maybe I'm missing something?

[avelad]

@theodab can you review it? Thanks!

[martinstark]

@avelad the issue that @fredrik-telia is pointing out looks quite serious, since that would mean the unit tests that currently exist for hls parser don't actually verify anything. It looks like whatever two objects are compared will pass the test. Should this be reported as a separate issue?

[avelad]

I think so

[martinstark]

would it make sense to also populate the kind property now that roles are being included?

[joeyparrish]

I need some help understanding why this line in the function testHlsParser in hls_parser_unit.js is truthy:

expect(actual).toEqual(manifest); when they are, in fact, not equal at all

actual (using console.log(JSON.stringify(actual)):

'{"presentationTimeline":{"presentationStartTime_":null,"presentationDelay_":0,"duration_":null,"segmentAvailabilityDuration_":null,"maxSegmentDuration_":1,"minSegmentStartTime_":null,"maxSegmentEndTime_":null,"clockOffset_":0,"static_":true,"userSeekStart_":0,"autoCorrectDrift_":true,"availabilityTimeOffset_":0,"startTimeLocked_":false},"variants":[{"id":2,"language":"und","primary":false,"audio":null,"video":{"id":1,"originalId":null,"segmentIndex":null,"mimeType":"video/mp4","codecs":"avc1","encrypted":false,"drmInfos":[],"keyIds":{},"language":"und","label":null,"type":"video","primary":false,"trickModeVideo":null,"emsgSchemeIdUris":null,"frameRate":60,"width":960,"height":540,"roles":[],"forced":false,"channelsCount":null,"audioSamplingRate":null,"spatialAudio":false,"closedCaptions":null},"bandwidth":200,"allowedByApplication":true,"allowedByKeySystem":true,"decodingInfos":[]}],"textStreams":[{"id":3,"originalId":"English (caption)","segmentIndex":null,"mimeType":"text/vtt","codecs":"","encrypted":false,"drmInfos":[],"keyIds":{},"language":"en","label":"English (caption)","type":"text","primary":true,"trickModeVideo":null,"emsgSchemeIdUris":null,"roles":[],"forced":false,"channelsCount":null,"audioSamplingRate":null,"spatialAudio":false,"closedCaptions":null},{"id":4,"originalId":"English (caption)","segmentIndex":null,"mimeType":"text/vtt","codecs":"","encrypted":false,"drmInfos":[],"keyIds":{},"language":"en","label":"English (caption)","type":"text","primary":true,"trickModeVideo":null,"emsgSchemeIdUris":null,"roles":[],"forced":false,"channelsCount":null,"audioSamplingRate":null,"spatialAudio":false,"closedCaptions":null}],"imageStreams":[],"offlineSessionIds":[],"minBufferTime":0,"sequenceMode":true}'

vs manifest (same deal using JSON.stringify):

'{"variants":[{"sample":{"audio":null,"video":{"sample":{"type":"video"}}}}],"textStreams":[{"sample":{"type":"text","language":"en","kind":"subtitle","mimeType":"application/mp4","codecs":""}},{"sample":{"type":"text","language":"en","kind":"subtitle","mimeType":"application/mp4","codecs":"","roles":["public.accessibility.describes-spoken-dialog","public.accessibility.describes-music-and-sound"]}}],"imageStreams":[],"presentationTimeline":{},"offlineSessionIds":[],"minBufferTime":0,"sequenceMode":true}'

Maybe I'm missing something?

Here's an expanded version of what you logged in "manifest" for clarity:

{
  "variants": [
    {
      "sample": {
        "audio": null,
        "video": {
          "sample": {
            "type": "video"
          }
        }
      }
    }
  ],
  "textStreams": [
    {
      "sample": {
        "type": "text",
        "language": "en",
        "kind": "subtitle",
        "mimeType": "application/mp4",
        "codecs": ""
      }
    },
    {
      "sample": {
        "type": "text",
        "language": "en",
        "kind": "subtitle",
        "mimeType": "application/mp4",
        "codecs": "",
        "roles": [
          "public.accessibility.describes-spoken-dialog",
          "public.accessibility.describes-music-and-sound"
        ]
      }
    }
  ],
  "imageStreams": [],
  "presentationTimeline": {},
  "offlineSessionIds": [],
  "minBufferTime": 0,
  "sequenceMode": true
}

What you're missing in the serialization through JSON is that some of those objects are not actual variants, but rather partial matchers constructed with jasmine.objectContaining(). They only contain certain fields, and others will be ignored in the comparison. This is intentional, and good to keep tests focused on the specific behaviors they are verifying. When you serialize one of these partial matchers, you see it's sample field, which is a private field used by the matcher to drive it custom matching behavior within jasmine.

I'll double-check the existing unit tests to make sure this partial matching is working correctly.

[joeyparrish]

Partial matching is working, but the test has a particular step that loads the relevant data:

await loadAllStreamsFor(actual);

This is unrealistic in this case, because it triggers lazy-loading for all streams. The only way a user would trigger a similar code path is by cycling through every text track, audio language, and resolution.

So roles are only missing before lazy-loading, and every single test case triggers lazy-loading on every single track before checking expectations.

I'll add a test case that fails without your fix.

[joeyparrish]

would it make sense to also populate the kind property now that roles are being included?

Looks like the answer is "yes". That is also missing in the regression test I'm creating.

[joeyparrish]

I added a regression test. You'll see that it's failing on two counts: missing kind and malformed roles.

[joeyparrish]

I fixed two bugs in this PR:

• kind missing
• roles should be an array, not a string

Both of these were caught by the new test I added.

[joeyparrish]

Thank you to @fredrik-telia and @martinstark for highlighting the test issues. I'm very glad to have both the bug and the test issues fixed now!

I will merge this when the test pass is complete.