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

Videoroom: ICE restarts as a subscriber #2537

Closed
remvst opened this issue Jan 27, 2021 · 4 comments
Closed

Videoroom: ICE restarts as a subscriber #2537

remvst opened this issue Jan 27, 2021 · 4 comments

Comments

@remvst
Copy link
Contributor

remvst commented Jan 27, 2021

Hey there,

I'm currently building an application using the videoroom plugin and was looking into ICE restarts. So far I think I got them to work for the publishing side, however I find the subscriber side a bit confusing when reading the docs.

Notice that the same exact steps we just went through (watch request, followed by JSEP offer by the plugin, followed by start request with JSEP answer by the viewer) is what you also use when renegotiations are needed, e.g., for the purpose of ICE restarts.

There is no mention of that watch request in the docs (and I get an error if I fire one), so I assumed it was a join request instead.

So I tried to send the following message:

{
  janus: 'message',
  body: {
    request: 'join',
    ptype: 'subscriber',
    room: 2329283243204937,
    feed: 1611781504646,
    audio: true,
    video: true,
    data: true
  },
  transaction: 'TODO0.08565046641877228',
  apisecret: 'janussecret'
}

which gives me an ack back:

{
  janus: 'ack',
  session_id: 6065274145616990,
  transaction: 'TODO0.08565046641877228'
}

and later on an error event (and also an error in Janus' logs):

{
  janus: 'event',
  session_id: 6065274145616990,
  transaction: 'TODO0.08565046641877228',
  sender: 4863436952761774,
  plugindata: {
    plugin: 'janus.plugin.videoroom',
    data: {
      videoroom: 'event',
      error_code: 425,
      error: 'Already in as a subscriber on this handle'
    }
  }
}

This seems fair but still prevents me from getting a new offer from Janus. Is there another (maybe undocumented) request that needs to be fired in order to perform ICE restarts for subscribers?

@tmatth
Copy link
Contributor

tmatth commented Jan 27, 2021

There is an issue in the documentation here, as you point out watch is not a valid videoroom request. I believe the sequence you want is:

  • join as subscriber you've already joined, so this step isn't needed
  • configure with restart:true
  • this responds with configured + the JSEP you need to process and send an answer for with a start request to complete the sequence

Hopefully someone more confident with the semantics here can submit a documentation fix PR :)

@remvst
Copy link
Contributor Author

remvst commented Jan 27, 2021

There is an issue in the documentation here, as you point out watch is not a valid videoroom request. I believe the sequence you want is:

* ~`join` as subscriber~ you've already joined, so this step isn't needed

* `configure` with `restart:true`

* this responds with `configured` + the JSEP you need to process and send an answer for with a `start` request to complete the sequence

Thank you for the quick reply, will try that and report back ;)

@remvst
Copy link
Contributor Author

remvst commented Jan 27, 2021

Can confirm I received a new offer with this solution (don't forget restart field in the configure request).

So I think there are two issues in the docs:

  • the watch request part should be replaced with a configure request
  • the restart field should be mentioned in the configure docs

I'd submit a PR but I'm also not super confident about the exact semantics (and also wouldn't want to take credit from you actually coming up with the solution)

@lminiero
Copy link
Member

That's a copy/paste type, since watch is a Streaming plugin request, and that's indeed how you restart subscriptions there (you do a watch+start again). Tristan is right, you use configure with a restart:true in the VideoRoom to ask the plugin to send a new offer that originates an ICE restart. I'll update the docs.

BogdanovKirill pushed a commit to 3dEYE/janus-gateway that referenced this issue Mar 10, 2021
commit caaba91
Author: Tijmen de Mes <tijmen@ag-projects.com>
Date:   Tue Feb 23 14:57:17 2021 +0100

    Added Content type to SIP message (meetecho#2567)

    * Added 'content_type' to received SIP MESSAGE
    * Added optional content type in sending SIP MESSAGE

commit c9baba9
Author: Alessandro Toppi <atoppi@meetecho.com>
Date:   Tue Feb 23 11:46:50 2021 +0100

    clang/ubsan fixes (meetecho#2556)

    * Fix some clang warnings.
    * Fix UBSanitizer error when sending RTCP SR.

commit beb28be
Author: Tvildo <davidtamuna@gmail.com>
Date:   Mon Feb 22 09:46:25 2021 -0800

    add call_id in received sip message (meetecho#2563)

    Add call_id in received SIP MESSAGE and INFO

commit 8246452
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Mon Feb 22 11:51:36 2021 +0100

    Fixed missing mutexes around VideoRoom ACL management

commit 4f8943a
Author: Tristan Matthews <tmatth@videolan.org>
Date:   Wed Feb 17 09:32:57 2021 -0500

    ice: fix conncheck typo (meetecho#2560)

    No functional change since the typo was used consistently.

commit 27dc51a
Author: nicolasduteil <nduteil@freedev.org>
Date:   Wed Feb 17 15:27:45 2021 +0100

    feat: add "call_id" to "calling", "declining", "updatingcall" & "incomingcall" events (meetecho#2557)

commit 2c81d02
Author: Hritik Utekar <hritikdrocks@gmail.com>
Date:   Wed Feb 17 19:54:46 2021 +0530

    Video moderation always returns unmuted (meetecho#2559)

commit 6503f42
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Wed Feb 17 13:53:53 2021 +0100

    Fixed typo in keepalive-conncheck usage

commit 1f45e02
Author: Alessandro Toppi <atoppi@meetecho.com>
Date:   Mon Feb 15 16:38:22 2021 +0100

    Set specific versions for Python 3 and meson in janus-ci yml.

commit d7c9ef0
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Mon Feb 15 15:28:48 2021 +0100

    Added audiocodec/videocodec supporto to 'joinandconfigure' in VideoRoom API

commit ad54495
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Fri Feb 12 15:28:21 2021 +0100

    Add new option to configure ICE nomination mode, if libnice is recent enough (meetecho#2541)

    * Add new option to configure ICE nomination mode, if libnice is recent enough
    * Added support for libnice keepalive-conncheck property

commit af8cc6e
Author: Nadin Zajimovic <zayim92@gmail.com>
Date:   Fri Feb 12 09:40:36 2021 +0100

    if inviting on destroy, send BYE instead of 480 response (meetecho#2554)

commit ad8bf79
Author: Alessandro Amirante <alex@meetecho.com>
Date:   Thu Feb 11 17:49:25 2021 +0100

    Fix typo in videoroom docs.

commit 26f5958
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Wed Feb 10 16:51:19 2021 +0100

    Fixed small leak in VideoRoom

commit 8ab7a00
Author: Alessandro Toppi <atoppi@meetecho.com>
Date:   Tue Feb 9 16:51:37 2021 +0100

    Initialize packet.is_rtp to false.

commit 66cf343
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Tue Feb 9 16:05:37 2021 +0100

    Add resolution and bitrate to Record&Play playback

commit 119d220
Author: Aleksander Guryanov <caiiiycuk@gmail.com>
Date:   Tue Feb 9 20:33:23 2021 +0700

    Update janus.d.ts (meetecho#2553)

    Function getBitrate() actually returns a string

commit 41399db
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Mon Feb 8 16:27:14 2021 +0100

    Allow up to 5 (rather than 3) audio/video codecs in the same VideoRoom

commit b81dd6d
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Mon Feb 8 16:26:19 2021 +0100

    Allow forcing audio/video codec for VideoRoom publishers via query string

commit 576abf5
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Mon Feb 8 15:17:56 2021 +0100

    Initialize VideoRoom participant recording state when room recording is active (fixes meetecho#2550)

commit 0ba74fb
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Mon Feb 8 11:53:42 2021 +0100

    Fixed broken AV1 post-processing

commit 09daec4
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Mon Feb 8 10:46:06 2021 +0100

    Renamed extern janus_callbacks variables in Lua and Duktape plugins (meetecho#2540)

commit 664022b
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Mon Feb 8 10:41:38 2021 +0100

    Bumped to version 0.11.1

commit 7732127
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Mon Feb 8 10:37:41 2021 +0100

    Updated Changelog (0.10.10)

commit 24a0eec
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Thu Feb 4 11:53:36 2021 +0100

    Videoroom race condition fixes (see meetecho#2509) (meetecho#2539)

    * Fixed missing room references that could cause crashes during race conditions
    * Fixed rare race condition on publisher join

commit 62440c5
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Thu Feb 4 11:52:44 2021 +0100

    Fix parsing of SDP to find payload type matching profiles (fixes meetecho#2544) (meetecho#2549)

commit 794e89a
Author: Bender <undead-empire@ya.ru>
Date:   Wed Feb 3 20:12:02 2021 +0300

    janus.js (meetecho#2548)

    customizeSdp callback added to handleRemoteJsep to be able to mangle remote SDP if needed

commit 213b6c7
Author: Alessandro Toppi <atoppi@meetecho.com>
Date:   Fri Jan 29 12:13:44 2021 +0100

    Make compiler fail if implicit-function-declaration is encountered.

commit dfa8016
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Fri Jan 29 10:21:50 2021 +0100

    Fixed non-portable call to strlcpy, and comment styles, in RabbitMQ code (see meetecho#2430)

commit b7b1e9e
Merge: 19ecf48 c0f0e1e
Author: Alessandro Toppi <atoppi@meetecho.com>
Date:   Fri Jan 29 08:02:18 2021 +0100

    Merge pull request meetecho#2430 from vgrid/master

    Updates RabbitMQ logic

commit 19ecf48
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Thu Jan 28 11:54:55 2021 +0100

    Fixed VideoRoom docs on ICE Restarts for subscribers (fixes meetecho#2537)

commit 2454802
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Wed Jan 27 11:22:13 2021 +0100

    Allow marking of RTP extensions in MJR recordings (meetecho#2527)

commit 0bb49bc
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Wed Jan 27 11:21:09 2021 +0100

    Moderator based muting/unmuting of VideoRoom streams (meetecho#2513)

commit 5e685e3
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Wed Jan 27 11:19:35 2021 +0100

    Reject a=extmap-allow-mixed in SDP, when offered

commit c0f0e1e
Author: Chris Wiggins <chris@wiggins.nz>
Date:   Wed Jan 27 09:59:02 2021 +1300

    Fix code style comments, also enable routing for direct exchanges

commit 257eb80
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Tue Jan 26 15:00:39 2021 +0100

    Configurable media direction when putting calls on-hold (SIP plugin) (meetecho#2525)

commit 7fb08c2
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Tue Jan 26 12:40:26 2021 +0100

    Added starting DTLS MTU to info returned by Janus API

commit 4d97028
Author: Sami Kuhmonen <sami@tokavuh.com>
Date:   Tue Jan 26 12:53:14 2021 +0200

    Report fail if binding to a socket fails in websockets (meetecho#2534)

commit 674367a
Author: Evgeniy Baranov <jeck.ru@gmail.com>
Date:   Mon Jan 25 12:00:37 2021 +0300

    fix race condition in audiobridge plugin changeroom request (meetecho#2535)

commit 3edb780
Author: Alberto Gonzalez Trastoy <albertogontras@gmail.com>
Date:   Sat Jan 23 14:01:27 2021 -0500

    Janus npm types upgrade (meetecho#2528)

commit 78434aa
Author: August Black <augustblack@gmail.com>
Date:   Sat Jan 23 12:00:52 2021 -0700

    set webrtc-adapter verstion to 7.4.0 (meetecho#2531)

commit 46a6c71
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Thu Jan 21 13:02:06 2021 +0100

    Reduced verbosity of a few LOG_WARN messages at startup

commit 34f6f89
Author: Andrew Lavrentev <andrew.lavrentev@gmail.com>
Date:   Thu Jan 21 14:48:20 2021 +0300

    Feature/enhance typings (meetecho#2518)

commit 16173af
Author: Rémi Vansteelandt <remvst@gmail.com>
Date:   Thu Jan 21 05:39:17 2021 -0500

    Fixed secret authentication on GET requests (meetecho#2524)

commit 62d75ab
Author: Nadin Zajimovic <zayim92@gmail.com>
Date:   Wed Jan 20 11:36:43 2021 +0100

    Dont send bye on early dialog (meetecho#2521)

commit 2141d9b
Author: Yurii Cherniavskyi <yurii.cherniavskyi@gmail.com>
Date:   Wed Jan 20 12:30:30 2021 +0200

    Update Webpack instruction after webrtc-adapter dependency update (meetecho#2519)

commit f994f7c
Author: fbellet <fabrice@bellet.info>
Date:   Wed Jan 20 11:28:17 2021 +0100

    Close nice agent resources asynchronously (meetecho#2492)

commit 79038e0
Author: Sergey Radionov <RSATom@gmail.com>
Date:   Tue Jan 19 18:16:03 2021 +0700

    mqttevh: tls support implementation finished (meetecho#2517)

    * mqttevh: tls support implementation finished
    * mqttevh: MQTTASYNC_OPERATION_INCOMPLETE is not error
    * mqttevh: allow send messages while connecting is still in progress

commit 97cd054
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Mon Jan 18 11:24:48 2021 +0100

    Fixed broken webrtc-adapter links (see meetecho#2515)

commit c0570a9
Author: Tristan Matthews <tmatth@videolan.org>
Date:   Thu Jan 14 13:24:48 2021 -0500

    html: update webrtc-adapter to 7.7.0 (meetecho#2515)

commit f57215a
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Thu Jan 14 10:11:57 2021 +0100

    Updated year in demos and docs

commit bbdd3e4
Author: Chris Wiggins <chris@wiggins.nz>
Date:   Tue Nov 17 11:49:42 2020 +1300

    Adds back in default outgoing queue behaviour. Adds support for auto-generated queue_names

commit ed1b5c6
Author: Chris Wiggins <chris@wiggins.nz>
Date:   Thu Nov 12 13:21:08 2020 +1300

    Adds RabbitMQ options for queues, durable, exclusive and autodelete

commit 24594f7
Author: Chris Wiggins <chris@wiggins.nz>
Date:   Wed Nov 11 18:05:24 2020 +1300

    Check RabbitMQ admin topic in a better way

commit 319c6fc
Author: Chris Wiggins <chris@wiggins.nz>
Date:   Wed Nov 11 16:09:26 2020 +1300

    Increase RabbitMQ logging on publish

commit 505eeef
Author: Chris Wiggins <chris@wiggins.nz>
Date:   Tue Nov 10 18:29:59 2020 +1300

    Fix queue_name_admin in rabbitmq transport

commit b3f7ad9
Author: Chris Wiggins <chris@wiggins.nz>
Date:   Tue Nov 10 18:19:07 2020 +1300

    Update rabbitmq logging information

commit f604aeb
Author: Chris Wiggins <chris@wiggins.nz>
Date:   Tue Nov 10 17:23:11 2020 +1300

    Updates RabbitMQ logic

    - Publishing to a topic does not require an outgoing queue, just the topic, so the outgoing queues are no longer declared
    - When the janus_exchange_type is topic, we want to be able to name the queue, and then bind an incoming topic from the exchange to that queue, so that functionality has been added
    - This is all backwards compatible with original logic, and won't break existing logic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants