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.

Sign up for GitHub

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

Jump to bottom

Add spatialScalability to VideoConfiguration #189

Merged
merged 4 commits into from
Apr 5, 2022
Merged

Add spatialScalability to VideoConfiguration #189

merged 4 commits into from
Apr 5, 2022

Conversation

drkron
Copy link
Contributor

@drkron drkron commented Jan 17, 2022 •
edited by pr-preview bot
Loading

Fixes #182

Preview | Diff

chcunningham
chcunningham reviewed Jan 25, 2022
View reviewed changes
Copy link
Contributor

@chcunningham chcunningham left a comment

Choose a reason for hiding this comment

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

All minor feedback

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@drkron
Copy link
Contributor Author

drkron commented Jan 25, 2022

All minor feedback

Thanks for the feedback! I've updated the PR according to your suggestions.

@drkron
Copy link
Contributor Author

drkron commented Feb 1, 2022

@chcunningham
What do you think of the current state of this PR? Do you have any other comments?

@aboba
Copy link

aboba commented Feb 2, 2022

Is "L1T1" always the default scalability mode for every codec? For VP8 in Chromium WebRTC, my understanding is that this is not the case.

@aboba aboba mentioned this pull request Feb 2, 2022
Merged
@chcunningham
Copy link
Contributor

chcunningham commented Feb 2, 2022 •
edited
Loading

cc @alvestrand

Is "L1T1" always the default scalability mode for every codec? For VP8 in Chromium WebRTC, my understanding is that this is not the case.

Very interesting, I hadn't realized. I found more context here.

Am I right that the vp8 default is entirely up to implementers? No spec that spells it out?

We could amend this area of the spec to say something like: "the default value implementer defined choice correspsonding to the default scalabilityMode (i.e. the mode you get if you don't specify one via setParameters())". It's a bit subtle, but probably less subtle than having different defaults between different APIs.

What do you think of the current state of this PR? Do you have any other comments?

LGTM % lingering default concern.

@chcunningham
Copy link
Contributor

LGTM, but ping @aboba @alvestrand to double check the approach in that last commit.

@alvestrand
Copy link

Comment on VP8: the implementors actually changed the default from L1T3 to L1T2 some time back. There were some howls of outrage from people who felt that L1T3 worked better for them, and a renewed interest in having a control surface for it.

aboba
aboba approved these changes Feb 23, 2022
View reviewed changes
Copy link

@aboba aboba left a comment

Choose a reason for hiding this comment

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

Looks fine.

aarongable pushed a commit to chromium/chromium that referenced this pull request Feb 24, 2022
Use spatialScalability instead of scalabilityMode for decodeInfo
1a95f2e 
An upcoming spec change to the MediaCapabilities API replaces
scalabilityMode with spatialScalability when calling
decodeInfo. This CL implements that change by adding
spatialScalability to VideoConfiguration.

See w3c/media-capabilities#189

Bug: chromium:1299424
Change-Id: I46215603640cc7f4cf97e7309be934f8adb1b358
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3484211
Reviewed-by: Will Cassella <[email protected]>
Reviewed-by: Henrik Boström <[email protected]>
Commit-Queue: Johannes Kron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#974639}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this pull request Mar 31, 2022
Merge M100: Use spatialScalability instead of scalabilityMode for dec…
82e1fa0 
…odeInfo

An upcoming spec change to the MediaCapabilities API replaces
scalabilityMode with spatialScalability when calling
decodeInfo. This CL implements that change by adding
spatialScalability to VideoConfiguration.

See w3c/media-capabilities#189

(cherry picked from commit 1a95f2e)

Bug: chromium:1299424
Change-Id: I46215603640cc7f4cf97e7309be934f8adb1b358
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3484211
Reviewed-by: Will Cassella <[email protected]>
Reviewed-by: Henrik Boström <[email protected]>
Commit-Queue: Johannes Kron <[email protected]>
Cr-Original-Commit-Position: refs/heads/main@{#974639}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3494416
Cr-Commit-Position: refs/branch-heads/4896@{#125}
Cr-Branched-From: 1f63ff4-refs/heads/main@{#972766}
@chcunningham
Copy link
Contributor

Dusting this off now that I'm back from leave. Thanks for reviewing! Merging now.

@chcunningham chcunningham merged commit 32d34de into w3c:main Apr 5, 2022
github-actions bot added a commit that referenced this pull request Apr 5, 2022
@chcunningham @github-actions
Merge pull request #189 from drkron/add_spatial_scalability
a68333d 
SHA: 32d34de
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
@copybara-github
Use spatialScalability instead of scalabilityMode for decodeInfo
7414501 
An upcoming spec change to the MediaCapabilities API replaces
scalabilityMode with spatialScalability when calling
decodeInfo. This CL implements that change by adding
spatialScalability to VideoConfiguration.

See w3c/media-capabilities#189

Bug: chromium:1299424
Change-Id: I46215603640cc7f4cf97e7309be934f8adb1b358
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3484211
Reviewed-by: Will Cassella <[email protected]>
Reviewed-by: Henrik Boström <[email protected]>
Commit-Queue: Johannes Kron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#974639}
NOKEYCHECK=True
GitOrigin-RevId: 1a95f2ee163601d91e812c276aebc4affb06e51f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chcunningham chcunningham chcunningham left review comments

@alvestrand alvestrand alvestrand approved these changes

@aboba aboba aboba approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use referenceScaling instead of scalabilityMode for decodingInfo queries
4 participants
@drkron @aboba @chcunningham @alvestrand