Skip to content

feat: deduplicate text streams in period flattening#2885

Merged
joeyparrish merged 4 commits intoshaka-project:masterfrom
valotvince:feat-dedup-text-streams
Oct 1, 2020
Merged

feat: deduplicate text streams in period flattening#2885
joeyparrish merged 4 commits intoshaka-project:masterfrom
valotvince:feat-dedup-text-streams

Conversation

@valotvince
Copy link
Contributor

Title said it all :)

In a multi-period stream with the same text streams over periods, we would get back duplicates text tracks when calling getTextTracks player API.

That PR should fix that behaviour.

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good otherwise. Thanks!

a3.bandwidth = 97065;
a2.roles = ['role1', 'role2'];

// t1 and t3 are duplicats
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit: typo on "duplicates"

Larger concern: the test has no expectations on the text streams, so it would pass even without your code change. Please add a check to the end of the test to show that t3 was filtered out. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't see that one 😅

@joeyparrish joeyparrish added the type: bug Something isn't working correctly label Sep 30, 2020
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@shaka-bot
Copy link
Collaborator

Test Failure:

Chrome 85.0.4183.121 (Linux x86_64)
  DashParser Manifest sets contentType to text for embedded text mime types FAILED
    Error: Expected 1 to be 2.
        at <Jasmine>
        at _callee41$ (test/dash/dash_parser_manifest_unit.js:1056:41 <- test/dash/dash_parser_manifest_unit.js:1268:49)
        at tryCatch (node_modules/babel-polyfill/dist/polyfill.js:6900:40)
        at Generator.invoke [as _invoke] (node_modules/babel-polyfill/dist/polyfill.js:7138:22)

    TypeError: Cannot read property 'type' of undefined
        at _callee41$ (test/dash/dash_parser_manifest_unit.js:1060:36 <- test/dash/dash_parser_manifest_unit.js:1273:44)
        at tryCatch (node_modules/babel-polyfill/dist/polyfill.js:6900:40)
        at Generator.invoke [as _invoke] (node_modules/babel-polyfill/dist/polyfill.js:7138:22)
        at Generator.prototype.<computed> [as next] (node_modules/babel-polyfill/dist/polyfill.js:6952:21)
        at step (test/dash/dash_parser_manifest_unit.js:3:191)
        at test/dash/dash_parser_manifest_unit.js:3:361

Failed 1 tests, passed 1961, skipped 31

@shaka-bot
Copy link
Collaborator

All tests passed!

@joeyparrish joeyparrish merged commit 54d48e1 into shaka-project:master Oct 1, 2020
joeyparrish pushed a commit that referenced this pull request Oct 1, 2020
Similar to what was already done for audio streams.
@valotvince valotvince deleted the feat-dedup-text-streams branch October 12, 2020 11:27
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

status: archived Archived and locked; will not be updated type: bug Something isn't working correctly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants