[1.x] Question about mvideoroom demo javascript code

[ioudas]

Hello,
I have a question about this line:

janus-gateway/html/mvideoroomtest.js

Line 594 in 03db74a

if(creatingFeed) {

1. The remoteFeed variable is set in success callback.
2. The creatingFeed variable is set in attached event handler

Since 1. always comes before 2. the line if(creatingFeed) doesn't seem to me like it serves it's intended purpose. When the handle is created but attached event has not arrived yet the code will send another subscribe anyway.

Which undesirable scenario is this code preventing?
Thank you

[lminiero]

It's meant to avoid we create the handle more than once. In that demo, we use a single handle/PeerConnection to receive all other participants, and so the first time we have to create it, for other participants joining/leaving it just needs to be updated.

[ioudas]

Thank you for reply, but I understand that intention. I didn't express myself clearly.
Let me rephrase the question: Why is creatingFeed set in 'attached' event handler and not in success callback of attach() method?

success always comes before attached therefore I don't think the creatingFeed condition can ever be executed (because handle will never be null if creatingFeed is still true.

[lminiero]

Because the success only tells you if we managed to attach to the plugin, while what we want to know is whether we managed to create a subscription (which involves further communication on that handle). As such, until we've managed to create a full subscription the first time, we prevent any further update.

[ioudas]

Yes, exactly, but in success the handle (remoteFeed) is set, therefore we will never get to the if(creatingFeed ) after that.

If the intent is to wait until attached, then either the conditions should be reversed - if(creatingFeed ) above if(remoteFeed) or remoteFeed must be set in attached handler.
I think former is more desirable (makes more sense to set handle the moment we got it).

[lminiero]

Ah ok, I think I understand your point now. You mean we should simply invert the checks, since creatingFeed is what gets the ball rolling, and the previous check may cause requests to be sent ahead of time. I do think it might make sense, especially in case of race conditions at startup (someone joins while you're creating the first subscription). Pinging @atoppi so he can have a look too.

[ioudas]

Yeah I should have phrased it this way in the first message :D

Thank for confirming the intent, I'm currently implementing the JS client in our project and I'll do it the same way (waiting for 'attached')

[atoppi]

If the intent is to wait until attached, then either the conditions should be reversed - if(creatingFeed ) above if(remoteFeed)

I agree.

[lminiero]

This commit should address it: I also modified the comments a bit to make the whole process easier to understand. Hopefully it should be less confusing now (for you and me both 😄 ). Thanks for spotting this!