Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional boundary checks on VideoPlaybackQuality attributes #3231

Conversation

tidoust
Copy link
Contributor

@tidoust tidoust commented Jun 24, 2016

Also added checks on the totalFrameDelay attribute, in a separate test because the feature is marked as at-risk in the current CR.

Note the test incorrectly assumed that setting the duration would start a SourceBuffer update, which is no longer true.

Also added checks on the totalFrameDelay attribute, in a separate test
because the feature is marked as at-risk in the current CR.

Note the test incorrectly assumed that setting the duration would start a
SourceBuffer update, which is no longer true.
@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/6660

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@wpt-pr-bot
Copy link
Collaborator

Reviewers for this pull request are: @acolwell, @bit, @shishimaru, @sideshowbarker, and @wolenetz.

Truncating the duration is now forbidden if it truncates a buffered range.
@wolenetz
Copy link
Member

FF folks landed a conflicting change. I'll work through such conflict after reviewing the rest of these PRs from @tidoust.

FYI-@jyavenard and @tidoust: Chromium change https://codereview.chromium.org/2102323002 also updated (probably with similar conflicts) related tests to accommodate the spec change to disallow duration truncating whole coded frames, and to disallow aborting range removals.

@wolenetz
Copy link
Member

VPQ was removed from MSE v1 as it was an at-risk feature. This PR is deprioritized accordingly. I have no objection to including it in the suite, though, since some implementations of it had been done already, and the test improvements will carry over to media playback quality spec, most likely.

@jdsmith3000
Copy link
Contributor

These tests look fine to me, though as @wolenetz notes, they test a section of the spec that has been removed for V1. Merging them in does not seem to be necessary for V1.

@wolenetz
Copy link
Member

wolenetz commented Sep 7, 2016

#3656 replaces this PR. Thank you @tidoust for putting this together.

@wolenetz wolenetz closed this Sep 7, 2016
wolenetz added a commit that referenced this pull request Sep 7, 2016
…backquality

De-conflicted and slightly reordered fix of @tidoust's PR #3231
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants