-
Notifications
You must be signed in to change notification settings - Fork 167
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
Add VP9 SVC support #643
Add VP9 SVC support #643
Conversation
🦋 Changeset detectedLatest commit: 1842e51 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -142,6 +144,9 @@ export default class PCTransport extends EventEmitter { | |||
if ( | |||
!media.fmtp.some((fmtp): boolean => { | |||
if (fmtp.payload === codecPayload) { | |||
if (!fmtp.config.includes('x-google-start-bitrate')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use 0.7x target bitrates as start bitrate, in the test case with 720p AV1, the publisher will start from 540p, then go to 720p about 3~5 seconds if network is good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like it'll impact other codecs too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it will only work for svc codecs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one remaining question:
how should we adjust default bitrate when it comes to VP9? AFAIK, it's more efficient than vp8.
@@ -142,6 +144,9 @@ export default class PCTransport extends EventEmitter { | |||
if ( | |||
!media.fmtp.some((fmtp): boolean => { | |||
if (fmtp.payload === codecPayload) { | |||
if (!fmtp.config.includes('x-google-start-bitrate')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like it'll impact other codecs too?
) { | ||
hasDDExt = true; | ||
} | ||
return hasAV1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this true on Safari as well? is it able to publish AV1 in SVC mode? If not, it'd be good to add a browser type check here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, Safari doesn't have av1 in its codec capabilities. By the way, munging sdp to enable Dependency Descriptor works well for vp9 svc in Safari too.
For camera usage, have tested 0.7x vp8 bitrates and it looks well, so using same bitrates as av1 now. |
@@ -248,6 +249,8 @@ export function determineAppropriateEncoding( | |||
if (codec) { | |||
switch (codec) { | |||
case 'av1': | |||
case 'vp9': | |||
encoding = { ...encoding }; | |||
encoding.maxBitrate = encoding.maxBitrate * 0.7; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is too aggressive for VP9. What I've read is av1 is about 25% more efficient compared to vp9.
Perhaps use 0.85 or 0.9 for vp9?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the comparison result of what you have read is real-time case? In my test the bitrates could vary a lot with different encode parameters for different scenarios (real-time/file). I don't have a definite result too, maybe 0.85 would be better that 0.7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side note: not sure how expected it is in general if users set a custom videoEncoding
that we alter the maxBitrate
value based on the codec here.
If a user consciously chooses to publish av1 or vp9 with a desired maxBitrate
why would we opt to adjust that beyond what the user has set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukasIO The determineAppropriateEncoding
is only executed to calculate from default presets when the user does not provide a customvideoEncoding
, so it will not override the user's config in your case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, missed that, thank you!
@davidzhao @lukasIO Going to merge this PR so that we can enable vp9/av1 svc now, any concern about it? |
on client side publishDefaults: { videoCodec:"av1", on server config room: but getting error from all VP9 supported browsers Uncaught (in promise) DOMException: Failed to execute 'setRemoteDescription' on 'RTCPeerConnection': Failed to set remote offer sdp: Failed to set remote video description send parameters for m-section with mid='2'. |
@cnderrauber any update on this?? |
@anishmenon Which version of lk-server are you using? You can try v1.4.2 to test it again, and post your offer & answer sdp if it still can't work. |
No description provided.