search

security-union / videocall-rs

teleconference system written in rust
https://www.videocall.rs
MIT License
1.36k stars 117 forks source link

Factor out AudioPeerDecoder and VideoPeerDecoder #85

Closed ronen closed 1 year ago

ronen commented 1 year ago

[First step towards #74] [Also includes the sequencing fixes from #83]

Factored out structs AudioPeerDecoder and VideoPeerDecoder, into a new submodule model::decode, so that the Attendants component no longer has any decoding logic in it.

Also moved VideoDecoderWithBuffer and configure_audio_context into model::decode, since they are only called by the decoders. All towards the goal of making model::decode a standalone bit that could be lifted into an eventual client-engine cate.

While doing this, DRY'd the code a bit: VideoPeerDecoder gets used for both video and screen share; and AudioPeerDecoder and VideoPeerDecoder are both instantiations of a generic PeerDecoder<...> that has the common keyframe and sequence mechanism.

And merged in the changes from #83 even though it hasn't yet been merged into main, since it looks like that's on its way to being accepted, and the relocation of the decoders in this PR would breaks auto merge.

darioalessandro commented 1 year ago

I just started to look at this, thank you for your contribution

darioalessandro commented 1 year ago

Hey @ronen I tested audio & video and lgtm.

Screen share seem to be broken, plus I see that the system is logging something on every frame, this is on the browser that is streaming the screen: Screenshot 2023-07-01 at 1 32 36 AM

ronen commented 1 year ago

Screen share seem to be broken, plus I see that the system is logging something on every frame, this is on the browser that is streaming the screen: Screenshot 2023-07-01 at 1 32 36 AM

I get the same behavior in main, so I don't think it's due to this PR... plus I think the errors are coming from encoding rather than decoding.

Screen share has always been a bit funky when i've tried it, hard to get it to start. I haven't filed an issue because I haven't tracked down what's going on in a clear manner. I thought it could have been related to connecting twice from two tabs in the same browser (which is how I'm doing my development/testing) -- I sometimes seem to have to start sharing in both connections then stop it in one of them for the sharing to appear, but I haven't found anything reliable.

darioalessandro commented 1 year ago

Hey @ronen can you please take another look with the latest main? Seems like screen sharing is working consistently for me. Yes you are right the log message is on the producer side, is this 100% unrelated to your PR? (I can't get main to print this)

ronen commented 1 year ago

Hi @darioalessandro, I tried on the latest main, but I'll try again with a clean build.

In the meantime... are we on different platforms? I'm on an intel x86_64 Mac, running the latest MacOS (13.4), testing in the latest Chrome (Version 114.0.5735.198 (Official Build) (x86_64). Doing all my testing on localhost, with two Chrome tabs connecting with two different usernames.

darioalessandro commented 1 year ago

arm Mac OS with the same version of Chrome

darioalessandro commented 1 year ago

I am looking at this again, I think that the issue is that we do not re-render the component on a couple of spots, I will push this over the line

ronen commented 1 year ago

I am looking at this again, I think that the issue is that we do not re-render the component on a couple of spots, I will push this over the line

Cool, thanks for looking into it more! Just got back from weekend away was about to dive in :)

ronen commented 1 year ago

Hey @darioalessandro, I can verify that d760779 makes screen sharing work better for me. But I notice that commit didn't make it into main -- was that intentional?

darioalessandro commented 1 year ago

@ronen I could not figure out how to commit to your mr directly (I could have forked your repo) so I'll do it on main in a sec