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

Audio randomly gets disabled after latest changes to webrtcbin #109

Closed
ehfd opened this issue Oct 12, 2023 · 12 comments
Closed

Audio randomly gets disabled after latest changes to webrtcbin #109

ehfd opened this issue Oct 12, 2023 · 12 comments
Labels
bug Something isn't working help wanted External contribution is required interface OS input, display, or audio interfaces transport Underlying media or data transport protocols web Web components including gst-web

Comments

@ehfd
Copy link
Member

ehfd commented Oct 12, 2023

Because the option to turn on and off audio was removed, it is expected that audio should always be on.

However, the audio goes off randomly.

It could be because we should check that both audio and video WebRTC streams, as well as the RTCDataChannel, are all connected before we declare the connection is established successfully. There is a possibility that this was omitted in the latest commits.

This has been reproduced by myself and a user who has reported the issue to me.

The metrics menu may, but not necessarily show the following:

Audio Stats
Latency: 0 ms
Codec: NA
Bit rate: 0.00 kbps
@ehfd ehfd added bug Something isn't working transport Underlying media or data transport protocols web Web components including gst-web interface OS input, display, or audio interfaces high-priority Must be addressed as soon as possible, remove when resolved labels Oct 12, 2023
@ehfd ehfd removed the high-priority Must be addressed as soon as possible, remove when resolved label Oct 12, 2023
@ehfd ehfd added the high-priority Must be addressed as soon as possible, remove when resolved label Oct 12, 2023
@ehfd
Copy link
Member Author

ehfd commented Oct 12, 2023

This issue has been tagged as Urgent.

@danisla

@ehfd ehfd changed the title Audio randomly does not get enabled after latest changes to webrtcbin Audio randomly gets disabled after latest changes to webrtcbin Oct 15, 2023
@ehfd
Copy link
Member Author

ehfd commented Oct 19, 2023

I believe that this issue is because the audio and video WebRTC elements do not cross-check that both protocols have been connected successfully.

@danisla
Copy link
Member

danisla commented Oct 20, 2023

How do you reproduce this?

@ehfd
Copy link
Member Author

ehfd commented Oct 20, 2023

@danisla Try to reload the web interface multiple times. Especially from a non-local connection passing through the internet. It's not guaranteed to happen every time, but I have at least one other user reproducing this.

The core problem is that the current code seemingly does not ensure that both WebRTC streams are established before exposing the web interface to the user.

@ehfd
Copy link
Member Author

ehfd commented Oct 20, 2023

Audio Stats
Latency: 0 ms
Codec: NA
Bit rate: 0.00 kbps

It seems like this is shown even when audio is active. Stats are not shown.

@ehfd
Copy link
Member Author

ehfd commented Oct 27, 2023

@Xosrov Could I ask how you solved this problem?

@Xosrov
Copy link

Xosrov commented Oct 28, 2023

Hey, I don't recall such an issue, but I did have a look at the new source code and I notice something:

if audio_only:
    self.build_audio_pipeline()
else:
    self.build_video_pipeline()

This implies the start_pipeline function is called twice; once for video and once for audio. Am I getting this right? If so, the self.pipeline object is being modified and that could cause the signaling process to be disturbed. There needs to be more logging to know for sure, though.

The way I had implemented it was to first preroll the capturing + encoding + packetizing pipeline, then attach and sync a specific webrtcbin to the already running pipeline, which In my experience made initializing the stream a lot faster.

Some other things (unrelated to this topic,) that you could consider adding to POSSIBLY improve the whole stream experience:

  • Activating all available header extensions for audio and video. these do show up in the resulting SDP, but I haven't tested if they have noticeable effect.
  • Since there are two webrtcbins now, I would enable NACK on both. I'm not sure why you're doing it here; as the if-statement implies you only use it for one and not both?
  • I also used to enable FEC specifically in audio streams, but again I haven't tested to see if they have an effect on audio quality.

I hope this helps.

@ehfd
Copy link
Member Author

ehfd commented Oct 28, 2023

Thanks for the massive help. @Xosrov
While I'm busy dealing with other things, I'll make sure to bake your suggestions into the code.

@ehfd ehfd added the help wanted External contribution is required label Nov 10, 2023
@ehfd
Copy link
Member Author

ehfd commented Jan 3, 2024

  • Since there are two webrtcbins now, I would enable NACK on both. I'm not sure why you're doing it here; as the if-statement implies you only use it for one and not both?
  • I also used to enable FEC specifically in audio streams, but again I haven't tested to see if they have an effect on audio quality.

Applied these two things in 6d16d00.
Let's see if fixing #131 is the core thing, though.

@ehfd ehfd removed the high-priority Must be addressed as soon as possible, remove when resolved label Jan 5, 2024
@ehfd
Copy link
Member Author

ehfd commented Jan 5, 2024

I understand that this is the same issue as #131. Please reopen if there are additional issues.

@ehfd ehfd closed this as completed Jan 5, 2024
@ehfd ehfd reopened this Feb 2, 2024
@ehfd
Copy link
Member Author

ehfd commented Feb 2, 2024

There is still an issue where one should ensure both WebRTC (video, audio) streams are connected before the start button is shown, and the reload button should be shown after failure in either of the two streams.

@ehfd ehfd added the high-priority Must be addressed as soon as possible, remove when resolved label Feb 2, 2024
@ehfd ehfd closed this as completed in eb45091 Apr 5, 2024
@ehfd
Copy link
Member Author

ehfd commented Apr 5, 2024

Please reopen if there are additional issues.

@ehfd ehfd removed the high-priority Must be addressed as soon as possible, remove when resolved label Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted External contribution is required interface OS input, display, or audio interfaces transport Underlying media or data transport protocols web Web components including gst-web
Projects
None yet
Development

No branches or pull requests

3 participants