-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: rust SVC example #3
Conversation
<figcaption>Receive preview</figcaption> | ||
</figure> | ||
</div> | ||
<figure> |
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.
dunno how to run lint
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.
Why did you modify it at all?
This project is too simple to have HTML linter
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 touched that file recently
then copy-pasted this file from master and here is result ...
it's not SVC yet, it's kinda simulcast |
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.
Overall, it would be much easier to review if there was just 2 commits:
- Copies
echo
example or whatever you take as a reference with 0 modifications - Changes necessary for SVC/Simulcasing features
Also can you submit fixes for resolution typos upstream as a standalone PR, please?
<figcaption>Receive preview</figcaption> | ||
</figure> | ||
</div> | ||
<figure> |
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.
Why did you modify it at all?
This project is too simple to have HTML linter
action: 'Init'; | ||
consumerTransportOptions: TransportOptions; | ||
producerTransportOptions: TransportOptions; | ||
routerRtpCapabilities: RtpCapabilities; |
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.
Why these formatting changes? Please keep current code style untouched.
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.
it was made by IDE :c
it doesn't even show that master and my branch differs
|
||
if (producer.kind === 'video') { | ||
videoProducer = producer; | ||
maxSpatialLayer = videoProducer.rtpParameters.encodings ? videoProducer.rtpParameters.encodings.length - 1 : -1 |
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.
Check if this works in Firefox, the order might be inverted there
Co-authored-by: Nazar Mokrynskyi <nazar@mokrynskyi.com>
@nazar-pc should we send message back server->client with updated preferred consumer layers, so we won't update those values locally on client? |
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.
Looks pretty good, though I didn't test it yet.
General comments:
- Use formatting from
node/.eslintrc.js
in TS code - Use standard Rust formatting for Rust file
- Squash history such that the first commit copy-pastes
echo
example with 0 changes and second commit adds required changes for this example, such that it is much easier to review comparing to current 27 commits
|
||
// And create producers for all tracks that were previously requested | ||
for (const track of mediaStream.getTracks()) { | ||
const codec = track.kind === 'video' ? device.rtpCapabilities.codecs?.find((codec) => codec.mimeType.toLowerCase() === 'video/vp9') ?? device.rtpCapabilities.codecs?.find((codec) => codec.mimeType.toLowerCase() === 'video/vp8') : undefined |
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.
VP9 SVC only works in Chrome, probably worth checking that or enable VP9 SVC only with special query parameter, so that it always works correctly by default.
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.
works in WebKit for me 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.
WebKit (Safari) still doesn't support VP9 at all by default, let alone SVC.
newPreferredSpatialLayer = preferredSpatialLayer - 1; | ||
newPreferredTemporalLayer = temporalLayers - 1; | ||
} else { | ||
newPreferredSpatialLayer = spatialLayers - 1; | ||
newPreferredTemporalLayer = temporalLayers - 1; |
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.
Math on these lines seems to be incorrect
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.
why?
that's an implementation of mediasoup-demo as we agreed before
- temporal can be decreased, we don't touch spatial
- we try to decrease spatial because temporal is already 0.
- we set max layers for both because they both were on 0.
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.
First branch here is correct, second branch is not because when it switches spatial layer down, it should switch temporal back to the highest value. Third branch simply allows layers to go into negative values instead of wrapping around.
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.
you misread the variable name... it doesn't go into a negative one...
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, indeed. then maxSpatialLayer
would be way less confusing than spatialLayers - 1
. And similarly increaseLayer
should use those variables instead of 2
.
if everything is fine, I'll just do the squash @nazar-pc |
I need squash the way I described to review it, there is 35 individual changes, it is hard to overlay them in my head to understand what the state of the codebase is as of the each individual commit. Having copy-paste of existing demo and modifications separately reduces the scope of review drastically. |
if (isFirefox) { | ||
setPreferredLayers(preferredSpatialLayer > 0 ? preferredSpatialLayer - 1 : spatialLayers - 1) | ||
return | ||
} |
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 don't understand what this is and this will go into negative numbers as well unless I'm missing something.
Firefox supports both temporal and spatial layers just fine.
versatica#809 is open upstream, thanks! |
No description provided.