Warn user when multiple IP routes are being used for the WebRTC streaming #591

rafaellehmkuhl · 2023-11-24T21:29:09Z

joaoantoniocardoso

I feel I had a bit of difficulty reading/memorizing the text.

joaoantoniocardoso · 2023-11-27T16:03:21Z

src/stores/video.ts

+        text: `Cockpit detected more than one IP being used to route the video streaming.
+          This situation often leads to video stuterring, specially if one of the IPs is from a non-wired connection.
+          To prevent issues and have an optimum streaming experience, please open the configuration of one of your
+          video widgets and choose the IP that should be used for the video streamings.`,


Suggested change

text: `Cockpit detected more than one IP being used to route the video streaming.

This situation often leads to video stuterring, specially if one of the IPs is from a non-wired connection.

To prevent issues and have an optimum streaming experience, please open the configuration of one of your

video widgets and choose the IP that should be used for the video streamings.`,

text: `Cockpit detected more than one IP being used to route the video streaming.

This situation often leads to video stuterring, specially if one of the IPs is from a non-wired connection.

To prevent issues and have an optimum streaming experience, please follow the steps below:

1. open the configuration of one of your

video widgets, and

2. choose the IP that should be used for the video streaming.`,

I don't know how to format it, but the text should not be centered and should have new lines.

By the way, shouldn't this configuration be global in cockpit over on the stream widget ?

I liked the suggestions. Will change.

Thanks for the notice @joaoantoniocardoso. I did that in a rush and ended up not styling the message.

This popup is going to appear for most/all users, so it should indeed look good.

Nice, that's much better. Can you switch the bullets to just the numbers? Or vice-versa? And there's a space missing between the words "your" and "video" ("yourvideo")

Followed this guide for list formatting.

…explicitly set This allows one to track when it is was already populated, and react to that.

joaoantoniocardoso · 2023-11-27T17:06:29Z

src/stores/video.ts

+            <li>1. open the configuration of one of your video widgets</li>
+            <li>2. choose the IP that should be used for the video streaming</li>


Suggested change

<li>1. open the configuration of one of your video widgets</li>

<li>2. choose the IP that should be used for the video streaming</li>

<li>1. Open the configuration of one of your video widgets.</li>

<li>2. Choose the IP that should be used for the video streaming.</li>

(following this)

Nicer indeed.

… candidates were not yet defined

patrickelectric

Why not having the IP configuration done on cockpit over stream widget ?

rafaellehmkuhl · 2023-11-27T17:25:40Z

Why not having the IP configuration done on cockpit over stream widget ?

Actually when I was first coding this patch we didn't have the video store yet, so it wouldn't be doable, but now that we have, I think it's indeed a better solution, specially because the variable is shared between all video widgets.

Will change!

joaoantoniocardoso · 2023-11-27T17:26:29Z

Why not having the IP configuration done on cockpit over stream widget ?

It's a trade-off... I'm not opposed to, also because the signaling address is also global, already limiting the person to receive video from different sources when they live in different networks/vehicles.

patrickelectric · 2023-11-27T17:34:42Z

The most common use case is to have a single vehicle with multiple cameras, the black/white list is.... a list, allowing it to a single list to work with multiple widgets with different vehicles in different networks. Having the user to configure every video stream widget for a single vehicle does not make sense to me, having a global configuration allows all the use cases and also simplifies it. Em seg., 27 de nov. de 2023 às 14:26, João Antônio Cardoso < ***@***.***> escreveu:

…

Why not having the IP configuration done on cockpit over stream widget ? It's a trade-off... I'm not opposed to, also because the signaling address is also global, already limiting the person to receive video from different sources when they live in different networks/vehicles. — Reply to this email directly, view it on GitHub <#591 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJIYCOPYBINWCJTY6K6NJDYGTENBAVCNFSM6AAAAAA7ZSTUFCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRYGI4TONRSGI> . You are receiving this because you commented.Message ID: ***@***.***>

-- Patrick José Pereira Electronics Engineer

rafaellehmkuhl · 2023-11-27T17:37:31Z

The most common use case is to have a single vehicle with multiple cameras, the black/white list is.... a list, allowing it to a single list to work with multiple widgets with different vehicles in different networks. Having the user to configure every video stream widget for a single vehicle does not make sense to me, having a global configuration allows all the use cases and also simplifies it. Em seg., 27 de nov. de 2023 às 14:26, João Antônio Cardoso < @.***> escreveu:
…
Why not having the IP configuration done on cockpit over stream widget ? It's a trade-off... I'm not opposed to, also because the signaling address is also global, already limiting the person to receive video from different sources when they live in different networks/vehicles.

The configuration is already global, not widget-wise.
It's just a matter of moving the component to a configuration page. I'm doing that right now.

patrickelectric · 2023-11-27T17:39:58Z

The most common use case is to have a single vehicle with multiple cameras, the black/white list is.... a list, allowing it to a single list to work with multiple widgets with different vehicles in different networks. Having the user to configure every video stream widget for a single vehicle does not make sense to me, having a global configuration allows all the use cases and also simplifies it. Em seg., 27 de nov. de 2023 às 14:26, João Antônio Cardoso < @.***> escreveu:
…
Why not having the IP configuration done on cockpit over stream widget ? It's a trade-off... I'm not opposed to, also because the signaling address is also global, already limiting the person to receive video from different sources when they live in different networks/vehicles.

The configuration is already global, not widget-wise. It's just a matter of moving the component to a configuration page. I'm doing that right now.

This can be addressed in another PR if you prefer.

rafaellehmkuhl · 2023-11-27T17:49:38Z

The most common use case is to have a single vehicle with multiple cameras, the black/white list is.... a list, allowing it to a single list to work with multiple widgets with different vehicles in different networks. Having the user to configure every video stream widget for a single vehicle does not make sense to me, having a global configuration allows all the use cases and also simplifies it. Em seg., 27 de nov. de 2023 às 14:26, João Antônio Cardoso < @.***> escreveu:
…
Why not having the IP configuration done on cockpit over stream widget ? It's a trade-off... I'm not opposed to, also because the signaling address is also global, already limiting the person to receive video from different sources when they live in different networks/vehicles.

The configuration is already global, not widget-wise. It's just a matter of moving the component to a configuration page. I'm doing that right now.

This can be addressed in another PR if you prefer.

Agree. Will merge this one them.

Documents: - bluerobotics/cockpit#591 - bluerobotics/cockpit#573

rafaellehmkuhl requested review from joaoantoniocardoso and ES-Alexander November 24, 2023 21:29

patrickelectric approved these changes Nov 27, 2023

View reviewed changes

joaoantoniocardoso requested changes Nov 27, 2023

View reviewed changes

Allow availableIceIps variable to be undefined when it was not yet …

a47330e

…explicitly set This allows one to track when it is was already populated, and react to that.

rafaellehmkuhl force-pushed the warn-user-about-multiple-stream-ips branch from 792b373 to f496c54 Compare November 27, 2023 17:01

rafaellehmkuhl requested a review from joaoantoniocardoso November 27, 2023 17:02

joaoantoniocardoso reviewed Nov 27, 2023

View reviewed changes

Create routine to warn the user if the allowed IPs for the WebRTC ICE…

39a81d5

… candidates were not yet defined

rafaellehmkuhl force-pushed the warn-user-about-multiple-stream-ips branch from f496c54 to 39a81d5 Compare November 27, 2023 17:12

rafaellehmkuhl requested a review from joaoantoniocardoso November 27, 2023 17:13

joaoantoniocardoso approved these changes Nov 27, 2023

View reviewed changes

patrickelectric reviewed Nov 27, 2023

View reviewed changes

rafaellehmkuhl merged commit f1ae9b4 into bluerobotics:master Nov 27, 2023
7 checks passed

rafaellehmkuhl deleted the warn-user-about-multiple-stream-ips branch November 27, 2023 17:49

patrickelectric mentioned this pull request Nov 28, 2023

Stream black/white list configuration should be global and not per widget #602

Closed

ES-Alexander added the docs-needed Change needs to be documented label Nov 29, 2023

ES-Alexander mentioned this pull request Nov 30, 2023

Improve video streaming multi-IP-route warning message #612

Merged

ES-Alexander added a commit to ES-Alexander/ardusub-zola that referenced this pull request Dec 12, 2023

advanced-usage: add video recording interface and warnings

3165933

Documents: - bluerobotics/cockpit#591 - bluerobotics/cockpit#573

ES-Alexander mentioned this pull request Dec 12, 2023

[Cockpit-devel] updates for first major release beta bluerobotics/ardusub-zola#143

Merged

12 tasks

ES-Alexander added a commit to ES-Alexander/ardusub-zola that referenced this pull request Dec 12, 2023

advanced-usage: add video recording interface and warnings

7b123ce

Documents: - bluerobotics/cockpit#591 - bluerobotics/cockpit#573

ES-Alexander added a commit to ES-Alexander/ardusub-zola that referenced this pull request Dec 12, 2023

advanced-usage: add video recording interface and warnings

e9e35cf

Documents: - bluerobotics/cockpit#591 - bluerobotics/cockpit#573

ES-Alexander added a commit to ES-Alexander/ardusub-zola that referenced this pull request Dec 13, 2023

advanced-usage: add video recording interface and warnings

c862ca9

Documents: - bluerobotics/cockpit#591 - bluerobotics/cockpit#573

ES-Alexander added a commit to bluerobotics/ardusub-zola that referenced this pull request Dec 13, 2023

advanced-usage: add video recording interface and warnings

c46565c

Documents: - bluerobotics/cockpit#591 - bluerobotics/cockpit#573

ES-Alexander added a commit to bluerobotics/ardusub-zola that referenced this pull request Dec 13, 2023

advanced-usage: add video recording interface and warnings

36dd6da

Documents: - bluerobotics/cockpit#591 - bluerobotics/cockpit#573

ES-Alexander added a commit to bluerobotics/ardusub-zola that referenced this pull request Dec 13, 2023

advanced-usage: add video recording interface and warnings

0cfb43b

Documents: - bluerobotics/cockpit#591 - bluerobotics/cockpit#573

ES-Alexander added docs-complete Change documentation has been completed and removed docs-needed Change needs to be documented labels Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn user when multiple IP routes are being used for the WebRTC streaming #591

Warn user when multiple IP routes are being used for the WebRTC streaming #591

rafaellehmkuhl commented Nov 24, 2023

joaoantoniocardoso left a comment •

edited

Loading

joaoantoniocardoso Nov 27, 2023

patrickelectric Nov 27, 2023

rafaellehmkuhl Nov 27, 2023

rafaellehmkuhl Nov 27, 2023

rafaellehmkuhl Nov 27, 2023

joaoantoniocardoso Nov 27, 2023 •

edited

Loading

rafaellehmkuhl Nov 27, 2023

joaoantoniocardoso Nov 27, 2023

rafaellehmkuhl Nov 27, 2023

rafaellehmkuhl Nov 27, 2023

patrickelectric left a comment

rafaellehmkuhl commented Nov 27, 2023

joaoantoniocardoso commented Nov 27, 2023

patrickelectric commented Nov 27, 2023 via email

rafaellehmkuhl commented Nov 27, 2023 •

edited

Loading

patrickelectric commented Nov 27, 2023

rafaellehmkuhl commented Nov 27, 2023

		<li>1. open the configuration of one of your video widgets</li>
		<li>2. choose the IP that should be used for the video streaming</li>

Warn user when multiple IP routes are being used for the WebRTC streaming #591

Warn user when multiple IP routes are being used for the WebRTC streaming #591

Conversation

rafaellehmkuhl commented Nov 24, 2023

joaoantoniocardoso left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaoantoniocardoso Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickelectric left a comment

Choose a reason for hiding this comment

rafaellehmkuhl commented Nov 27, 2023

joaoantoniocardoso commented Nov 27, 2023

patrickelectric commented Nov 27, 2023 via email

rafaellehmkuhl commented Nov 27, 2023 • edited Loading

patrickelectric commented Nov 27, 2023

rafaellehmkuhl commented Nov 27, 2023

joaoantoniocardoso left a comment •

edited

Loading

joaoantoniocardoso Nov 27, 2023 •

edited

Loading

rafaellehmkuhl commented Nov 27, 2023 •

edited

Loading